Skip to content
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

fix(44466): Fixes parsing contextual keyword casts as arrow functions #49029

Merged
merged 3 commits into from
May 10, 2022
Merged

fix(44466): Fixes parsing contextual keyword casts as arrow functions #49029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3dd9ef4
fix(44466): Fixes parsing contextual keyword casts as arrow functions
DavidSouther May 9, 2022
958adfc
Use all modifiers test
DavidSouther May 9, 2022
e9d39bd
Remove spurious lookAhead, as this fn is already in a lookAhead
DavidSouther May 9, 2022
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
4 changes: 4 additions & 0 deletions src/compiler/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4538,6 +4538,10 @@ namespace ts {
// isn't actually allowed, but we want to treat it as a lambda so we can provide
// a good error message.
if (isModifierKind(second) && second !== SyntaxKind.AsyncKeyword && lookAhead(nextTokenIsIdentifier)) {
Copy link
Member

Choose a reason for hiding this comment

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

The fact that nextTokenIsIdentifier checks for async and yield itself makes me feel like there should be some sort of context for as, rather than special casing, but like you said on #44466 (comment), I'm not entirely certain that there's another expression that can behave like this.

This whole "does this look like an arrow function" is effectively a heuristic and continues to bite us (see: #16241), so I think it's likely ok to disable this heuristic here like you've done.

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 the other option is reworking this along the lines Daniel suggested earlier in #44466, but looking for a matched close paren followed by an arrow? But doing that leaves a lot of lookahead for errors to accumulate, and you're not guaranteed to find the matching paren, much less the arrow itself.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that this fix is good as-is; this block (as commented above) is to do better error recovery if someone writes code like (foo number) => ..., which is guaranteed to fail to parse later. The likelihood of that broken code appearing and someone legitimately naming a type as seems very, very low, so I'm happy with just not having recovery in this case.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose though, that one could write a test that is (foo as) => 1234 or something, and see if you should actually return TriState.Unknown here instead.

Or, that this should be a lookAhead in the if statement before the code you've added (the "isn't actually allowed" one), to better fit with the heuristic.

Copy link
Member

Choose a reason for hiding this comment

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

I have tested both, and they both pass the tests in the PR, though they all differ on a test like:

let out: any = undefined;
type as = any;

export const e = (declare as) => 1234;

With the two lookahead versions, you get an emitted exports.e = declare;, but with Unknown, it actually tries to parse that broken code as an arrow function again and complains about the modifier, and emits:

var e = function (as) { return 1234; };
exports.e = e;

Maybe that's better in the error case, preserving the recovery?

@DanielRosenwasser curious as to what you think; I'm not convinced it matters either way if both fix the bug, just an esoteric recovery.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is an okay fix; however, if we add satisfies (or any other new infix keyword) then we'll have another similar bug in the future.

if (nextToken() === SyntaxKind.AsKeyword) {
// https://github.com/microsoft/TypeScript/issues/44466
return Tristate.False;
}
return Tristate.True;
}

Expand Down
23 changes: 23 additions & 0 deletions tests/baselines/reference/modifierParenCast.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
//// [modifierParenCast.ts]
let readonly: any = undefined;
let override: any = undefined;
let out: any = undefined;
let declare: any = undefined;

export const a = (readonly as number);
export const b = (override as number);
export const c = (out as number);
export const d = (declare as number);

//// [modifierParenCast.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.d = exports.c = exports.b = exports.a = void 0;
var readonly = undefined;
var override = undefined;
var out = undefined;
var declare = undefined;
exports.a = readonly;
exports.b = override;
exports.c = out;
exports.d = declare;
33 changes: 33 additions & 0 deletions tests/baselines/reference/modifierParenCast.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
=== tests/cases/compiler/modifierParenCast.ts ===
let readonly: any = undefined;
>readonly : Symbol(readonly, Decl(modifierParenCast.ts, 0, 3))
>undefined : Symbol(undefined)

let override: any = undefined;
>override : Symbol(override, Decl(modifierParenCast.ts, 1, 3))
>undefined : Symbol(undefined)

let out: any = undefined;
>out : Symbol(out, Decl(modifierParenCast.ts, 2, 3))
>undefined : Symbol(undefined)

let declare: any = undefined;
>declare : Symbol(declare, Decl(modifierParenCast.ts, 3, 3))
>undefined : Symbol(undefined)

export const a = (readonly as number);
>a : Symbol(a, Decl(modifierParenCast.ts, 5, 12))
>readonly : Symbol(readonly, Decl(modifierParenCast.ts, 0, 3))

export const b = (override as number);
>b : Symbol(b, Decl(modifierParenCast.ts, 6, 12))
>override : Symbol(override, Decl(modifierParenCast.ts, 1, 3))

export const c = (out as number);
>c : Symbol(c, Decl(modifierParenCast.ts, 7, 12))
>out : Symbol(out, Decl(modifierParenCast.ts, 2, 3))

export const d = (declare as number);
>d : Symbol(d, Decl(modifierParenCast.ts, 8, 12))
>declare : Symbol(declare, Decl(modifierParenCast.ts, 3, 3))

41 changes: 41 additions & 0 deletions tests/baselines/reference/modifierParenCast.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
=== tests/cases/compiler/modifierParenCast.ts ===
let readonly: any = undefined;
>readonly : any
>undefined : undefined

let override: any = undefined;
>override : any
>undefined : undefined

let out: any = undefined;
>out : any
>undefined : undefined

let declare: any = undefined;
>declare : any
>undefined : undefined

export const a = (readonly as number);
>a : number
>(readonly as number) : number
>readonly as number : number
>readonly : any

export const b = (override as number);
>b : number
>(override as number) : number
>override as number : number
>override : any

export const c = (out as number);
>c : number
>(out as number) : number
>out as number : number
>out : any

export const d = (declare as number);
>d : number
>(declare as number) : number
>declare as number : number
>declare : any

11 changes: 11 additions & 0 deletions tests/cases/compiler/modifierParenCast.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
//@target: ES5

let readonly: any = undefined;
let override: any = undefined;
let out: any = undefined;
let declare: any = undefined;

export const a = (readonly as number);
export const b = (override as number);
export const c = (out as number);
export const d = (declare as number);