-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Track non-null unknown types in control flow analysis #45575
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
Conversation
| return includes & TypeFlags.Any ? includes & TypeFlags.IncludesWildcard ? wildcardType : anyType : unknownType; | ||
| return includes & TypeFlags.Any ? | ||
| includes & TypeFlags.IncludesWildcard ? wildcardType : anyType : | ||
| includes & TypeFlags.Null || containsType(typeSet, unknownType) ? unknownType : nonNullUnknownType; |
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’s an example where this produces nonNullUnknownType? Would nonNullUnknownType itself have to be one of the union constituents?
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.
Yes, it'll produce nonNullUnknownType when there are no regular unknownType instances in the set (which implies all are nonNullUnknownType instances) and the set contains no TypeFlags.Null types. Effectively, a non-null unknown type survives only if there are no regular unknown types and no null types.
| assumeTrue ? TypeFacts.EQNull : TypeFacts.NENull : | ||
| assumeTrue ? TypeFacts.EQUndefined : TypeFacts.NEUndefined; | ||
| return getTypeWithFacts(type, facts); | ||
| return type.flags & TypeFlags.Unknown && facts & (TypeFacts.NENull | TypeFacts.NEUndefinedOrNull) ? nonNullUnknownType : getTypeWithFacts(type, facts); |
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.
Can this not just be handled by getTypeWithFacts?
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.
Well, getTypeWithFacts(…) is used in many more places, and nonNullUnknownType should never escape control flow analysis: https://github.com/microsoft/TypeScript/blob/7e231a2ebfce3692663fe2583a19a8cb0c59e457/src/compiler/checker.ts#L23118-L23119
So probably not?
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.
|
This reminded me of #29317! 😮 Does special-casing this mean the team is still undecided on Negated Types? |
|
In any case, thanks for this fix! I have this pop up every so often, and it'll be nice to see it finally solved. 😀 |
| const nonInferrableAnyType = createIntrinsicType(TypeFlags.Any, "any", ObjectFlags.ContainsWideningType); | ||
| const intrinsicMarkerType = createIntrinsicType(TypeFlags.Any, "intrinsic"); | ||
| const unknownType = createIntrinsicType(TypeFlags.Unknown, "unknown"); | ||
| const nonNullUnknownType = createIntrinsicType(TypeFlags.Unknown, "unknown"); |
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.
It's already pretty hard to understand what these are for. I'd encourage us to start documenting what the use-cases for each of these are. For example.
| const nonNullUnknownType = createIntrinsicType(TypeFlags.Unknown, "unknown"); | |
| /** | |
| * An `unknown` type that is used purely for narrowing. | |
| * Used to record that a `x !== null` check has occurred to specially handle `typeof x === "object"`. | |
| */ | |
| const nonNullUnknownType = createIntrinsicType(TypeFlags.Unknown, "unknown"); |
With this PR we fix the following long standing issue:
Above, the order of the checks mattered because
unknownis an "indivisible" non-union type and therefore a check forx !== nullcouldn't narrowxin the secondifstatement. We implemented a partial fix in #37507, but this only worked for truthy checks combined withtypeofchecks using the&&operator in the same expression.With this PR we remove the partial fix and instead introduce an internal
nonNullUnknownTypethat is used in control flow analysis to represent anunknowntype that is known to be non-null. The non-null unknown type results when theunknowntype is subjected to a truthiness check or anx !== nullcheck, and when the non-null unknown is subsequently subjected to atypeof x === 'object'check, we narrow toobjectinstead ofobject | null. Because we rely on control flow analysis, this solution properly reflects the effects of non-null checks in separate expressions or statements:Fixes #28131.