Skip to content

Comments

[flake8-pyi] Fix several correctness issues with custom-type-var-return-type (PYI019)#15851

Merged
AlexWaygood merged 4 commits intomainfrom
alex/pyi-panic
Jan 31, 2025
Merged

[flake8-pyi] Fix several correctness issues with custom-type-var-return-type (PYI019)#15851
AlexWaygood merged 4 commits intomainfrom
alex/pyi-panic

Conversation

@AlexWaygood
Copy link
Member

Summary

This PR fixes several correctness issues in our PYI019 rule:

  • The rule would panic on this snippet. There shouldn't even be a diagnostic on this snippet, since it's not a valid use of the S type variable:
    class F:
        @classmethod
        def m[S](cls: type[S], /) -> S[int]: ...
  • The rule would panic on this snippet. Again, there shouldn't even be a diagnostic emitted here; it's an invalid use of a type variable:
    class F:
        def m[S](self: S) -> S[int]: ...
  • The rule had a false positive on this snippet, where type is shadowed locally:
    def shadowed_type():
        type = 1
        class A:
            @classmethod
            def m[S](cls: type[S]) -> S: ...
  • The rule had a false negative on this snippet, where the fully qualified version of builtins.type rather than the implicit builtin binding:
    import builtins
    
    class UsesFullyQualifiedType:
        @classmethod
        def m[S](cls: builtins.type[S]) -> S: ...  # PYI019
  • The rule had a false negative on this snippet, where the return annotation is type[S] rather than simply S:
    class Foo:
        @classmethod
        def f[S](cls: type[S]) -> type[S]: ...

Fixes #15849

Test Plan

I've added new fixtures added that trigger panics and/or incorrect behaviour on main.

@AlexWaygood AlexWaygood added bug Something isn't working rule Implementing or modifying a lint rule labels Jan 31, 2025
@AlexWaygood AlexWaygood requested review from dylwil3 and ntBre January 31, 2025 13:35
@github-actions
Copy link
Contributor

github-actions bot commented Jan 31, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

This looks great! I had to throw in one nit, but the changes were easy to follow, especially with the new variable names, and the tests made it clear what was going on.

@AlexWaygood AlexWaygood enabled auto-merge (squash) January 31, 2025 14:14
@AlexWaygood AlexWaygood merged commit 44ac17b into main Jan 31, 2025
20 checks passed
@AlexWaygood AlexWaygood deleted the alex/pyi-panic branch January 31, 2025 14:19
dcreager added a commit that referenced this pull request Jan 31, 2025
* main:
  [`flake8-pyi`] Fix several correctness issues with `custom-type-var-return-type` (`PYI019`) (#15851)
  [`pyupgrade`] Reuse replacement logic from `UP046` and `UP047` (`UP040`) (#15840)
  [`refurb`] Avoid `None | None` as well as better detection and fix (`FURB168`) (#15779)
  Remove non-existing `lint.extendIgnore` editor setting (#15844)
  [`refurb`] Mark fix as unsafe if there are comments (`FURB171`) (#15832)
  [`flake8-comprehensions`] Skip when `TypeError` present from too many (kw)args for `C410`,`C411`, and `C418` (#15838)
  [`pyflakes`] Visit forward annotations in `TypeAliasType` as types (`F401`) (#15829)
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.

Panic in PYI019 autofix

2 participants