Fixed a regression related to determining argument index when spread elements are involved #57637
Conversation
…elements are involved
src/services/signatureHelp.ts
Outdated
| @@ -485,8 +484,7 @@ function getArgumentIndex(argumentsList: Node, node: Node, checker: TypeChecker) | |||
| // The list we got back can include commas. In the presence of errors it may | |||
| // also just have nodes without commas. For example "Foo(a b c)" will have 3 | |||
| // 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.
| // The argument count for a list is normally the number of non-comma children it has. | ||
| // For example, if you have "Foo(a,b)" then there will be three children of the arg | ||
| // list 'a' '<comma>' 'b'. So, in this case the arg count will be 2. However, there | ||
| // is a small subtlety. If you have "Foo(a,)", then the child list will just have | ||
| // 'a' '<comma>'. So, in the case where the last child is a comma, we increase the | ||
| // 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. |
…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