Do not freeze an unresolved type variable default into an instance - #21873
Open
luantaraschi wants to merge 1 commit into
Open
Do not freeze an unresolved type variable default into an instance#21873luantaraschi wants to merge 1 commit into
luantaraschi wants to merge 1 commit into
Conversation
Contributor
|
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.
Fixes #21741.
Bisected to 47ca4c2 (#21491). Before it, a class whose type variable default held a placeholder was marked incomplete and its body was not analysed; now it defers and carries on, so a leftover placeholder reaches the method signatures.
Where the placeholder comes from
fix_instance()fills a missing type argument fromtv.defaultwithout checking whether that default is ready:During a pass where the default is still a forward reference,
process_typevar_parameters()has it asPlaceholderType(None, ...), and that is what gets substituted. In the reproducer,ParamMeasT = ParameterBase | Callable[[], None]names the bare generic, so the alias target becomesParameterBase[<placeholder None>].That freezes. A placeholder at the top of an alias target marks the alias incomplete, but a nested one deliberately does not, so that recursive aliases like
A = Sequence[str | A]stay legal:So the alias is committed with the placeholder inside it, no further pass is asked for, and nothing ever resolves it:
visit_placeholder_type()gives up on a placeholder with no fullname. From there it travels into theTypedDictitem that names the alias, and then into the signature of the method that returns that TypedDict, whereanalyze_func_def()seeshas_placeholder(result)on the final iteration and callsdefer().The reported
Cannot resolve TypedDict item (possible cyclic definition)is the same placeholder arriving one step earlier. It is not a real cycle in the user's code.The change
fix_instance()takes the analyzer API, and when the default it is about to substitute still has a placeholder it either asks for another pass or, on the final iteration, falls back toAny. The second half matters as much as the first:defer()documents that it must not be called once there is no pass left, and this is the one place that knows the default will never arrive.Output for the reproducer now matches 2.1.0 exactly, both the crash and the spurious TypedDict error being gone:
The third caller of
fix_instance(), the one insemanal.pythat repairs an alias target, is left as it was: it has no test pulling on it here and I did not want to widen the change without one.Tests
testTypeVarDefaultPlaceholderInImportCycleTypedDictincheck-typevar-defaults.test, a four-module cycle reduced from the reporter's repository. On master it is anINTERNAL ERROR. It also pins the answer rather than just the absence of a crash: the default resolves toc.Instand the TypedDict item comes out astuple[a.Base[c.Inst] | (def ()), ...].Ran locally on Windows:
testcheck8226 passed, plustestsemanal,testtransform,testdeps,testmerge,testfinegrainedandtesttypegen, 1944 passed. Self check clean.