[ty] Reuse name resolution for builtin autofix checks - #28037
Conversation
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 97.69%. The percentage of expected errors that received a diagnostic held steady at 93.71%. The number of fully passing files held steady at 110/136. |
Memory usage reportMemory usage unchanged ✅ |
|
| Project | Old Time | New Time | Change |
|---|---|---|---|
dd-trace-py |
2.23s | 0.76s | -66% |
Flaky changes detected. This PR summary excludes flaky changes; see the HTML report for details.
5b2fbbb to
7e989b7
Compare
| .filter(|key| key.contains("infer_scope_types_impl")) | ||
| .collect::<Vec<_>>() | ||
| }); | ||
| assert_eq!(cycles, Vec::<String>::new()); |
There was a problem hiding this comment.
nit: assert!(cycles.is_empty()); ?
There was a problem hiding this comment.
Hmm, I think that would give a worse error message if the assertion failed, because it wouldn't print out the elements that actually appeared in the Vec?
| def check(): | ||
| items: [int] # snapshot: invalid-type-form | ||
| list = builtins.list |
There was a problem hiding this comment.
What about when the items is in an inner class definition given that class variables are evaluated first?
import builtins
def check():
class C:
items: [int]
list = builtins.listThis currently suggests to replace it with list[...] but it raises a NameError on 3.13 but not from 3.14 due to deferred annotations.
There was a problem hiding this comment.
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:
- Ordinary inference tolerates assumptions that autofix eligibility cannot. For example, the class-body compatibility rule deliberately promotes possibly-bound values to definitely bound. Reusing that result does not establish that introducing a name is safe.
- My last change crossed an inappropriate boundary. Changing shared closure snapshots to answer an autofix question affected ordinary inference and discarded useful information. That was a mistake. Insufficient evidence for an autofix should have meant withholding the fix, not changing the semantic index’s behavior.
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 main is 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.
|
Thanks for reviewing @dhruvmanila ❤️ |
Summary
Stacked on top of #28029, addressing the review discussion about reusing name resolution.
Use ty's existing name-resolution and binding-inference machinery to check whether an autofix can refer to a standard builtin class. This allows fixes to recognize builtin imports and aliases, ignore unreachable module assignments, and follow
global/nonlocaldeclarations. Ambiguous bindings and project-level replacement classes still suppress the fix.Extract
PlaceLoadSource::infer_typeto share source-type evaluation betweenTypeInferenceBuilderandSemanticModel, and generalize the existing resolution mode for expressions absent from the semantic index. The introduced name has no indexed use site, so the check considers all reachable bindings and conservatively treats bindings in the current scope as possibly unbound.Test Plan
Add mdtests for builtin aliases, scope declarations, unreachable assignments, ambiguous and unbound names, and project-level builtin overrides. Add a query regression asserting that producing these autofixes does not re-enter scope inference.