Prevent valid JSX from being seen as the start of a generic arrow function, fix crashes #52450
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
Consider me no longer suspicious; it's lowercase so it's an intrinsic. |
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at c705d8c. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at c705d8c. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at c705d8c. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at c705d8c. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at c705d8c. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at c705d8c. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at c705d8c. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at c705d8c. You can monitor the build here. |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
1 similar comment
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
The recent esbuild commit regarding generic arrow functions in TSX files is related: evanw/esbuild@8824778 In that to this PR you can just add: else if (third === SyntaxKind.GreaterThanToken) {
return Tristate.Unknown;
}However, there's sort of a mutual exclusivity between the relevant piece of code: let fn = async <T> () => {};And this test: var x1 = <T>() => {}</T>;
x1.isElement;I'm personally convinced both should parse (and so esbuild doesn't have to change), but I'm not totally sure how to get there given our current goofy arrow function parsing method. |
typescript-bot
added
For Milestone Bug
|
@jakebailey Here they are:
CompilerComparison Report - main..52450
System
Hosts
Scenarios
TSServerComparison Report - main..52450
System
Hosts
Scenarios
StartupComparison Report - main..52450
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
Thanks for looking at this! I'm worried that this PR potentially makes these already-complicated rules too much more complicated. Previously you could definitively tell whether something is an arrow function in JSX or not by examining the next few tokens. I thought that property was kind of nice. But now it depends on a lot more code and arbitrary look-ahead, which adds many more additional ambiguities to resolve. For example, this PR currently means the following: Having to distinguish between Anyway, I thought I'd put a plug in for keeping the look-ahead rules for this edge case simple like they were before, instead of (as far as I understand) now requiring unbounded look-ahead and relying on the full details of TypeScript's arrow function parser (including error-recovery for invalid syntax). It probably doesn't matter too much what's done here IMO since this is an edge case as long as the internal compiler error is fixed. But I was expecting something like the addition of a rule for the |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
I can understand that; having fixed numerous bugs in this same place it's getting a little frustrating. I'll try and see if there's a more straightforward way here, but I'm having a little bit of a hard time thinking of how to do it. |
|
Some quick thoughts (feel free to disregard). The internal compiler error could potentially be fixed like this: export function rangeOfTypeParameters(sourceFile: SourceFile, typeParameters: NodeArray<TypeParameterDeclaration>): TextRange {
// Include the `<>`
const pos = typeParameters.pos - 1;
- const end = skipTrivia(sourceFile.text, typeParameters.end) + 1;
+ const end = Math.min(sourceFile.text.length, skipTrivia(sourceFile.text, typeParameters.end) + 1);
return { pos, end };
}And if you wanted to add support for the const isArrowFunctionInJsx = lookAhead(() => {
parseOptional(SyntaxKind.ConstKeyword);
const third = nextToken();
if (third === SyntaxKind.ExtendsKeyword) {
const fourth = nextToken();
switch (fourth) {
case SyntaxKind.EqualsToken:
case SyntaxKind.GreaterThanToken:
+ case SyntaxKind.SlashToken:
return false;
default:
return true;
}
}
else if (third === SyntaxKind.CommaToken || third === SyntaxKind.EqualsToken) {
return true;
}
return false;
}); |
|
Oh, funny. I should have tried the latter (I later tried nearly the same thing to fix that other oddity you changed esbuild to match TypeScript on). I'll try it out in the morning. |
|
That suggestion worked great, thanks! I don't think we need to modify |
The crash can still occur in other cases such as |
|
Ah, you're totally right. I'll add that; we definitely don't want more crashes, even if the code doesn't parse. |
jakebailey
changed the title
|
I checked, and your suggested fix is again good; this probably fixes other potential crashes for users of this function in the LS. There's one other place which does a skipTrivia+1 but it's in a place where it's impossible for the file to have ended. |
This adjusts esbuild's behavior to match the changes to the TypeScript compiler that were introduced by microsoft/TypeScript#52450.
Fixes #52449
The arrow function lookahead was too overzealous in determining what was "definitely" an arrow function. Switching a "true" to an "unknown" result fixes the issue.
But, I am suspicious of this because there is no error saying that the variableconstisn't found (unlikeT).