Drop variable statements when the declaration list gets transformed to an empty list#59893
Conversation
| var _a = exports.cilBlurLinear[0]; | ||
| var _b = exports.cilBlurLinear[0]; | ||
| var _c = exports.cilBlurLinear[0]; |
There was a problem hiding this comment.
we can see here that const [[]] = ... isn't as "optimized" as const [,] = .... I think it's fine-ish. All this code assumes downlevelIteration: false so it's OK to drop those element access expressions but it's also kinda OK to keep them. It would be cool to use a consistent logic here but that's a separate issue in my eyes
There was a problem hiding this comment.
I'd prefer we didn't just entirely elide the destructuring given proxies/getters - but I'll defer to others. @RyanCavanaugh @rbuckton
| var ; | ||
| let; | ||
| var ; |
There was a problem hiding this comment.
All those inputs are incorrect in the first place. So I'm not quite bothered that they produce different kind of an output. For an invalid input it's OK to emit an invalid output. It just happened that fixing a case for a valid input changed this here - but I think that's fine too. In a similar vein, I think it's OK to drop invalid statements from the invalid input.
There was a problem hiding this comment.
nit on the file name - there are no parameters in this file
There was a problem hiding this comment.
ah, good catch - that was a bad reuse of one of the other test names 😅
| //// [tests/cases/conformance/es6/destructuring/emptyArrayBindingPatternParameter05.ts] //// | ||
|
|
||
| //// [emptyArrayBindingPatternParameter05.ts] | ||
| export const cilBlurLinear: string[][] = [[]]; |
There was a problem hiding this comment.
It's a little hard to tell whats happening in the output of this test. Maybe introduce 3 different variables so it's clearer where and how they're being destructured.
| var _a = exports.cilBlurLinear[0]; | ||
| var _b = exports.cilBlurLinear[0]; | ||
| var _c = exports.cilBlurLinear[0]; |
There was a problem hiding this comment.
I'd prefer we didn't just entirely elide the destructuring given proxies/getters - but I'll defer to others. @RyanCavanaugh @rbuckton
| const | ||
|
|
||
| //// [downlevelLetConst1.js] | ||
| var ; |
There was a problem hiding this comment.
Spooky given this test is named in such a way that implies that it's testing exactly this transformation?
There was a problem hiding this comment.
see https://github.com/microsoft/TypeScript/pull/59893/files#r1749158145 , i think the same principle would apply here
this particular test case is testing const, the next one in line is testing const a and the next one is const a = 1. So they just exercise different kinds of inputs and now the question is if it's ok to drop the variable declaration that was invalid in the first place
…output-for-empty-destructuring-patterns
Andarist
force-pushed
the
fix/empty-var-in-es5-output-for-empty-destructuring-patterns
branch
from
d309ebb to
9a99293
Compare
4 participants
fixes #59877