Fixed parenthesized array literal expressions spread in calls not being tupleized #54623
Conversation
| function checkArrayLiteral(node: ArrayLiteralExpression, checkMode: CheckMode | undefined, forceTuple: boolean | undefined): Type { | ||
| const elements = node.elements; | ||
| const elementCount = elements.length; | ||
| const elementTypes: Type[] = []; | ||
| const elementFlags: ElementFlags[] = []; | ||
| pushCachedContextualType(node); | ||
| const inDestructuringPattern = isAssignmentTarget(node); | ||
| const isSpreadIntoCallOrNew = isSpreadElement(node.parent) && isCallOrNewExpression(node.parent.parent); | ||
| const inConstContext = isSpreadIntoCallOrNew || isConstContext(node); |
There was a problem hiding this comment.
While reading the source code today I noticed that isSpreadIntoCallOrNew here (as part of inConstContext).
I think it was semantically incorrect (and I added it here). It didn't exactly create any issues because isConstContext here is mainly used to decide if the checked type should be readonly or not.
Since I removed part of the condition we can now observe removal of the readonly modifier from some snapshot tests. I think it's fine... or at least, I wasn't yet able to prove that those tuples should be readonly. Even in actual const contexts etc a mutable array is still fine. Even if it turns out that the readonly modifier is needed for some case, I think that it should be added independently, based on isSpreadIntoCallOrNew.
As a bonus, I noticed that parenthesized expressions were not handled here so I improved this.
There was a problem hiding this comment.
The changed baselines that didn’t strictly need to change make this hard to approve. I’d rather not find out that the readonly was important in an untested edge case because of a regression.
There was a problem hiding this comment.
Should I split this into 2 PRs then? Note that the change that allowed those and introduced those readonly modifiers in some cases were only merged into 5.1 so removing it really shouldn't affect many people.
There was a problem hiding this comment.
Ah, I missed that the same PR originally made them readonly. Probably fine then
| function checkArrayLiteral(node: ArrayLiteralExpression, checkMode: CheckMode | undefined, forceTuple: boolean | undefined): Type { | ||
| const elements = node.elements; | ||
| const elementCount = elements.length; | ||
| const elementTypes: Type[] = []; | ||
| const elementFlags: ElementFlags[] = []; | ||
| pushCachedContextualType(node); | ||
| const inDestructuringPattern = isAssignmentTarget(node); | ||
| const isSpreadIntoCallOrNew = isSpreadElement(node.parent) && isCallOrNewExpression(node.parent.parent); | ||
| const inConstContext = isSpreadIntoCallOrNew || isConstContext(node); |
There was a problem hiding this comment.
The changed baselines that didn’t strictly need to change make this hard to approve. I’d rather not find out that the readonly was important in an untested edge case because of a regression.
sandersn
added
the
For Uncommitted Bug
No description provided.