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

Jump to bottom

Do not parse template arguments in JavaScript files. #36673

Merged
merged 1 commit into from
Nov 19, 2020
Merged

Do not parse template arguments in JavaScript files. #36673

merged 1 commit into from
Nov 19, 2020

Conversation

mprobst
Copy link
Contributor

@mprobst mprobst commented Feb 7, 2020

Fixes #36662.

This is somewhat preliminary. The main drawback of the change is that users will get substantially worse error messages if they accidentally include <Type, Arguments> in JavaScript files (see the associated errors.txt changes).

To mitigate, we could:

  1. keep parsing type arguments in JSX element contexts (<Element<Type, Arg> x=1>). I don't think this is ambiguous with any JS syntax.
  2. attempt to special case and pass down a flag to produce a better error message for code like fnCall<Type, Argument>()

(2) could be tricky to get right enough to really improve developer's life, so I think whether it's worth doing depends on how likely we think the scenario is in the first place.

@fatcerberus
Copy link

fatcerberus commented Feb 9, 2020 •
edited
Loading

users will get substantially worse error messages if they accidentally include <Type, Arguments> in JavaScript files

Worse, they may get no error message if they accidentally include a single <TypeArgument> (particularly in the case that the type argument has representation in value space, e.g. a class) because that will now be parsed as a less-than operator followed by greater-than operator. Unfortunately there's no heuristic that would be able to tell the difference with 100% accuracy 😦

@mprobst
Copy link
Contributor Author

mprobst commented Feb 12, 2020

@fatcerberus ack, but that's fundamentally because in JS, foo<TypeArgument>(bar) has a defined and standardized meaning as comparison operations. I think it's ultimately less confusing to be correct according to the spec here rather than being incorrect with better error messages.

@sandersn sandersn added the For Backlog Bug PRs that fix a backlog bug label Feb 14, 2020
@sandersn sandersn assigned sandersn and andrewbranch and unassigned sandersn Mar 12, 2020
@andrewbranch andrewbranch added the Breaking Change Would introduce errors in existing code label Apr 1, 2020
@andrewbranch
Copy link
Member

