-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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.
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
Drop variable statements when the declaration list gets transformed to an empty list #59893
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit on the file name - there are no parameters in this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
var _a = exports.cilBlurLinear[0]; | ||
var _b = exports.cilBlurLinear[0]; | ||
var _c = exports.cilBlurLinear[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer we didn't just entirely elide the destructuring given proxies/getters - but I'll defer to others. @RyanCavanaugh @rbuckton
@@ -4,4 +4,3 @@ | |||
const | |||
|
|||
//// [downlevelLetConst1.js] | |||
var ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
d309ebb
to
9a99293
Compare
fixes #59877