Dedup tparam-default substitution between alt and solver layers - #4406
Open
nitishagar wants to merge 1 commit into
Open
Dedup tparam-default substitution between alt and solver layers#4406nitishagar wants to merge 1 commit into
nitishagar wants to merge 1 commit into
Conversation
The logic for substituting references to earlier type parameters inside a tparam's default was duplicated in two places: `get_tparam_default` in the alt/class layer (used during call/specialization) and an inlined copy in `finish_class_targs` in the solver (used during class-level finalization). The two copies differed only in their lookup source and their fallback value for out-of-scope references — differences that are intentional, since one path runs where an out-of-scope reference is an already-reported error and the other runs during finalization with no such error. Extract a shared `substitute_tparam_defaults` helper into `pyrefly_types::quantified` (next to the existing `as_gradual_type_helper`, which already uses the same TypeVar/TypeVarTuple/ParamSpec/Quantified match idiom). The helper takes the default by value, a `&dyn Fn(&Name) -> Option<Type>` lookup closure, and a caller-chosen fallback `Type`, so each call site keeps its own lookup source and fallback semantics. Both callers are rewritten to delegate to it; the `// TODO: deal with code duplication in get_tparam_default` marker is removed. This is behavior-preserving: each call site passes the same lookup and fallback it used inline, so the produced types are identical for every input. The full `cargo test -p pyrefly --lib` suite (7462 tests) passes unchanged. Fixes facebook#4298
Contributor
|
This pull request has been imported. If you are a Meta employee, you can view this in D114523923. (Because this pull request was imported automatically, there will not be any future comments.) |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
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
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
The logic for substituting references to earlier type parameters inside a tparam's
defaultwas duplicated in two places:get_tparam_defaultinpyrefly/lib/alt/class/targs.rs(used during call/specialization)finish_class_targsinpyrefly/lib/solver/solver.rs(used during class-level finalization), flagged by a// TODO: deal with code duplication in get_tparam_defaultmarker.The two copies differed only in their lookup source and their fallback value for out-of-scope references — differences that are intentional, since one path runs where an out-of-scope reference is an already-reported error (returns
Any(Implicit)) and the other runs during finalization with no such error (returns the param's gradual type).This PR extracts a shared
substitute_tparam_defaultshelper intopyrefly_types::quantified(next to the existingas_gradual_type_helper, which already uses the sameTypeVar/TypeVarTuple/ParamSpec/Quantifiedmatch idiom). The helper takes the default by value, a&dyn Fn(&Name) -> Option<Type>lookup closure, and a caller-chosen fallbackType, so each call site keeps its own lookup source and fallback semantics. Both callers are rewritten to delegate to it, and the// TODOmarker is removed.Behavior
This is a behavior-preserving refactor. Each call site passes the same lookup and fallback it used inline, so the produced types are identical for every input:
get_tparam_defaultpassesname_to_idx->checked_targslookup withself.heap.mk_any_implicit()fallback (unchanged).finish_class_targspassesseen_params->new_targs-then-targslookup withparam.as_gradual_type()fallback (unchanged).The two fallbacks are deliberately not collapsed — they intentionally differ (error path vs finalization path).
Test plan
cargo build -p pyrefly_types— cleancargo build -p pyrefly— cleancargo test -p pyrefly --lib— 7462 passed, 0 failed, 3 ignored (the full typechecker regression suite; zero behavior diff)python3 test.py --no-test --no-tensor-shapes --no-conformance --no-jsonschema— clean for the touched filesFixes #4298
AI-generated: this PR was prepared by an AI agent (ZCode) following a reviewed implementation plan. The code has been read, built, and tested as described above.