Fixed a regression related to determining argument index when spread elements are involved#57637
Conversation
…elements are involved
src/services/signatureHelp.ts
Outdated
| // args without commas. We want to find what index we're at. So we count | ||
| // forward until we hit ourselves, only incrementing the index if it isn't a | ||
| // comma. | ||
| // forward until we hit ourselves. |
There was a problem hiding this comment.
I removed this part of the comment because I adjusted the behavior (see tests/cases/fourslash/signatureHelpSkippedArgs1.ts). I find the new behavior better and it was easier for me to rewrite those loops while accommodating for that test case.
| // arg count by one to compensate. | ||
| // | ||
| // Note: this subtlety only applies to the last comma. If you had "Foo(a,," then | ||
| // we'll have: 'a' '<comma>' '<missing>' |
There was a problem hiding this comment.
The mention of <missing> was added here like 9 years ago but the missing node is not used here for years already (I checked against some 3.x versions). When dealing with fn(,,,,) we just get a list of comma tokens
| //// const fn = thisArg[fnName]; | ||
| //// return function () { | ||
| //// return new Promise((resolve) => { | ||
| //// fn.call(thisArg, ...arguments, /*1*/); |
There was a problem hiding this comment.
Crash from #57622 (comment) happens in completions but it's caused by the same underlying issue. If requested I can add an extra completions-oriented test case too
|
@typescript-bot cherry-pick this to release-5.4 |
|
Hey, @jakebailey! I've created #57987 for you. |
DanielRosenwasser
left a comment
There was a problem hiding this comment.
I get the general gist here, and I don't see much changing in the way of existing tests which I know we spent a bunch of time on regarding picking the right overload. So I think we can take this PR.
…e-5.4 (#57987) Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
fixes #57622 (comment)
likely fixes #57623 (comment)
likely fixes #57826
the issue is a regression from #56372