Skip to content

Add a regression test for discriminating property access in parenthesized expression #51778

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.

Sign up for GitHub

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

Closed
Andarist wants to merge 2 commits into from
Closed

Add a regression test for discriminating property access in parenthesized expression #51778

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3329725
Add a regression test for discriminating property access in parenthes…
Andarist Dec 6, 2022
1478f88
Merge remote-tracking branch 'origin/main' into regression-test/extra…
Andarist Feb 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions tests/baselines/reference/narrowingTypeofUndefined.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,15 @@ if (typeof a.error !== 'undefined') {
else {
a.result.prop; // number
}

// extra parens on purpose
if (typeof (a.error) === 'undefined') {
a.result.prop; // number
}


//// [narrowingTypeofUndefined.js]
"use strict";
if (typeof a.error === 'undefined') {
a.result.prop; // number
}
Expand All @@ -29,3 +35,7 @@ if (typeof a.error !== 'undefined') {
else {
a.result.prop; // number
}
// extra parens on purpose
if (typeof (a.error) === 'undefined') {
a.result.prop; // number
}
30 changes: 22 additions & 8 deletions tests/baselines/reference/narrowingTypeofUndefined.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,17 @@ if (typeof a.error === 'undefined') {

a.result.prop; // number
>a.result.prop : Symbol(prop, Decl(narrowingTypeofUndefined.ts, 0, 95))
>a.result : Symbol(result, Decl(narrowingTypeofUndefined.ts, 0, 43), Decl(narrowingTypeofUndefined.ts, 0, 85))
>a.result : Symbol(result, Decl(narrowingTypeofUndefined.ts, 0, 85))
>a : Symbol(a, Decl(narrowingTypeofUndefined.ts, 0, 13))
>result : Symbol(result, Decl(narrowingTypeofUndefined.ts, 0, 43), Decl(narrowingTypeofUndefined.ts, 0, 85))
>result : Symbol(result, Decl(narrowingTypeofUndefined.ts, 0, 85))
>prop : Symbol(prop, Decl(narrowingTypeofUndefined.ts, 0, 95))
}
else {
a.error.prop; // string
>a.error.prop : Symbol(prop, Decl(narrowingTypeofUndefined.ts, 0, 27))
>a.error : Symbol(error, Decl(narrowingTypeofUndefined.ts, 0, 18), Decl(narrowingTypeofUndefined.ts, 0, 67))
>a.error : Symbol(error, Decl(narrowingTypeofUndefined.ts, 0, 18))
>a : Symbol(a, Decl(narrowingTypeofUndefined.ts, 0, 13))
>error : Symbol(error, Decl(narrowingTypeofUndefined.ts, 0, 18), Decl(narrowingTypeofUndefined.ts, 0, 67))
>error : Symbol(error, Decl(narrowingTypeofUndefined.ts, 0, 18))
>prop : Symbol(prop, Decl(narrowingTypeofUndefined.ts, 0, 27))
}

Expand All @@ -36,17 +36,31 @@ if (typeof a.error !== 'undefined') {

a.error.prop; // string
>a.error.prop : Symbol(prop, Decl(narrowingTypeofUndefined.ts, 0, 27))
>a.error : Symbol(error, Decl(narrowingTypeofUndefined.ts, 0, 18), Decl(narrowingTypeofUndefined.ts, 0, 67))
>a.error : Symbol(error, Decl(narrowingTypeofUndefined.ts, 0, 18))
>a : Symbol(a, Decl(narrowingTypeofUndefined.ts, 0, 13))
>error : Symbol(error, Decl(narrowingTypeofUndefined.ts, 0, 18), Decl(narrowingTypeofUndefined.ts, 0, 67))
>error : Symbol(error, Decl(narrowingTypeofUndefined.ts, 0, 18))
>prop : Symbol(prop, Decl(narrowingTypeofUndefined.ts, 0, 27))
}
else {
a.result.prop; // number
>a.result.prop : Symbol(prop, Decl(narrowingTypeofUndefined.ts, 0, 95))
>a.result : Symbol(result, Decl(narrowingTypeofUndefined.ts, 0, 43), Decl(narrowingTypeofUndefined.ts, 0, 85))
>a.result : Symbol(result, Decl(narrowingTypeofUndefined.ts, 0, 85))
>a : Symbol(a, Decl(narrowingTypeofUndefined.ts, 0, 13))
>result : Symbol(result, Decl(narrowingTypeofUndefined.ts, 0, 43), Decl(narrowingTypeofUndefined.ts, 0, 85))
>result : Symbol(result, Decl(narrowingTypeofUndefined.ts, 0, 85))
>prop : Symbol(prop, Decl(narrowingTypeofUndefined.ts, 0, 95))
}

// extra parens on purpose
if (typeof (a.error) === 'undefined') {
>a.error : Symbol(error, Decl(narrowingTypeofUndefined.ts, 0, 18), Decl(narrowingTypeofUndefined.ts, 0, 67))
>a : Symbol(a, Decl(narrowingTypeofUndefined.ts, 0, 13))
>error : Symbol(error, Decl(narrowingTypeofUndefined.ts, 0, 18), Decl(narrowingTypeofUndefined.ts, 0, 67))

a.result.prop; // number
>a.result.prop : Symbol(prop, Decl(narrowingTypeofUndefined.ts, 0, 95))
>a.result : Symbol(result, Decl(narrowingTypeofUndefined.ts, 0, 85))
>a : Symbol(a, Decl(narrowingTypeofUndefined.ts, 0, 13))
>result : Symbol(result, Decl(narrowingTypeofUndefined.ts, 0, 85))
>prop : Symbol(prop, Decl(narrowingTypeofUndefined.ts, 0, 95))
}

32 changes: 25 additions & 7 deletions tests/baselines/reference/narrowingTypeofUndefined.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,47 +11,65 @@ declare const a: { error: { prop: string }, result: undefined } | { error: undef
if (typeof a.error === 'undefined') {
>typeof a.error === 'undefined' : boolean
>typeof a.error : "string" | "number" | "bigint" | "boolean" | "symbol" | "undefined" | "object" | "function"
>a.error : { prop: string; }
>a.error : { prop: string; } | undefined
>a : { error: { prop: string; }; result: undefined; } | { error: undefined; result: { prop: number; }; }
>error : { prop: string; }
>error : { prop: string; } | undefined
>'undefined' : "undefined"

a.result.prop; // number
>a.result.prop : number
>a.result : { prop: number; }
>a : { error: { prop: string; }; result: undefined; } | { error: undefined; result: { prop: number; }; }
>a : { error: undefined; result: { prop: number; }; }
>result : { prop: number; }
>prop : number
}
else {
a.error.prop; // string
>a.error.prop : string
>a.error : { prop: string; }
>a : { error: { prop: string; }; result: undefined; } | { error: undefined; result: { prop: number; }; }
>a : { error: { prop: string; }; result: undefined; }
>error : { prop: string; }
>prop : string
}

if (typeof a.error !== 'undefined') {
>typeof a.error !== 'undefined' : boolean
>typeof a.error : "string" | "number" | "bigint" | "boolean" | "symbol" | "undefined" | "object" | "function"
>a.error : { prop: string; }
>a.error : { prop: string; } | undefined
>a : { error: { prop: string; }; result: undefined; } | { error: undefined; result: { prop: number; }; }
>error : { prop: string; }
>error : { prop: string; } | undefined
>'undefined' : "undefined"

a.error.prop; // string
>a.error.prop : string
>a.error : { prop: string; }
>a : { error: { prop: string; }; result: undefined; } | { error: undefined; result: { prop: number; }; }
>a : { error: { prop: string; }; result: undefined; }
>error : { prop: string; }
>prop : string
}
else {
a.result.prop; // number
>a.result.prop : number
>a.result : { prop: number; }
>a : { error: undefined; result: { prop: number; }; }
>result : { prop: number; }
>prop : number
}

// extra parens on purpose
if (typeof (a.error) === 'undefined') {
>typeof (a.error) === 'undefined' : boolean
>typeof (a.error) : "string" | "number" | "bigint" | "boolean" | "symbol" | "undefined" | "object" | "function"
>(a.error) : { prop: string; } | undefined
>a.error : { prop: string; } | undefined
>a : { error: { prop: string; }; result: undefined; } | { error: undefined; result: { prop: number; }; }
>error : { prop: string; } | undefined
>'undefined' : "undefined"

a.result.prop; // number
>a.result.prop : number
>a.result : { prop: number; }
>a : { error: undefined; result: { prop: number; }; }
>result : { prop: number; }
>prop : number
}
Expand Down
7 changes: 7 additions & 0 deletions tests/cases/compiler/narrowingTypeofUndefined.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// @strict: true

declare const a: { error: { prop: string }, result: undefined } | { error: undefined, result: { prop: number } }

if (typeof a.error === 'undefined') {
Expand All @@ -13,3 +15,8 @@ if (typeof a.error !== 'undefined') {
else {
a.result.prop; // number
}

// extra parens on purpose
if (typeof (a.error) === 'undefined') {
a.result.prop; // number
}
Comment on lines +19 to +
Copy link
Copy Markdown
Contributor Author

@Andarist Andarist Dec 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This was reporting an error before this PR: #51720

The issue was that typeOfExpr.expression was passed to getDiscriminantPropertyAccess here:
https://github.com/microsoft/TypeScript/pull/50344/files#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R25331

and it got fixed by @ahejlsberg since he changed this and now target is passed there:
https://github.com/microsoft/TypeScript/pull/51720/files#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R26520

This works since target is the result of getReferenceCandidate:
https://github.com/microsoft/TypeScript/pull/50344/files#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R25329
and that includes "skip parenthesis"-like logic:
https://github.dev/microsoft/TypeScript/blob/c124d0e2604bac1a428c24b98a1dd6508d860812/src/compiler/checker.ts#L25182-L25185

Copy link
Copy Markdown
Member

@ahejlsberg ahejlsberg Dec 6, 2022

Choose a reason for hiding this comment

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

Yup, that was an intended fix. I'm not sure we need this extra test since #51270 already includes a regression test from #51700 with two typeof checks that differ only in parentheses. It was in fact that difference that caused me to fix the issue.

Copy link
Copy Markdown
Contributor Author

@Andarist Andarist Dec 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

#51720 has a test with optional chaining and parenthesis - that has worked before your PR despite the fact that the version without parenthesis reported an error. This test is kinda about the opposite situation (no optional chaining and parenthesis) - the version without parenthesis worked alright while the version with them reported an error.

I've concluded that they are slightly different and thus I've created a PR (I already had it locally as I was looking into fixing #51700 when you picked it up). I understand that they can be seen as similar enough though. Feel free to merge or close - whatever you think is best here.