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

Issue with new PYI034 autofix when first param is annotated #14421

Closed
Avasam opened this issue Nov 18, 2024 · 5 comments · Fixed by #14801
Closed

Issue with new PYI034 autofix when first param is annotated #14421

Avasam opened this issue Nov 18, 2024 · 5 comments · Fixed by #14801
Labels
bug Something isn't working fixes Related to suggested fixes for violations

Comments

@Avasam
Copy link

Avasam commented Nov 18, 2024

Thanks @InSyncWithFoo for adding an autofix in #14217 .

  • A minimal code snippet that reproduces the bug.

I did notice an issue that I think should be possible to handle. This case:

class Container(tuple):
    def __new__(cls: Type[Container], *args, **kwargs) -> Container: ...

gets autofixed to

class Container(tuple):
    def __new__(cls: Type[Container], *args, **kwargs) -> Self: ...

Which causes type error:

error: "Self" cannot be used in a function with a `self` or `cls` parameter that has a type annotation other than "Self" (reportGeneralTypeIssues)

I think it should be:

class Container(tuple):
    def __new__(cls, *args, **kwargs) -> Self: ...
  • The command you invoked (e.g., ruff /path/to/file.py --fix), ideally including the --isolated flag.

ruff check --select=PYI034 --fix --preview --unsafe-fixes --isolated

  • The current Ruff settings (any relevant sections from your pyproject.toml).

None

  • The current Ruff version (ruff --version).

ruff 0.7.4

@InSyncWithFoo
Copy link
Contributor

Admittedly, the fix is unsafe here (and, indeed, it was marked as such), but I'm not sure if this falls within the scope of PYI034. I would say it is closer to PYI019.

@Avasam
Copy link
Author

Avasam commented Nov 18, 2024

I don't think this falls under PYI019, there are no typevar used here.

Ruff probably doesn't need to try and fix code like def __new__(cls: Type[Container], *args, **kwargs) -> Self: ... when it's encountered in the wild, as it's invalid to begin with. So I'm not asking for a rule to fix it here, but it shouldn't produce it either if possible.

The autofix is indeed unsafe, as there are cases where you wouldn't want to use Self. But assuming this is not one of those cases, it's correct to attempt an autofix here, the current implementation just doesn't yet handle it.


However (and if) this case ends up being handled, both known reasons for this fix to be unsafe could be documented.

  1. Intentionally returning a specific class from __new__
  2. May introduce type issues, for instance if the self/cls param is annotated)

@Avasam
Copy link
Author

Avasam commented Nov 18, 2024

Maybe Ruff should have a rule to disallow (and remove) non-typevar annotations on self/cls? 🤔
That'd solve the issue and this issue could be closed with only documenting the fix safety.

@MichaReiser
Copy link
Member

MichaReiser commented Nov 18, 2024

@InSyncWithFoo If I recall, then #14238 handles type annotations in self/cls positions correctly. Any concerns with detecting type annotation for self/cls that can't be replaced with Self and bailing out of the fix?

@MichaReiser MichaReiser added bug Something isn't working fixes Related to suggested fixes for violations labels Nov 18, 2024
@InSyncWithFoo
Copy link
Contributor

InSyncWithFoo commented Nov 18, 2024

I think it would only be safe to remove the annotation (not just replace) when:

  • self is annotated with Container and cls with type[Container].
  • Container is not generic.

Consider this contrived example:

# Before
class Container[T]:
    def __init__(self, v: T, /) -> None: ...
    def map(self: Container, mapper: Callable, /) -> Container: ...
#                 ^^^^^^^^^     `Container[Any]`     ^^^^^^^^^

# After
class Container[T]:
    def __init__(self, v: T, /) -> None: ...
    def map(self, mapper: Callable, /) -> Self: ...
#                          `Container[T]` ^^^^
c = Container(0)  # Container[int]

# Before
reveal_type(c.map(lambda _: ''))  # Container[Any]
# After
reveal_type(c.map(lambda _: ''))  # Container[int]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixes Related to suggested fixes for violations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants