Better error message for unparenthesized function/constructor type notation in union/intersection types #39570
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DanielRosenwasser
approved these changes
Jul 13, 2020
src/compiler/parser.ts
Outdated
Comment on lines
3573
to
3574
| functionTypeDiagnostic: DiagnosticMessage, | ||
| constructorTypeDiagnostic: DiagnosticMessage |
There was a problem hiding this comment.
Instead of passing along the diagnostics, I might just pass along the information of whether or not this is a union type or an intersection type.
There was a problem hiding this comment.
@DanielRosenwasser Thank you for your reviews!
I updated this function so it now receives a single parameter isInUnionType: boolean.
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at 50ab58c. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 50ab58c. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 50ab58c. You can monitor the build here. Update: The results are in! |
|
Hey @DanielRosenwasser, 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. |
|
@DanielRosenwasser Here they are:Comparison Report - master..39570
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>
sandersn
requested changes
Jul 14, 2020
src/compiler/parser.ts
Outdated
| // the function type and constructor type shorthand notation | ||
| // are not allowed directly in unions and intersections, but we'll | ||
| // try to parse them gracefully and issue a helpful message. | ||
| if (isStartOfFunctionType() || token() === SyntaxKind.NewKeyword) { |
There was a problem hiding this comment.
is there an already-defined isStartOfConstructorType? I'll go check...
There was a problem hiding this comment.
nope, but the only other call also is followed by || token() === SntaxKind.NewKeyword. Can you move that clause into the function?
| diagnostic = Diagnostics.Constructor_type_notation_must_be_parenthesized_when_used_in_an_intersection_type; | ||
| } | ||
| } | ||
| parseErrorAtRange(type, diagnostic); |
There was a problem hiding this comment.
side note: I bet this newish function could replace a lot of other lower-level error-reporting functions in the parser; it's only used 3 other places right now.
|
Hmm, parse time is frequently just baaarely significantly slower, and never significantly faster, so I'd call this a noticeable but very small slowdown. @DanielRosenwasser do you think this is still worth taking? I'd like to see a round or two of optimisation personally. |
Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>
|
@sandersn Thank you for your reviews! I applied all your suggestions including refactoring |
|
@typescript-bot perf test this |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 32562ff. You can monitor the build here. Update: The results are in! |
src/compiler/parser.ts
Outdated
| if (token() === operator || hasLeadingOperator) { | ||
| const types = [type]; | ||
| while (parseOptional(operator)) { | ||
| types.push(parseConstituentType()); | ||
| types.push(parseFunctionOrConstructorTypeToError(operator === SyntaxKind.BarToken) || parseConstituentType()); |
There was a problem hiding this comment.
You can hoist operator === SyntaxKind.BarToken to isUnionType and share that at both locations.
|
@DanielRosenwasser Here they are:Comparison Report - master..39570
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>
|
@typescript-bot perf test this |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at e4c84fc. You can monitor the build here. Update: The results are in! |
|
@DanielRosenwasser Here they are:Comparison Report - master..39570
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
These perf results seem weirdly suspicious. If the next one doesn't seem to have those same jumps, I think it's safe to call it a fluke. @typescript-bot perf test this |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at e4c84fc. You can monitor the build here. Update: The results are in! |
|
@DanielRosenwasser Here they are:Comparison Report - master..39570
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
sandersn
approved these changes
Jul 15, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #39548.
With this PR, we parse unparenthesized function types in union/intersection types anyway, and emit a specialized error message for them.
Example
Current Behavior (TS 4.0 beta)
New Behavior