-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Use Top materializations for TypeIs special form
#20591
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 ✅ |
|
3cb9437 to
2afd6a9
Compare
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-argument-type |
7 | 9 | 7 |
unresolved-attribute |
1 | 0 | 12 |
possibly-missing-attribute |
0 | 2 | 9 |
invalid-assignment |
2 | 0 | 7 |
invalid-return-type |
2 | 2 | 5 |
invalid-type-form |
0 | 0 | 3 |
not-iterable |
0 | 0 | 2 |
too-many-positional-arguments |
2 | 0 | 0 |
unsupported-operator |
0 | 0 | 2 |
no-matching-overload |
0 | 1 | 0 |
unused-ignore-comment |
1 | 0 | 0 |
| Total | 15 | 14 | 47 |
Ecosystem analysisOverall this PR adds about as many diagnostics as it removes, but I think it makes our narrowing behaviour for There are several hits that look like true positives: we're now inferring less dynamic types which is allowing us to highlight places where the code arguably is unsound ( The new diagnostic on We have some new false positives in ruff/crates/ty_python_semantic/src/types/signatures.rs Lines 1286 to 1291 in 57e1ff8
We have a new false positive on |
2afd6a9 to
ad032c5
Compare
No, it should be |
Right, yes, sorry |
Top materializations for TypeIs special formTop materializations for TypeIs special form
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.
I don't love implicitly transforming a type explicitly written by the user :/ but I do feel like this practically improves the results, given the way TypeIs is currently used in typeshed. Narrowing with a gradual type is just not really very useful, if it's handled precisely (as we do).
I feel like the user intent expressed by -> TypeIs[Foo[Any]] is "the type is some kind of Foo, I don't know which kind" (this is accurately modeled by taking the top materialization), but also then "treat the contained type of Foo as dynamic if I end up using it" -- and this latter part is what we don't do.
I wonder what it would look like if, instead of doing this, we simplified Foo[Any] & Foo[Bar] to Foo[Bar & Any]? I think that's a correct simplification? And I think that would give the same benefits as this PR, but with fewer false positives -- it would better respect the user's intent in writing TypeIs[Foo[Any]]. WDYT?
Edit: I guess then we'd have false negatives compared to other type checkers, who are happy to simplify Foo[Any] & Foo[Bar] to just Foo[Bar]. Which is not correct, but is maybe practically what users expect.
carljm
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.
Going ahead and approving this -- I don't love it, but it seems like a net improvement for now, and it's easy to reverse if we come up with a better idea.
That's only a correct simplification if |
* main: (21 commits) [ty] Literal promotion refactor (#20646) [ty] Add tests for nested generic functions (#20631) [`cli`] Add conflict between `--add-noqa` and `--diff` options (#20642) [ty] Ensure first-party search paths always appear in a sensible order (#20629) [ty] Use `typing.Self` for the first parameter of instance methods (#20517) [ty] Remove unnecessary `parsed_module()` calls (#20630) Remove `TextEmitter` (#20595) [ty] Use fully qualified names to distinguish ambiguous protocols in diagnostics (#20627) [ty] Ecosystem analyzer: relax timeout thresholds (#20626) [ty] Apply type mappings to functions eagerly (#20596) [ty] Improve disambiguation of class names in diagnostics (#20603) Add the *The Basics* title back to CONTRIBUTING.md (#20624) [`playground`] Fix quick fixes for empty ranges in playground (#20599) Update dependency ruff to v0.13.2 (#20622) [`ruff`] Fix minor typos in doc comments (#20623) Update dependency PyYAML to v6.0.3 (#20621) Update cargo-bins/cargo-binstall action to v1.15.6 (#20620) Fixed documentation for try_consider_else (#20587) [ty] Use `Top` materializations for `TypeIs` special form (#20591) [ty] Simplify `Any | (Any & T)` to `Any` (#20593) ...
Summary
Typeshed has many functions that return
TypeIs[SomeCovariantGeneric[Any]]. This is somewhat unfortunate for ty, because for our purposes it's almost always better to intersect withCovariantGeneric[object]for the purposes of type narrowing, but it's somewhat hard to change these in typeshed without creating false-positive diagnostics for users of other type checkers. This PR therefore introduces a workaround for the typeshed issue: under the hood, we convert the type argument passed toTypeIsto its top materialization before intersecting with that type.Test Plan
I added an mdtest that fails on
mainwithout this change. It's similar to the false-positive diagnostics relating to the use ofasyncio.iscoroutinethat we see going away onhomeassistant/corein the mypy_primer report.Some more ecosystem analysis is found in #20591 (comment).