-
Notifications
You must be signed in to change notification settings - Fork 2.4k
[ty] Reuse name resolution for builtin autofix checks #28037
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
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
What about when the
itemsis in an inner class definition given that class variables are evaluated first?This currently suggests to replace it with
list[...]but it raises aNameErroron 3.13 but not from 3.14 due to deferred annotations.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 tried fixing this, but it quickly started spiralling in complexity, and codex kept finding more and more issues with each review pass. I ended up asking codex to reconsider this approach in its entirety and here's its conclusion (I gave it a leading question, so I probably biased it, but still):
I recommend abandoning this implementation and retaining the conservative helper.
Sharing name-resolution logic was a reasonable starting point, but the branch has exposed two different requirements:
I don’t think robust reuse is impossible. It would need a clearly separate contract: reuse whatever lookup information is available, but reject cases whose safety depends on inference assumptions or missing execution-order information. Ordinary inference should remain unchanged. However, building and maintaining that distinction would need a worthwhile benefit.
Here, the gains are mostly additional fixes involving aliases, unreachable shadowing, and similar cases. Given that missing those fixes is acceptable, I don’t see enough benefit to justify the additional machinery and coupling. The existing helper’s simple rule—reject visible shadowing and project-level builtin overrides—is much easier to reason about.
I would retain any useful regression tests that pass with the conservative implementation and drop the implementation changes. I haven’t changed the branch.
So I think it is far from trivial to simply "reuse our existing machinery" for name lookup when it comes to autofixes, unfortunately, @MichaReiser. The problem is that our existing machinery wasn't designed for autofix safety in mind, and would have to be significantly redesigned to capture a lot more information if we wanted to reuse it for that purpose. I don't think putting that much effort in is justified at this stage.
The status quo on
mainis fine for now: we offer autofixes in the common case, and we refrain from offering them in edge cases where we can't be confident that we'd give a good fix.