[ty] Reachability analysis for isinstance(…) branches
#19503
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sharkdp
commented
Jul 23, 2025
| elif isinstance(x, C): | ||
| pass | ||
| else: | ||
| no_diagnostic_here |
Contributor
Author
There was a problem hiding this comment.
On main, we emit a diagnostic here, i.e. we do not detect this branch as unreachable. On this branch, we do detect this as unreachable.
Contributor
|
sharkdp
commented
Jul 23, 2025
Contributor
Author
There was a problem hiding this comment.
I added tests for match statements as well, but they don't work yet (see astral-sh/ty#99 (comment))
Contributor
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
possibly-unresolved-reference |
0 | 24 | 0 |
invalid-return-type |
0 | 4 | 0 |
unresolved-attribute |
0 | 3 | 0 |
invalid-assignment |
0 | 2 | 0 |
| Total | 0 | 33 | 0 |
sharkdp
changed the title
isinstance(…) callsisinstance(…) branches
AlexWaygood
approved these changes
Jul 23, 2025
Comment on lines
1258
to
1272
| if match (self, first_arg) { | ||
| (KnownFunction::IsInstance, Type::NominalInstance(_)) => is_instance(first_arg), | ||
| // We do not handle unions specifically here, because something like `A | SubclassOfA` would | ||
| // have been simplified to `A` anyway | ||
| (KnownFunction::IsInstance, Type::Intersection(intersection)) => { | ||
| intersection.positive(db).iter().any(is_instance) | ||
| } | ||
| (KnownFunction::IsInstance, ty) => ty | ||
| .literal_fallback_instance(db) | ||
| .as_ref() | ||
| .is_some_and(is_instance), | ||
| _ => false, | ||
| } { | ||
| overload.set_return_type(Type::BooleanLiteral(true)); | ||
| } |
Member
There was a problem hiding this comment.
we could possibly also set the return type to Literal[False] if ClassType::could_coexist_in_mro_with() returns false for the two classes (indicating that it would be impossible to create a class that subclasses both classes simultaneously):
ruff/crates/ty_python_semantic/src/types/class.rs
Lines 492 to 497 in b605c3e
doesn't have to be done in this PR, though
Contributor
Author
There was a problem hiding this comment.
Ah, interesting! I'm somehow assuming that negative answers will not yield a lot of benefit, but I might be wrong. Will evaluate this separately.
| report_bad_argument_to_get_protocol_members(context, call_expression, *class); | ||
| } | ||
|
|
||
| KnownFunction::IsInstance | KnownFunction::IsSubclass => { |
Member
There was a problem hiding this comment.
it might make sense to apply similar handling for issubclass() too, just for symmetry if for nothing else. But again, that doesn't have to be done in this PR
Contributor
Author
There was a problem hiding this comment.
I did sort of a 80:20 thing here. We could do many sophisticated things, but I'm not sure if it's worth it? Might be quick to try out though, will evaluate separately.
AlexWaygood
reviewed
Jul 23, 2025
Comment on lines
+968
to
+969
| // We could probably try to infer more precise types in some of these cases, but it's unclear | ||
| // if it's worth the effort. |
Member
There was a problem hiding this comment.
I do think the fact that class-literals are all instances of type comes up fairly often, so I think that would be worth reflecting. (But the same is not true for generic aliases, and thus we also can't apply the same logic to SubclassOf types either.)
Contributor
Author
There was a problem hiding this comment.
Didn't yield any new ecosystem results, but doesn't hurt 😄 — thanks.
Member
There was a problem hiding this comment.
For the record -- I experimented in #19507 with what it would look like to do a more exhaustive match here. No ecosystem hits at all, so I think you were right to choose a simpler route here :-)
UnboundVariable
pushed a commit
to UnboundVariable/ruff
that referenced
this pull request
Jul 23, 2025
* main: (28 commits) [ty] highlight the argument in `static_assert` error messages (astral-sh#19426) [ty] Infer single-valuedness for enums based on `int`/`str` (astral-sh#19510) [ty] Restructure submodule query around `File` dependency [ty] Make `Module` a Salsa ingredient [ty] Reachability analysis for `isinstance(…)` branches (astral-sh#19503) [ty] Normalize single-member enums to their instance type (astral-sh#19502) [ty] Invert `ty_ide` and `ty_project` dependency (astral-sh#19501) [ty] Implement mock language server for testing (astral-sh#19391) [ty] Detect enums if metaclass is a subtype of EnumType/EnumMeta (astral-sh#19481) [ty] perform type narrowing for places marked `global` too (astral-sh#19381) [ty] Use `ThinVec` for sub segments in `PlaceExpr` (astral-sh#19470) [ty] Splat variadic arguments into parameter list (astral-sh#18996) [`flake8-pyi`] Skip fix if all `Union` members are `None` (`PYI016`) (astral-sh#19416) Skip notebook with errors in ecosystem check (astral-sh#19491) [ty] Consistent use of American english (in rules) (astral-sh#19488) [ty] Support iterating over enums (astral-sh#19486) Fix panic for illegal `Literal[…]` annotations with inner subscript expressions (astral-sh#19489) Move fix suggestion to subdiagnostic (astral-sh#19464) [ty] Implement non-stdlib stub mapping for classes and functions (astral-sh#19471) [ty] Disallow illegal uses of `ClassVar` (astral-sh#19483) ... # Conflicts: # crates/ty_ide/src/goto.rs
dcreager
added a commit
that referenced
this pull request
Jul 23, 2025
* main: [ty] Fix narrowing and reachability of class patterns with arguments (#19512) [ty] Implemented partial support for "find references" language server feature. (#19475) [`flake8-use-pathlib`] Add autofix for `PTH101`, `PTH104`, `PTH105`, `PTH121` (#19404) [`perflint`] Parenthesize generator expressions (`PERF401`) (#19325) [`pep8-naming`] Fix `N802` false positives for `CGIHTTPRequestHandler` and `SimpleHTTPRequestHandler` (#19432) [`pylint`] Handle empty comments after line continuation (`PLR2044`) (#19405) Move concise diagnostic rendering to `ruff_db` (#19398) [ty] highlight the argument in `static_assert` error messages (#19426) [ty] Infer single-valuedness for enums based on `int`/`str` (#19510) [ty] Restructure submodule query around `File` dependency [ty] Make `Module` a Salsa ingredient [ty] Reachability analysis for `isinstance(…)` branches (#19503) [ty] Normalize single-member enums to their instance type (#19502) [ty] Invert `ty_ide` and `ty_project` dependency (#19501) [ty] Implement mock language server for testing (#19391) [ty] Detect enums if metaclass is a subtype of EnumType/EnumMeta (#19481) [ty] perform type narrowing for places marked `global` too (#19381)
AlexWaygood
pushed a commit
that referenced
this pull request
Jul 25, 2025
## Summary
Add more precise type inference for a limited set of `isinstance(…)`
calls, i.e. return `Literal[True]` if we can be sure that this is the
correct result. This improves exhaustiveness checking / reachability
analysis for if-elif-else chains with `isinstance` checks. For example:
```py
def is_number(x: int | str) -> bool: # no "can implicitly return `None` error here anymore
if isinstance(x, int):
return True
elif isinstance(x, str):
return False
# code here is now detected as being unreachable
```
This PR also adds a new test suite for exhaustiveness checking.
## Test Plan
New Markdown tests
### Ecosystem analysis
The removed diagnostics look good. There's [one
case](https://github.com/pytorch/vision/blob/f52c4f1afd7dec25cbe7b98bcf1cbc840298e8da/torchvision/io/video_reader.py#L125-L143)
where a "true positive" is removed in unreachable code. `src` is
annotated as being of type `str`, but there is an `elif isinstance(src,
bytes)` branch, which we now detect as unreachable. And so the
diagnostic inside that branch is silenced. I don't think this is a
problem, especially once we have a "graying out" feature, or a lint that
warns about unreachable code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Add more precise type inference for a limited set of
isinstance(…)calls, i.e. returnLiteral[True]if we can be sure that this is the correct result. This improves exhaustiveness checking / reachability analysis for if-elif-else chains withisinstancechecks. For example:This PR also adds a new test suite for exhaustiveness checking.
Test Plan
New Markdown tests
Ecosystem analysis
The removed diagnostics look good. There's one case where a "true positive" is removed in unreachable code.
srcis annotated as being of typestr, but there is anelif isinstance(src, bytes)branch, which we now detect as unreachable. And so the diagnostic inside that branch is silenced. I don't think this is a problem, especially once we have a "graying out" feature, or a lint that warns about unreachable code.