Tentatively filing this as a breaking change even though that’s sort of debatable. Most of the time this is going to be swapping one error for another; very occasionally it will be removing an undesired error (which is the point of the PR, #36662), but as @fatcerberus pointed out, there could be a possible scenario where you were getting a desired error (type arguments can’t be used in JS) before, and aren’t now.

I’d like to dig into the specifics of whether that could actually ever happen without getting a new error in the vein of “Operator '<' cannot be applied to types...” I guess if enough of the variables in play are declared as any, that would do the trick.

@andrewbranch
Copy link
Member

keep parsing type arguments in JSX element contexts

Also FWIW, I do think this would be an easy win if it’s not ambiguous.

@DanielRosenwasser
Copy link
Member

For checkJs, you'll get a bad error - but we can potentially do some gymnastics to figure it out. The real break is for API consumers.

@andrewbranch
Copy link
Member

andrewbranch commented Apr 1, 2020 •
edited
Loading

For checkJs, you'll get a bad error

But not when a bunch of stuff is any 😕. This would become error-free: https://www.typescriptlang.org/play?noImplicitAny=false&useJavaScript=true#code/MYGwhgzhAEAqCeAHApgQQE4HMCuBbZAdgC7QDeAvgFCUBm2BwRAlgPYHQ0AUNLLANNABGYdAEoylaFOjpkRbOnY8WAHgQoMOfMQB8nYWIDclKkA

@DanielRosenwasser
Copy link
Member

Well shame on you for having anys!

@DanielRosenwasser
Copy link
Member

Jokes aside, I think I'd be surprised to see a user write explicit type arguments in a JS file that's unchecked. TypeScript users already see explicit type arguments as a bit of a code smell.

@sandersn
Copy link
Member

sandersn commented Apr 2, 2020

One possible exception is .js files that are flow-checked. I'm not sure whether there are more of those than of .js files with syntax that is currently parsing as type arguments.

@DanielRosenwasser
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 3, 2020 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at f308ac1. You can monitor the build here.

@DanielRosenwasser
Copy link
Member

Just want to make sure we still give good errors on

  • type parameters function declarations and expressions
  • type parameters arrow functions
  • and type references

Maybe @bterlson can help find examples that are valid JS we also currently have issues with

@DanielRosenwasser DanielRosenwasser mentioned this pull request Jul 9, 2020
Closed
@DanielRosenwasser
Copy link
Member

@typescript-bot pack this
@typescript-bot test this
@typescript-bot user test this
@typescript-bot run dt
@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 10, 2020 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at f308ac1. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 10, 2020 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at f308ac1. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 10, 2020 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at f308ac1. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 10, 2020 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at f308ac1. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 10, 2020 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at f308ac1. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

@DanielRosenwasser
The results of the perf run you requested are in!

Here they are:

Comparison Report - master..36673

Metric master 36673 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 343,962k (± 0.01%) 343,423k (± 0.01%) -540k (- 0.16%) 343,333k 343,543k
Parse Time 2.00s (± 0.50%) 2.02s (± 0.70%) +0.01s (+ 0.60%) 2.00s 2.06s
Bind Time 0.81s (± 0.64%) 0.82s (± 0.54%) +0.01s (+ 0.86%) 0.81s 0.83s
Check Time 4.73s (± 0.29%) 4.74s (± 0.80%) +0.01s (+ 0.23%) 4.64s 4.79s
Emit Time 5.19s (± 0.59%) 5.21s (± 1.48%) +0.02s (+ 0.42%) 5.12s 5.51s
Total Time 12.73s (± 0.25%) 12.78s (± 0.87%) +0.05s (+ 0.40%) 12.65s 13.19s
Monaco - node (v10.16.3, x64)
Memory used 339,143k (± 0.03%) 339,267k (± 0.03%) +124k (+ 0.04%) 339,064k 339,415k
Parse Time 1.58s (± 0.55%) 1.58s (± 0.92%) +0.00s (+ 0.32%) 1.55s 1.61s
Bind Time 0.71s (± 0.78%) 0.72s (± 0.51%) +0.00s (+ 0.56%) 0.71s 0.72s
Check Time 4.91s (± 0.64%) 4.89s (± 0.64%) -0.01s (- 0.29%) 4.83s 4.95s
Emit Time 2.74s (± 0.35%) 2.74s (± 0.83%) -0.00s (- 0.11%) 2.69s 2.80s
Total Time 9.95s (± 0.32%) 9.94s (± 0.49%) -0.01s (- 0.08%) 9.80s 10.02s
TFS - node (v10.16.3, x64)
Memory used 302,064k (± 0.02%) 302,062k (± 0.02%) -2k (- 0.00%) 301,968k 302,186k
Parse Time 1.20s (± 0.68%) 1.22s (± 0.67%) +0.01s (+ 1.16%) 1.20s 1.24s
Bind Time 0.66s (± 1.03%) 0.67s (± 1.02%) +0.00s (+ 0.15%) 0.65s 0.68s
Check Time 4.39s (± 0.39%) 4.42s (± 0.82%) +0.03s (+ 0.78%) 4.35s 4.49s
Emit Time 2.89s (± 0.92%) 2.88s (± 0.69%) -0.01s (- 0.48%) 2.82s 2.91s
Total Time 9.15s (± 0.40%) 9.18s (± 0.37%) +0.03s (+ 0.34%) 9.12s 9.26s
material-ui - node (v10.16.3, x64)
Memory used 459,445k (± 0.01%) 459,143k (± 0.01%) -303k (- 0.07%) 459,034k 459,262k
Parse Time 2.04s (± 0.29%) 2.04s (± 0.40%) -0.00s (- 0.05%) 2.02s 2.06s
Bind Time 0.65s (± 1.88%) 0.65s (± 1.57%) 0.00s ( 0.00%) 0.63s 0.67s
Check Time 12.93s (± 0.92%) 12.85s (± 0.78%) -0.07s (- 0.57%) 12.72s 13.21s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.62s (± 0.75%) 15.55s (± 0.67%) -0.07s (- 0.46%) 15.39s 15.92s
Angular - node (v12.1.0, x64)
Memory used 320,857k (± 0.13%) 320,761k (± 0.02%) -96k (- 0.03%) 320,601k 320,882k
Parse Time 1.98s (± 0.60%) 1.99s (± 0.56%) +0.01s (+ 0.71%) 1.97s 2.02s
Bind Time 0.80s (± 1.34%) 0.80s (± 0.42%) +0.00s (+ 0.25%) 0.80s 0.81s
Check Time 4.63s (± 0.83%) 4.66s (± 0.56%) +0.03s (+ 0.69%) 4.62s 4.74s
Emit Time 5.37s (± 1.29%) 5.36s (± 0.64%) -0.01s (- 0.19%) 5.31s 5.44s
Total Time 12.78s (± 0.75%) 12.82s (± 0.22%) +0.03s (+ 0.26%) 12.77s 12.89s
Monaco - node (v12.1.0, x64)
Memory used 321,581k (± 0.02%) 321,575k (± 0.03%) -6k (- 0.00%) 321,394k 321,817k
Parse Time 1.54s (± 0.99%) 1.56s (± 0.48%) +0.02s (+ 1.10%) 1.54s 1.57s
Bind Time 0.69s (± 1.40%) 0.69s (± 0.99%) +0.01s (+ 0.87%) 0.69s 0.72s
Check Time 4.65s (± 1.27%) 4.68s (± 0.37%) +0.03s (+ 0.62%) 4.64s 4.73s
Emit Time 2.78s (± 1.48%) 2.79s (± 0.42%) +0.02s (+ 0.61%) 2.77s 2.82s
Total Time 9.66s (± 1.21%) 9.73s (± 0.20%) +0.07s (+ 0.67%) 9.68s 9.76s
TFS - node (v12.1.0, x64)
Memory used 286,569k (± 0.01%) 286,574k (± 0.03%) +5k (+ 0.00%) 286,459k 286,772k
Parse Time 1.23s (± 0.74%) 1.24s (± 0.66%) +0.01s (+ 0.49%) 1.22s 1.26s
Bind Time 0.64s (± 0.81%) 0.64s (± 0.90%) +0.00s (+ 0.63%) 0.63s 0.65s
Check Time 4.30s (± 0.59%) 4.32s (± 0.58%) +0.02s (+ 0.44%) 4.26s 4.38s
Emit Time 2.92s (± 0.80%) 2.94s (± 1.25%) +0.02s (+ 0.82%) 2.89s 3.05s
Total Time 9.09s (± 0.46%) 9.14s (± 0.38%) +0.05s (+ 0.57%) 9.07s 9.22s
material-ui - node (v12.1.0, x64)
Memory used 437,822k (± 0.01%) 437,380k (± 0.07%) -441k (- 0.10%) 436,236k 437,679k
Parse Time 2.04s (± 0.88%) 2.04s (± 0.29%) -0.00s (- 0.10%) 2.02s 2.05s
Bind Time 0.64s (± 0.94%) 0.63s (± 0.82%) -0.00s (- 0.63%) 0.62s 0.64s
Check Time 11.65s (± 0.72%) 11.61s (± 0.79%) -0.04s (- 0.31%) 11.43s 11.85s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 14.32s (± 0.55%) 14.28s (± 0.66%) -0.04s (- 0.29%) 14.08s 14.52s
Angular - node (v8.9.0, x64)
Memory used 340,444k (± 0.01%) 340,091k (± 0.01%) -352k (- 0.10%) 339,972k 340,191k
Parse Time 2.53s (± 0.27%) 2.53s (± 0.37%) +0.00s (+ 0.04%) 2.51s 2.54s
Bind Time 0.85s (± 0.58%) 0.85s (± 0.70%) -0.01s (- 0.94%) 0.83s 0.86s
Check Time 5.35s (± 0.46%) 5.36s (± 0.52%) +0.01s (+ 0.09%) 5.30s 5.42s
Emit Time 5.90s (± 1.08%) 5.96s (± 1.13%) +0.06s (+ 0.97%) 5.87s 6.15s
Total Time 14.64s (± 0.51%) 14.69s (± 0.46%) +0.05s (+ 0.37%) 14.57s 14.84s
Monaco - node (v8.9.0, x64)
Memory used 340,492k (± 0.01%) 340,497k (± 0.01%) +5k (+ 0.00%) 340,379k 340,609k
Parse Time 1.87s (± 0.43%) 1.87s (± 0.40%) +0.00s (+ 0.11%) 1.86s 1.89s
Bind Time 0.88s (± 0.25%) 0.88s (± 0.77%) +0.00s (+ 0.34%) 0.87s 0.90s
Check Time 5.40s (± 0.62%) 5.41s (± 0.43%) +0.01s (+ 0.19%) 5.36s 5.45s
Emit Time 3.22s (± 0.37%) 3.23s (± 0.56%) +0.01s (+ 0.25%) 3.19s 3.27s
Total Time 11.37s (± 0.36%) 11.40s (± 0.37%) +0.03s (+ 0.23%) 11.33s 11.47s
TFS - node (v8.9.0, x64)
Memory used 303,833k (± 0.02%) 303,847k (± 0.02%) +15k (+ 0.00%) 303,752k 303,972k
Parse Time 1.54s (± 0.48%) 1.54s (± 0.53%) -0.00s (- 0.00%) 1.53s 1.56s
Bind Time 0.67s (± 0.77%) 0.67s (± 0.71%) -0.00s (- 0.60%) 0.66s 0.68s
Check Time 5.03s (± 1.38%) 4.94s (± 0.95%) -0.09s (- 1.79%) 4.85s 5.09s
Emit Time 3.03s (± 2.64%) 3.13s (± 1.85%) +0.10s (+ 3.14%) 2.91s 3.22s
Total Time 10.27s (± 0.35%) 10.28s (± 0.39%) +0.00s (+ 0.03%) 10.21s 10.39s
material-ui - node (v8.9.0, x64)
Memory used 463,640k (± 0.01%) 463,426k (± 0.01%) -214k (- 0.05%) 463,334k 463,499k
Parse Time 2.41s (± 0.34%) 2.40s (± 0.70%) -0.02s (- 0.75%) 2.36s 2.43s
Bind Time 0.78s (± 0.85%) 0.77s (± 0.96%) -0.01s (- 1.66%) 0.76s 0.79s
Check Time 17.19s (± 1.33%) 17.11s (± 1.13%) -0.08s (- 0.46%) 16.59s 17.49s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 20.39s (± 1.12%) 20.28s (± 1.02%) -0.11s (- 0.53%) 19.73s 20.71s
Angular - node (v8.9.0, x86)
Memory used 195,323k (± 0.03%) 195,070k (± 0.01%) -253k (- 0.13%) 195,004k 195,116k
Parse Time 2.45s (± 0.82%) 2.47s (± 1.17%) +0.02s (+ 0.82%) 2.42s 2.56s
Bind Time 0.99s (± 0.62%) 0.99s (± 0.66%) -0.00s (- 0.10%) 0.97s 1.00s
Check Time 4.83s (± 0.84%) 4.87s (± 1.02%) +0.03s (+ 0.68%) 4.79s 5.01s
Emit Time 5.95s (± 1.07%) 5.93s (± 1.13%) -0.02s (- 0.27%) 5.77s 6.08s
Total Time 14.22s (± 0.67%) 14.26s (± 0.82%) +0.04s (+ 0.26%) 14.06s 14.62s
Monaco - node (v8.9.0, x86)
Memory used 193,511k (± 0.02%) 193,501k (± 0.03%) -10k (- 0.01%) 193,355k 193,640k
Parse Time 1.92s (± 0.99%) 1.91s (± 0.46%) -0.02s (- 0.88%) 1.88s 1.92s
Bind Time 0.70s (± 0.43%) 0.70s (± 1.43%) +0.01s (+ 0.86%) 0.69s 0.74s
Check Time 5.50s (± 0.48%) 5.49s (± 0.47%) -0.01s (- 0.18%) 5.43s 5.55s
Emit Time 2.68s (± 0.65%) 2.68s (± 0.96%) +0.00s (+ 0.11%) 2.63s 2.74s
Total Time 10.80s (± 0.38%) 10.78s (± 0.40%) -0.02s (- 0.17%) 10.66s 10.86s
TFS - node (v8.9.0, x86)
Memory used 173,771k (± 0.01%) 173,763k (± 0.02%) -8k (- 0.00%) 173,678k 173,874k
Parse Time 1.58s (± 0.67%) 1.61s (± 1.15%) +0.03s (+ 2.03%) 1.57s 1.65s
Bind Time 0.64s (± 1.01%) 0.65s (± 1.05%) +0.01s (+ 1.25%) 0.64s 0.67s
Check Time 4.68s (± 0.79%) 4.69s (± 0.55%) +0.01s (+ 0.15%) 4.63s 4.75s
Emit Time 2.80s (± 1.33%) 2.82s (± 1.10%) +0.02s (+ 0.54%) 2.73s 2.87s
Total Time 9.70s (± 0.56%) 9.76s (± 0.52%) +0.06s (+ 0.60%) 9.63s 9.89s
material-ui - node (v8.9.0, x86)
Memory used 262,544k (± 0.02%) 262,408k (± 0.02%) -136k (- 0.05%) 262,299k 262,499k
Parse Time 2.45s (± 0.47%) 2.47s (± 0.78%) +0.03s (+ 1.06%) 2.44s 2.53s
Bind Time 0.67s (± 1.91%) 0.67s (± 1.26%) +0.00s (+ 0.45%) 0.66s 0.69s
Check Time 15.67s (± 0.51%) 15.66s (± 0.74%) -0.01s (- 0.06%) 15.45s 15.91s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 18.79s (± 0.40%) 18.80s (± 0.57%) +0.02s (+ 0.09%) 18.60s 19.02s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-166-generic
Architecturex64
Available Memory16 GB
Available Memory1 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v8.9.0, x64)
  • node (v8.9.0, x86)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v8.9.0, x64)
  • Angular - node (v8.9.0, x86)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v8.9.0, x64)
  • Monaco - node (v8.9.0, x86)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v8.9.0, x64)
  • TFS - node (v8.9.0, x86)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v8.9.0, x64)
  • material-ui - node (v8.9.0, x86)
Benchmark Name Iterations
Current 36673 10
Baseline master 10

@DanielRosenwasser DanielRosenwasser added the API Relates to the public API for TypeScript label Nov 19, 2020
andrewbranch
andrewbranch approved these changes Nov 19, 2020
View reviewed changes
Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Thanks for your patience, everyone. I think we should take this 🌟

@DanielRosenwasser
Copy link
Member

Marking this with the API label just because if you were ever using TS to parse Flow-y code, you're now going to have issues.

@andrewbranch andrewbranch merged commit 6b04f50 into microsoft:master Nov 19, 2020
@andrewbranch andrewbranch mentioned this pull request Nov 19, 2020
Merged
@mprobst mprobst deleted the parse-js branch June 15, 2021 10:08
@microsoft microsoft deleted a comment from 1883Milewski Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewbranch andrewbranch andrewbranch approved these changes

Assignees

@andrewbranch andrewbranch

Labels
API Relates to the public API for TypeScript Breaking Change Would introduce errors in existing code For Backlog Bug PRs that fix a backlog bug
Projects
PR Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JS value being parsed as a TS type argument
6 participants
@mprobst @fatcerberus @andrewbranch @DanielRosenwasser @sandersn @typescript-bot