Skip to content

support QualifiedName when narrowing inside loops #43592

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

Merged
merged 6 commits into from
May 21, 2021

Conversation

Zzzen
Copy link
Contributor

@Zzzen Zzzen commented Apr 8, 2021

Fixes #43411

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Apr 8, 2021
if (foo.a.b) {
type B = typeof foo.a.b;
~~~~~
!!! error TS2532: Object is possibly 'undefined'.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this is a bug. Should it be fixed in this PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm pretty sure that possibly-undefined errors also use control flow, so this is a sign that something else needs to be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

                case SyntaxKind.QualifiedName:
-                   if (currentFlow && parent.kind === SyntaxKind.TypeQuery) {
+                   if (currentFlow && isPartOfTypeQuery(node)) {
                        node.flowNode = currentFlow;
                    }

binding children of QualifiedName fixed it.

https://github.com/microsoft/TypeScript/pull/43592/files#diff-9ad14a2e2e66533cd0c3f02ecf01f7e6bc9d60cdf2e125df2a14ce0713222898R2521

case SyntaxKind.QualifiedName:
const left = getFlowCacheKey((<QualifiedName>node).left, declaredType, initialType, flowContainer);
const right = (<QualifiedName>node).right.escapedText;
return `${left}.${right}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use the same structure as the property/element access code below. Specifically, getFlowCacheKey can return undefined.

The way that isMatchingReference looks is probably the right way to copy the code, but it might also be possible to unify the handling of all three node kinds into one code path by finding/making more functions like getAccessedPropertyName.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 I think I gonna just copy from isMatchingReference because the structure of QualifiedName is kinda different from ElementAccessExpression

if (foo.a.b) {
type B = typeof foo.a.b;
~~~~~
!!! error TS2532: Object is possibly 'undefined'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm pretty sure that possibly-undefined errors also use control flow, so this is a sign that something else needs to be fixed.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one request to make the test less confusing.

properties.foo; // type is { aaa: string; bbb: string; }
for (const x of [1, 2, 3]) {
properties.foo; // type is { aaa: string; bbb: string; }
type FooOrUndefined = typeof properties.foo; //type is { aaa: string; bbb: string; } | undefined
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove this comment or change it to be "type should be { aaa: string, bbb: string }", now that the bug is fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌🏻done

@sandersn sandersn merged commit 756392c into microsoft:master May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

typeof operator not using undefined guard in when used in a loop nested in the guard
4 participants