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

Add inlay hints support #42089

Merged
merged 57 commits into from
Jun 25, 2021

Conversation

Kingwl
Copy link
Contributor

@Kingwl Kingwl commented Dec 23, 2020

Fixes #42073

Features:

  • Parameter name hint for arguments
  • Type hint for parameter
  • Type hint for variable
  • Type hint for field
  • Type hint for return type
  • Type hint for call chains
  • Value hint for numerical enums
  • Control to show parameter name hint for non-literal arguments or not
  • Control to show type hints for require assigned variable.
  • Control to show parameter name if arguments is the same as parameter name

Something need to consider:

  • Truncated: For now. I have truncate the hints with hard code 30.
  • Preference: We have many options to specified the behavior. Currently we pass them by UserPreferences.
  • Complex types: Many types may very very long after serialization (function like, complex object, etc.). Should we handle them?
  • Type Parameter: The root cause is we could inference type from type node or type. Which one should we take? I think type is better.

Thanks!

@typescript-bot
Copy link
Collaborator

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Dec 23, 2020
@Kingwl
Copy link
Contributor Author

Kingwl commented Dec 23, 2020

@mjbvz
Please take a look about protocol or naming.
Thanks!

@Kingwl
Copy link
Contributor Author

Kingwl commented Dec 24, 2020

image

Basically work like this.

@Kingwl
Copy link
Contributor Author

Kingwl commented Dec 27, 2020

@typescript-bot pack this.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 27, 2020

Heya @Kingwl, I've started to run the tarball bundle task on this PR at d2fbd1e. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 27, 2020

Hey @Kingwl, 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/92086/artifacts?artifactName=tgz&fileId=5999C09B7BA4F0275D38BC9FCAC3474C22A8EDBA92BDD336E0A2658DE15F7C1B02&fileName=/typescript-4.2.0-insiders.20201227.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@4.2.0-pr-42089-5".;

@Kingwl Kingwl changed the title [WIP] Add signature arguments label support Dec 28, 2020
@Kingwl Kingwl marked this pull request as ready for review December 28, 2020 08:04
@typescript-bot
Copy link
Collaborator

The TypeScript team hasn't accepted the linked issue #42073. If you can get it accepted, this PR will have a better chance of being reviewed.

@Kingwl Kingwl changed the title Add signature arguments label support Dec 28, 2020
@Kingwl Kingwl marked this pull request as draft December 29, 2020 10:02
@andrewbranch
Copy link
Member

Should identifier be a literal argument?

Well, let me think about some examples:

f(1);
f(1 + 2);

f("Some String");
f("Some String".toLowerCase());
f(someString);
f(someString.toLowerCase());

f([0, 1, 2]);
f([0, 1, 2].map(processNumber));
f(integers);
f(integers.map(processNumber));

In these cases, I feel like the literal arguments and the expressions made up of literals convey the same amount of semantic information. I don’t think the + 2 or the .toLowerCase() or the .map(processNumber) does anything to negate the value of the parameter name hint.

That said, someString vs. someString.toLowerCase() and integers vs. integers.map(processNumber) also feel like they convey about the same amount of information. I’m fine just doing the same thing as C#, but the ParenthesizedExpression example in the test looked very strange to me. I think it would be appropriate to skipParentheses since we generally recognize that parens have no semantic meaning. But it’s not a big deal. Maybe someone would find it valuable to be able to force a hint to show up by parenthesizing an identifier. It’s not a blocker from me, just curious what the rationale was.

@Kingwl
Copy link
Contributor Author

Kingwl commented Jun 24, 2021

Thanks for the review. I've updated followed the comments.

src/compiler/types.ts Outdated Show resolved Hide resolved
src/server/protocol.ts Outdated Show resolved Hide resolved
src/server/protocol.ts Outdated Show resolved Hide resolved
@jessetrinity
Copy link
Contributor

I haven't tried this out yet to see how it actually plays. I assume I need vscode insiders?

@andrewbranch
Copy link
Member

@jessetrinity you need a local build of microsoft/vscode#113412

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Thanks for bearing with us on the last-minute updates, @Kingwl 🌟

Copy link
Contributor

@jessetrinity jessetrinity left a comment

Choose a reason for hiding this comment

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

InlayHintKind was changed to

export const enum InlayHintKind {
  Type = "Type",
  Parameter = "Parameter",
  Enum = "Enum",
}

in protocol.ts but nowhere else.

src/services/types.ts Outdated Show resolved Hide resolved
tests/baselines/reference/api/tsserverlibrary.d.ts Outdated Show resolved Hide resolved
tests/baselines/reference/api/typescript.d.ts Outdated Show resolved Hide resolved
tests/cases/fourslash/fourslash.ts Outdated Show resolved Hide resolved
@jessetrinity jessetrinity merged commit 66b4ba4 into microsoft:main Jun 25, 2021
PR Backlog automation moved this from Waiting on author to Done Jun 25, 2021
@Kingwl Kingwl deleted the signature_arguments_labels_support branch June 25, 2021 06:42
@Kingwl
Copy link
Contributor Author

Kingwl commented Jun 25, 2021

Thanks!

@Avishayy
Copy link

Avishayy commented Jan 9, 2023

Thanks for creating such a great feature!

I have a question, why is there a limit on the hint length? Specifically, why is maxHintsLength set to 30? Are there any plans to change it sometime?

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