Fix temp vars referenced in parameter #38130
Conversation
|
@DanielRosenwasser I'd like some feedback on the change to the error message. I'm considering simplifying it to the following: |
|
/cc @weswigham, @andrewbranch for review as suggested reviewers isn't working. |
|
I think that for initializer, the old message or or both work. I think that in the case of the binding pattern, you should special case the message to be a variation of that so that we don't confuse people with "or binding pattern" |
|
That message is somewhat redundant, isn't it? The reason I like |
|
My suggestion would also be similar to the other existing related message: |
|
Regarding "cannot reference itself", the reason I might change that one as well is this: function f({ [a]: a }) {}Binding |
|
Looking at the errors produced by parameterInitializersForwardReferencing.2.ts , this is a breaking change, isn’t it? This currently compiles in 3.8.3 (and seems to work in every runtime I’ve tried), but looks like it’s an error in this PR? |
|
Are you trying them in ES2015 or ES5? |
|
Oh sorry. The error is introduced only when using syntax that introduces temporaries. Your example should continue to work fine, but if you do |
|
Ah ok, interesting. So this PR does issue an error where there wasn’t one before, but only in cases where runtime behavior was already broken because of a name/scope conflict introduced by downleveling. Is that right? |
|
Correct. Its the same error we introduce for the same reason when the target is ES5, there are just now more cases that are accurately covered now. |
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 45eca94. You can monitor the build here. Update: The results are in! |
|
I do think if I happened to hit this error, I would have a really hard time understanding why TypeScript thinks that |
|
Right now I'm erring on the side of emulating the same behavior as what happens with the ES5 target. We might want to consider improving that messaging across the board, but I don't have an idea of a concise message for that either. |
|
Maybe these?
|
|
Hmm. The third bullet point is a bit harder to determine while in the middle of |
|
@rbuckton Here they are:Comparison Report - master..38130
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@DanielRosenwasser should I merge this and discuss changing the messages later, or should I wait? |
|
The messages seem fine overall, I'd just merge. |
|
(If you feel like they could be better, I encourage you to follow up, but don't let it block merging this) |
As of ES2015 it is illegal for the evaluation of one parameter to reference another parameter or a variable declared in the body of the function. As a result, any time we introduce a temporary variable when we emit a parameter with an initializer or binding pattern when targeting ES2015 or later we must move the evaluation to the body of the function.
This also adds a heuristic to the checker to ensure the correct name resolution for cases where our emit moves the evaluation of the parameter to the body of the function, and thus report an error when a parameter refers to an identifier that is shadowed in the body of the function.
Fixes #36295