[ty] perform type narrowing for places marked global too
#19381
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
oconnor663
commented
Jul 16, 2025
| # of narrowing. | ||
| global x | ||
| if x == 1: | ||
| y: int = x # allowed, because x cannot be None in this branch |
Contributor
Author
There was a problem hiding this comment.
This fails on main, as per astral-sh/ty#311 (comment).
Contributor
|
Contributor
Author
|
Quite a lot of removed diagnostics in |
Contributor
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
possibly-unbound-attribute |
0 | 61 | 1 |
invalid-return-type |
0 | 39 | 0 |
unsupported-operator |
0 | 11 | 0 |
invalid-argument-type |
0 | 4 | 0 |
non-subscriptable |
0 | 4 | 0 |
call-non-callable |
0 | 3 | 0 |
not-iterable |
0 | 3 | 0 |
invalid-assignment |
0 | 1 | 1 |
invalid-context-manager |
0 | 1 | 0 |
unused-ignore-comment |
1 | 0 | 0 |
| Total | 1 | 127 | 2 |
Collaborator
If a place that is not |
Member
|
@sharkdp is this a PR you could review? |
sharkdp
approved these changes
Jul 21, 2025
Contributor
sharkdp
left a comment
There was a problem hiding this comment.
Thank you, this looks great. @oconnor663 I'll leave it up to you if you want to include the suggested refactoring around narrow_place_with_applicable_constraints here, or not.
carljm
approved these changes
Jul 22, 2025
This fixes a bug reported at: astral-sh/ty#311 (comment)
809e662 to
fbdcc1c
Compare
CodSpeed Instrumentation Performance ReportMerging #19381 will not alter performanceComparing Summary
|
CodSpeed WallTime Performance ReportMerging #19381 will not alter performanceComparing Summary
|
… in `infer_place_load`" This reverts commit fbdcc1c.
Contributor
Author
|
The 14-18% performance regression in |
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
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.
This fixes a bug reported at:
astral-sh/ty#311 (comment)
I don't understand the narrowing machinery very well yet, so I don't have much intuition for whether this is a Good Change, other than that it doesn't seem to break anything. We call
narrow_place_with_applicable_constraintsfive separate times (previously four) in this one function, and that seems a little fishy. When would we not want to do it?