Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refine pyupgrade's TimeoutErrorAlias lint (UP041) to remove false positives #8587

Merged
merged 3 commits into from
Nov 10, 2023

Conversation

BurntSushi
Copy link
Member

@BurntSushi BurntSushi commented Nov 9, 2023

Previously, this lint had its alias detection logic a little
backwards. That is, for Python 3.11+, it would only detect
asyncio.TimeoutError as an alias, but it should have also detected
socket.timeout as an alias. And in Python <3.11, it would falsely
detect asyncio.TimeoutError as an alias where it should have only
detected socket.timeout as an alias.

We fix it so that both asyncio.TimeoutError and socket.timeout are
detected as aliases in Python 3.11+, and only socket.timeout is
detected as an alias in Python 3.10.

Fixes #8565

Test Plan

I tested this by updating the existing snapshot test which had erroneously
asserted that socket.timeout should not be replaced with TimeoutError in Python
3.11+. I also added a new regression test that targets Python 3.10 and ensures
that the suggestion to replace asyncio.TimeoutError with TimeoutError does not
occur.

Copy link
Contributor

github-actions bot commented Nov 9, 2023

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@alex-700
Copy link
Contributor

alex-700 commented Nov 9, 2023

@BurntSushi hi! Thanks for the PR!

If I understand correctly, you decide to leave linting asyncio.TimeoutError even for py310 (but make the fix unsafe).
IMHO, it looks a bit counterintuitive.
The user, targeted to py310, can do nothing with the triggered lint about asyncio.TimeoutError, but suppressing.
Should we just do not produce a warning about asyncio.TimeoutError in the case of py310?

@charliermarsh
Copy link
Member

Ah yeah, I think we may have misdiagnosed in the original issue. We probably shouldn't raise on asyncio.TimeoutError in Python 3.10 at all. The initial PR actually suggests that this was the intention:

Screen Shot 2023-11-09 at 5 08 56 PM

But then has this code:

/// Return `true` if an [`Expr`] is an alias of `TimeoutError`.
fn is_alias(expr: &Expr, semantic: &SemanticModel, target_version: PythonVersion) -> bool {
    semantic.resolve_call_path(expr).is_some_and(|call_path| {
        if target_version >= PythonVersion::Py311 {
            matches!(call_path.as_slice(), [""] | ["asyncio", "TimeoutError"])
        } else {
            matches!(
                call_path.as_slice(),
                [""] | ["asyncio", "TimeoutError"] | ["socket", "timeout"]
            )
        }
    })
}

We probably want to return both errors if >= Python 3.11, and only socket.timeout error if Python 3.10?

@alex-700
Copy link
Contributor

@charliermarsh yes.
Something like

/// Return `true` if an [`Expr`] is an alias of `TimeoutError`.
fn is_alias(expr: &Expr, semantic: &SemanticModel, target_version: PythonVersion) -> bool {
    semantic.resolve_call_path(expr).is_some_and(|call_path| {
        if target_version >= PythonVersion::Py311 {
            matches!(
                call_path.as_slice(),
                [""] | ["asyncio", "TimeoutError"] | ["socket", "timeout"]
            )
        } else {
            matches!(call_path.as_slice(), [""] | ["socket", "timeout"])
        }
    })
}

^ to minimize a difference from the current code.

Or maybe something like

/// Return `true` if an [`Expr`] is an alias of `TimeoutError`.
fn is_alias(expr: &Expr, semantic: &SemanticModel, target_version: PythonVersion) -> bool {
    semantic
        .resolve_call_path(expr)
        .is_some_and(|call_path| match call_path.as_slice() {
            [""] => true,
            ["socket", "timeout"] if target_version >= PythonVersion::Py310 => true,
            ["asyncio", "TimeoutError"] if target_version >= PythonVersion::Py311 => true,
            _ => false,
        })
}

or (implicitly used the information that target_version >= py310)

/// Return `true` if an [`Expr`] is an alias of `TimeoutError`.
fn is_alias(expr: &Expr, semantic: &SemanticModel, target_version: PythonVersion) -> bool {
    semantic
        .resolve_call_path(expr)
        .is_some_and(|call_path| {
            matches!(call_path.as_slice(), [""] | ["socket", "timeout"])
            || matches!(call_path.as_slice(), ["asyncio", "TimeoutError"] if target_version >= PythonVersion::Py311)
        })
}

P.S. I am actually does not understand why the check for [""] is here 😄

@BurntSushi
Copy link
Member Author

Right, the unsafe fix on Python 3.10 for asyncio.TimeoutError was what I was uncertain about. I structured this PR such that all I have to do is drop the last commit. I fixed the logic as @charliermarsh indicated above in the first commit. :-)

@BurntSushi
Copy link
Member Author

Okay, I've dropped the last commit and updated the PR description.

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thanks!

Previously, this lint had its alias detection logic a little
backwards. That is, for Python 3.11+, it would *only* detect
asyncio.TimeoutError as an alias, but it should have also detected
socket.timeout as an alias. And in Python <3.11, it would falsely
detect asyncio.TimeoutError as an alias where it should have only
detected socket.timeout as an alias.

We fix it so that both asyncio.TimeoutError and socket.timeout are
detected as aliases in Python 3.11+, and only socket.timeout is
detected as an alias in Python 3.10.
Without this assert, the lint could falsely trigger a safe fix
that is actually unsafe if the higher level logic calling the lint
changes. That is, right now, the lint is only invoked for 3.10+
so everything is fine, but nothing is stopping that from changing.
So we make the assumption clear: if it changes, then we should get
a pretty loud panic.
This new test checks that safe fixes are not suggested for replacing
asyncio.TimeoutError with TimeoutError in Python <3.11. Namely, before
3.11, these were distinct types. So switching from asyncio.TimeoutError
to TimeoutError could result in a change of semantics.
@zanieb zanieb added bug Something isn't working rule Implementing or modifying a lint rule labels Nov 10, 2023
@BurntSushi BurntSushi merged commit a7dbe9d into main Nov 10, 2023
17 checks passed
@BurntSushi BurntSushi deleted the ag/fix-8565 branch November 10, 2023 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UP041 false-positive in the case of target=py310
4 participants