Add Support for Using Aliased Discriminants in Conditional Statements #56173
Conversation
Zzzen
force-pushed
the
aliased-discriminants
branch
from
2d7c54b
to
c914ebf
Compare
src/compiler/checker.ts
Outdated
| case SyntaxKind.ObjectBindingPattern: | ||
| case SyntaxKind.ArrayBindingPattern: | ||
| return isVariableDeclaration(node.parent) && isVarConstLike(node.parent); |
There was a problem hiding this comment.
shouldn't this lead to calling isConstantReference recursively (perhaps accompanied by some other checks but still)? What about nested binding patterns (const { a: { b: b2 } } = ...)?
There was a problem hiding this comment.
Thank you for bringing this to my attention! I was surprised to discover that TypeScript currently lacks support for nested binding patterns during cross symbol analysis. I don't know if it is intended though.
type Nested = {
type: 'string';
resp: {
data: string
}
} | {
type: 'number';
resp: {
data: number;
}
}
{
let resp!: Nested;
const { resp: { data }, type } = resp;
if (type === 'string') {
data satisfies string; // should not error
}
if (resp.type === 'string') {
resp.resp.data satisfies string;
}
}There was a problem hiding this comment.
This sounds like #38839 (comment)
I was more thinking about cases like this:
type Nested =
| {
resp: {
type: "string";
data: string;
};
}
| {
resp: {
type: "number";
data: number;
};
};
declare const nested: Nested;
const {
resp: { type, data },
} = nested;
if (type === "string") {
data satisfies string; // should be OK
}This keeps the dependency between the discriminant and another property "at the same level" but at the same time both are coming from a nested binding pattern.
There was a problem hiding this comment.
I think that I might have accidentally fixed this in this PR. When I sat down to it I totally forgot about the conversation here 😅
Zzzen
force-pushed
the
aliased-discriminants
branch
from
a6be0ab
to
34aed6e
Compare
| @@ -27160,6 +27160,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| case SyntaxKind.ElementAccessExpression: | |||
| // The resolvedSymbol property is initialized by checkPropertyAccess or checkElementAccess before we get here. | |||
| return isConstantReference((node as AccessExpression).expression) && isReadonlySymbol(getNodeLinks(node).resolvedSymbol || unknownSymbol); | |||
| case SyntaxKind.ObjectBindingPattern: | |||
| case SyntaxKind.ArrayBindingPattern: | |||
| return isBindingElement(node.parent) ? isConstantReference(node.parent.parent) : isVariableDeclaration(node.parent) && isVarConstLike(node.parent); | |||
There was a problem hiding this comment.
an alternative solution for this could look smth like this:
| return isBindingElement(node.parent) ? isConstantReference(node.parent.parent) : isVariableDeclaration(node.parent) && isVarConstLike(node.parent); | |
| const rootDeclaration = getRootDeclaration(node.parent); | |
| return isVariableDeclaration(rootDeclaration) && isVarConstLike(rootDeclaration); |
There was a problem hiding this comment.
I believe that this should work as well: TS playground but the problem is that we are dealing with a pseudo-reference here. At this point, we can't easily check if the original symbol for which we are performing the narrowing is a non-reassigned parameter.
This could easily be seen as a separate issue though. I only mention it here since I was touching the nearby code and I noticed this now.
There was a problem hiding this comment.
Wish there was a 'const' modifier available for parameters and we could just check that!
There was a problem hiding this comment.
This should make that possible: #56313
This comment was marked as duplicate.
This comment was marked as duplicate.
|
@typescript-bot run DT |
|
Heya @gabritto, I've started to run the parallelized Definitely Typed test suite on this PR at 9ff723c. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based top-repos suite on this PR at 9ff723c. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the regular perf test suite on this PR at 9ff723c. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based user code test suite on this PR at 9ff723c. You can monitor the build here. Update: The results are in! |
|
@gabritto Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@gabritto Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @gabritto, the results of running the DT tests are ready. |
|
@gabritto Here are the results of running the top-repos suite comparing Everything looks good! |
Fixes #55577