Skip to content
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

Allow signature help argument count to be equal to argument index #58203

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Andarist
Copy link
Contributor

fixes the issue reported here #58191 (comment)
a regression from #56372 , not fixed by #57637

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Apr 15, 2024
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

Comment on lines -290 to -292
if (argumentIndex !== 0) {
Debug.assertLessThan(argumentIndex, argumentCount);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it might look weird that this allows what usually isn't allowed - it all depends on the definition of those values. I think it makes sense to treat them as follows:

  • argument index - a min argument index at the position
  • argument count - a minimum count of known arguments

A spread element doesn't always increase such an argument count but the argument index at its positions gets increased

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what should an editor do when the argument index exceeds the count? The index has to be capped, doesn't it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of bounds indexes are just ignored; on Python I did this intentionally, too (previously used a negative number until some rust-based LSP clients started getting picky). See: microsoft/language-server-protocol#1271

But, I'm more confused by how a spread element would change the number here; we're definitely indexing them correctly left to right, correct? Like if we have (x, {y, z}), we don't go out of bounds on z because it's "at index 2" but the number of real params is 2? I assume not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what should an editor do when the argument index exceeds the count? The index has to be capped, doesn't it?

The added baseline here shows how we can still highlight the parameter in the signature - at least in this case 😉

But, I'm more confused by how a spread element would change the number here

function test(...args: any[]) {}

const twoNums = [2, 3] as const
test(1, ...twoNums)

In the situation above the rest adds 2 to the count (and not 1). See the PR that changed this: #56372

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: That's a spread, not a rest. Rest is what you do when destructuring, spread is what you do at construction ("rest" doesn't make sense in that context because you can have multiple).

@jakebailey
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 19, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
pack this ✅ Started ✅ Results

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 19, 2024

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/161380/artifacts?artifactName=tgz&fileId=BCF59F43A5EC9B506AB89789B11D216B6577E1C070792765F4BBC471CF9C056502&fileName=/typescript-5.5.0-insiders.20240419.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.5.0-pr-58203-3".;

@sandersn sandersn added this to Not started in PR Backlog May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
  
Not started
Development

Successfully merging this pull request may close these issues.

None yet

5 participants