Fixed crash related to JS synthethic rest param and preceeding parameters with initializers #57458
Conversation
…ters with initializers
b09a575 to
0b00da1
Compare
There was a problem hiding this comment.
I have quite a bit of different variations on tuple types used as rest here. They don't test anything specific to the referenced issues (beyond what the first 2 cases test already). I was stress-testing this and trying to trigger the report of A_required_parameter_cannot_follow_an_optional_parameter in some specific combinations. That led me to creating: #57565
I think it still doesn't hurt to leave all of those different cases here.
|
|
||
| /** @type {(a: number, b: boolean | undefined, ...rest: string[]) => void} */ | ||
| const test1 = function(value, options = undefined) { | ||
| if (arguments.length === 0) { |
There was a problem hiding this comment.
This is the only reference to arguments here. I think that perhaps this pattern (checking arguments.length) wouldn't have to make containsArgumentsReference to return true. I'm not sure if there is an appetite for this though.
src/compiler/checker.ts
Outdated
| if (isFunctionExpressionOrArrowFunction(functionDecl)) { | ||
| const contextualSignature = getContextualSignature(functionDecl); | ||
| if (contextualSignature) { | ||
| return getRestTypeAtPosition(contextualSignature, functionDecl.parameters.length); |
There was a problem hiding this comment.
Is it intentional that this isn’t assigned to links.type?
There was a problem hiding this comment.
It's intentional in the sense that I followed what some other, similar, situation was already doing. At the time of writing this, I knew that I'm not assigning to links.type and I've decided not to do it... but now I'm not 100% sure why I didn't do it.
Taking a quick look at related functions I might have been trying to replicate he intention of the comment in getTypeOfVariableOrParameterOrProperty. OTOH, that still assigns links.type - just conditionally. But yeee, I think it was to make sure that the type set by assignParameterType prevails.
There was a problem hiding this comment.
getTypeOfVariableOrParameterOrProperty only assigns conditionally because there is a checkMode, and we only cache types computed with CheckMode.Normal. This function appears to be a pure function of symbol and so AFAIK its type can/should be cached to prevent repeating work next time it’s asked for.
src/compiler/checker.ts
Outdated
| if (links.deferralParent === neverType) { | ||
| const functionDecl = links.jsSyntheticRestParameterFunctionDeclaration!; | ||
| if (isFunctionExpressionOrArrowFunction(functionDecl)) { |
There was a problem hiding this comment.
It kind of looks like you could check just links.jsSyntheticRestParameterFunctionDeclaration. Is the neverType actually relevant here?
There was a problem hiding this comment.
Yeah, you are right - this is just checking the same thing really and I can simplify it.
|
@andrewbranch this issue has already been fixed by #57580 (it doesn't come with the tests contained by this PR here - you can check them out in the current nightly though: TS playground). So the question is: is the fix here still worth pursuing? I think it might be because the other PR fixed just one of the call hierarchies leading to this problem. I now don't have any other test case at hand to prove the necessity of this PR here though |
|
It sounds like this fix may still be worth pursuing, but a failing test case probably does need to be found or reverse engineered. |
fixes #57435