Better error message for unparenthesized function/constructor type notation in union/intersection types#39570
Merged
DanielRosenwasser merged 10 commits intomicrosoft:masterfrom Jul 15, 2020
Merged
Conversation
DanielRosenwasser
approved these changes
Jul 13, 2020
Member
There was a problem hiding this comment.
I think this is the right approach. While I'd prefer passing a single piece of information for "is this a union or intersection", I'd be okay with weaving the diagnostics through. I'd like to get input from others though in case there's anything I'm missing.
src/compiler/parser.ts
Outdated
Comment on lines
+3573
to
+3574
| functionTypeDiagnostic: DiagnosticMessage, | ||
| constructorTypeDiagnostic: DiagnosticMessage |
Member
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.
Contributor
Author
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.
Member
|
@typescript-bot pack this |
Collaborator
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at 50ab58c. You can monitor the build here. |
Collaborator
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 50ab58c. You can monitor the build here. |
Collaborator
|
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! |
Collaborator
|
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. |
Collaborator
|
@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
Member
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
Looks good, just some minor style changes, and I want to make sure that performance is not hurt by the extra check, since isStartOfFunctionType does lookahead upon seeing a (.
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) { |
Member
There was a problem hiding this comment.
is there an already-defined isStartOfConstructorType? I'll go check...
Member
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); |
Member
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.
Member
|
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>
Contributor
Author
|
@sandersn Thank you for your reviews! I applied all your suggestions including refactoring |
Member
|
@typescript-bot perf test this |
Collaborator
|
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
| const types = [type]; | ||
| while (parseOptional(operator)) { | ||
| types.push(parseConstituentType()); | ||
| types.push(parseFunctionOrConstructorTypeToError(operator === SyntaxKind.BarToken) || parseConstituentType()); |
Member
There was a problem hiding this comment.
You can hoist operator === SyntaxKind.BarToken to isUnionType and share that at both locations.
Collaborator
|
@DanielRosenwasser Here they are:Comparison Report - master..39570
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>
Member
|
@typescript-bot perf test this |
Collaborator
|
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! |
Collaborator
|
@DanielRosenwasser Here they are:Comparison Report - master..39570
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Member
|
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 |
Collaborator
|
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! |
Collaborator
|
@DanielRosenwasser Here they are:Comparison Report - master..39570
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
sandersn
approved these changes
Jul 15, 2020
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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