-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Fix false-positive diagnostics on super() calls
#20814
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
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
3441f30 to
129a1d0
Compare
129a1d0 to
23c6f89
Compare
7cdcb6c to
13a31f3
Compare
13a31f3 to
545873d
Compare
|
With this PR, the |
sharkdp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
I mostly reviewed the implementation and trust you on the semantics here (or to pull someone in for a second review if you are unsure about parts of it).
I checked all the Some parts of this I'm slightly unsure about are:
I think we can easily iterate on any of these, which is why I merged despite being unsure about them 😄 |
…tity * origin/main: (24 commits) Update Python compatibility from 3.13 to 3.14 in README.md (#20852) [syntax-errors]: break outside loop F701 (#20556) [ty] Treat `Callable`s as bound-method descriptors in special cases (#20802) [ty] Do not bind self to non-positional parameters (#20850) Fix syntax error false positives on parenthesized context managers (#20846) [ty] Remove 'pre-release software' warning (#20817) Render unsupported syntax errors in formatter tests (#20777) [ty] Treat functions, methods, and dynamic types as function-like `Callable`s (#20842) [ty] Move logic for `super()` inference to a new `types::bound_super` submodule (#20840) [ty] Fix false-positive diagnostics on `super()` calls (#20814) [ty] Move `class_member` to `member` module (#20837) [`ruff`] Use DiagnosticTag for more flake8 and numpy rules (#20758) [ty] Prefer declared base class attribute over inferred attribute on subclass (#20764) [ty] Log files that are slow to type check (#20836) Update cargo-bins/cargo-binstall action to v1.15.7 (#20827) Update CodSpeedHQ/action action to v4.1.1 (#20828) Update Rust crate pyproject-toml to v0.13.7 (#20835) Update Rust crate anstream to v0.6.21 (#20829) Update Rust crate libc to v0.2.177 (#20832) Update Rust crate memchr to v2.7.6 (#20834) ...
* main: (25 commits) [ty] Diagnostic for generic classes that reference typevars in enclosing scope (#20822) Update Python compatibility from 3.13 to 3.14 in README.md (#20852) [syntax-errors]: break outside loop F701 (#20556) [ty] Treat `Callable`s as bound-method descriptors in special cases (#20802) [ty] Do not bind self to non-positional parameters (#20850) Fix syntax error false positives on parenthesized context managers (#20846) [ty] Remove 'pre-release software' warning (#20817) Render unsupported syntax errors in formatter tests (#20777) [ty] Treat functions, methods, and dynamic types as function-like `Callable`s (#20842) [ty] Move logic for `super()` inference to a new `types::bound_super` submodule (#20840) [ty] Fix false-positive diagnostics on `super()` calls (#20814) [ty] Move `class_member` to `member` module (#20837) [`ruff`] Use DiagnosticTag for more flake8 and numpy rules (#20758) [ty] Prefer declared base class attribute over inferred attribute on subclass (#20764) [ty] Log files that are slow to type check (#20836) Update cargo-bins/cargo-binstall action to v1.15.7 (#20827) Update CodSpeedHQ/action action to v4.1.1 (#20828) Update Rust crate pyproject-toml to v0.13.7 (#20835) Update Rust crate anstream to v0.6.21 (#20829) Update Rust crate libc to v0.2.177 (#20832) ...
…rable * origin/main: (26 commits) [ty] Add separate type for typevar "identity" (#20813) [ty] Diagnostic for generic classes that reference typevars in enclosing scope (#20822) Update Python compatibility from 3.13 to 3.14 in README.md (#20852) [syntax-errors]: break outside loop F701 (#20556) [ty] Treat `Callable`s as bound-method descriptors in special cases (#20802) [ty] Do not bind self to non-positional parameters (#20850) Fix syntax error false positives on parenthesized context managers (#20846) [ty] Remove 'pre-release software' warning (#20817) Render unsupported syntax errors in formatter tests (#20777) [ty] Treat functions, methods, and dynamic types as function-like `Callable`s (#20842) [ty] Move logic for `super()` inference to a new `types::bound_super` submodule (#20840) [ty] Fix false-positive diagnostics on `super()` calls (#20814) [ty] Move `class_member` to `member` module (#20837) [`ruff`] Use DiagnosticTag for more flake8 and numpy rules (#20758) [ty] Prefer declared base class attribute over inferred attribute on subclass (#20764) [ty] Log files that are slow to type check (#20836) Update cargo-bins/cargo-binstall action to v1.15.7 (#20827) Update CodSpeedHQ/action action to v4.1.1 (#20828) Update Rust crate pyproject-toml to v0.13.7 (#20835) Update Rust crate anstream to v0.6.21 (#20829) ...
Summary
This PR fixes a variety of false-positive
invalid-super-argumentdiagnostics that we can emit onmain. These aren't that common in the ecosystem at the moment, but they will become much more common when we infer a good type forselfin method bodies. #20723 (comment) indicates that if we added an improved type ofselfright now, it would lead to nearly 500 newinvalid-super-argumentdiagnostics being added, and I'd guess that nearly all of those are false positives.There were two problems with our current logic: this
matchwas not an exhaustivematch, and was returningNonefor a large number of types. In reality, nearly all objects are valid as the second argument to asuper(x, y)call as long as they are an instance or subclass of the first argument. (The same is true if the arguments are provided implicitly by the interpreter, as is usually the case.) This PR makes thematchexhaustive, and significantly reduces the number of types which we disallow as the second argument tosuper(): we now only disallow "purely structural" types such asCallabletypes and synthesized protocols:ruff/crates/ty_python_semantic/src/types.rs
Lines 11332 to 11359 in 7064c38
The second problem relates to this
is_subclass_ofcall here:ruff/crates/ty_python_semantic/src/types.rs
Lines 11450 to 11454 in 7064c38
With our current implementation of
is_subclass_of,list[Unknown]is not recognised as being a "subclass of"Sequence[int], for example, becauseSequence[int]does not literally appear in the MRO oflist[Unknown](Sequence[Unknown]does). We've discussed elsewhere that this is a bit of a footgun since it doesn't really correspond to the subclass relationship between class objects at runtime, but for now I just work around the footgun :-)Fixes astral-sh/ty#987, fixes astral-sh/ty#697
Test plan
Mdtests and snapshots