Conversation
Contributor
|
Contributor
Author
|
Primer results suggest there's something necessary missing in the new version of our TYPE_CHECKING support here. |
Contributor
|
The diff from |
Member
|
it appears as though we're no longer viewing the remainder of the module as unreachable code (in which most diagnostics should be suppressed) following this |
Member
|
#21449 looks like it got rid of the same set of false positives without adding any of the new false positives. Is this PR still needed, do you think? |
Contributor
Author
|
No, I think we can close this. There are some remaining inconsistencies in our support for |
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
3 participants
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
#19372 added support for detecting whether a function is defined inside an
if TYPE_CHECKINGblock, and if so, allowing it to be a "stub" function (empty body, even with non-None return type).This PR improves that support by also supporting it for
if typing.TYPE_CHECKINGas well asif TYPE_CHECKING. In order to do so, it changes our basic approach toTYPE_CHECKINGsupport. Rather than inferring a type ofLiteral[True]for all symbols namedTYPE_CHECKING, instead we specially handleifconditions with purely syntactic matching (on anyNamematchingTYPE_CHECKINGor any attribute expression with attributeTYPE_CHECKING). This means we no longer recognize aliasing ofTYPE_CHECKING(e.g.from typing import TYPE_CHECKING as TC), and we'll recognizefoo.TYPE_CHECKINGno matter whatfoois. (In both cases this matches the behavior of all other existing type checkers.) Arguably it's better to not support aliasingTYPE_CHECKING, given its unusual behavior of matching any variable namedTYPE_CHECKING.We currently don't handle conditions like
if TYPE_CHECKING or get_bool()(other type checkers do). If this proves to be relevant, we can add it.I think it would also be possible to remove the extra boolean flag on scopes and in SemanticIndexBuilder, and rely instead on reachability constraints (which we already track on scopes) for detecting when a scope is defined inside
if TYPE_CHECKING. This would require adding new terminal reachability constraints forTYPE_CHECKINGand probablyNOT_TYPE_CHECKINGas well, which would behave likeALWAYS_TRUEandALWAYS_FALSE, but would also allow detecting that a reachability constraint is dependent onTYPE_CHECKING. I didn't choose to do that here, but it's a possible future improvement.Another alternative to this entire PR would be to instead double down on our handling of
TYPE_CHECKINGin type inference rather than syntactically, add a newSpecialFormfor "the type ofTYPE_CHECKING", and then we could evaluate reachability constraints under "considerTYPE_CHECKINGFalse" conditions in order to detect code that is only reachable due toTYPE_CHECKING.Test Plan
mdtests