Include Default Parameter Values in Signature Help

[zardoy]

Fixes #16665

Handled cases:

1. Parameter initialzer case eg const a = (b = true) => {}.

It was suspicious easy to fix for regular parameter initialzer. In some cases (I think it should be really rare) initialzer can be really long. So answering @a-tarasyuk #16665 (comment) :

function fn(a = {
  b: () => {},
  c: 2,
  d: {
    e: [1, 3, 4]
  }
}) {}

Will be shown as

fn(a?: { b: () => void; c: number; d: { e: number[]; }; } = { b: () => { }, c: 2, d: { e: [1, 3, 4] } }): void

Is that okay? 🤷

I don't like the idea of hiding information in this case, but trying to merge it to format like { c = 2 as number } would be cool imo

2. Jsdoc optional default case.

I changed parser.ts in order to store jsdoc optional default parsed expression in node. (seems it broke everything)

Will add tests tomorrow.

[jakebailey]

Given default values don't appear in d.ts files, this doesn't seem like a good idea in general because what the tooltips show will end up differing depending on how the project is loaded, right?

[zardoy]

Given default values don't appear in d.ts files, this doesn't seem like a good idea in general because what the tooltips show will end up differing depending on how the project is loaded.

I don't really get what you mean, so you think it should be extended with support for looking into source definition of function and trying to figure out the default value for parameter? Sounds interesting for some libs (but not many I think).

Also, I feel request is coming from working with large codebases first, not for libs that mostly well-documentated.

But again, even in d.ts there is a way to specify default values for parameters:

interface A {
    /**
     * @param {} [youMakeMeHappy=true] test
     */
    (youMakeMeHappy: boolean)
}

It's just a matter of d.ts generation process

[jakebailey]

The case I'm thinking of is a composite project (or maybe just references + declaration?); my understanding (someone should correct me if I'm wrong) is that if a dependency already has d.ts files generated, we can skip all of the work of type checking the dependency and instead use its d.ts files, saving a lot of time. But, those files won't have default values, so depending on whether or not those d.ts files exist, the tooltip will differ. And since having the files is the "good" case where things are fast, that's what we want to happen as much as possible, meaning that it's likely the defaults won't actually be shown to anyone in this scenario.

[zardoy]

Yes, I understand your use case of using d.ts, I was using it myself in monorepo setup. Extending my comment above, Maybe we can change the process of generation d.ts later to ensure default values aren't lost?

[jakebailey]

Maybe we can change the process of generation d.ts later to ensure default values aren't lost?

How would you emit a d.ts file from code like this?

function foo(x = someComputedValue()) { ... }

We'd have to start limiting this to just specific kinds of literals that we can rewrite (as the above likely wouldn't be workable), then track down all of the existing code which assumes that d.ts files won't ever contain anything in value space. I'm skeptical it's worth it, but, Python type stubs recently changed to start allowing default values, so... 😅

[zardoy]

Maybe we can change the process of generation d.ts later to ensure default values aren't lost?

How would you emit a d.ts file from code like this?

function foo(x = someComputedValue()) { ... }

Hm.. It seems I might be missing something, why not something like this?:

/**
 * @param {} [x=someComputedValue()]
 */
declare function foo(x?: any): void;

The same content would be displayed in signature help. I don't see anything bad here. I think this is behavior that everyone expects: just copy the initialzer text. So how can it be improved in theory?

[typescript-bot]

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

[zardoy]

@jakebailey I think I now understand what did you mean in #52819 (comment). Since I made changes to general method of typechecker symbolToParameterDeclaration you probably thought my intention is to emit initializers of parameters in d.ts files, but I wanted them to be displayed in signature help only (see added tests). Sorry, I should've added tests and NodeBuilderFlag back then.

Also notice that this pr changes typescript lib declarations (I'm now adding initializer property to JSDocParameterTag).

[zardoy]

Okay, I think I've completed it, so will be waiting for your review. Let me know if you have any thoughts

[sandersn]

@zardoy Can you explain your implementation choices back on the original bug, as well as describing how the PR addresses the questions raised there? That would help get it accepted; I don't want to put too much effort into reviewing non-accepted bugs, and it looks like the barrier for this one was an uncertainty around some semantics, plus deciding whether it's OK to output the entire type when it's large.

[zardoy]

Thank you so much for viewing this one! Of course, I don't mind having a discussion before a review.

First of all, I want to make the following clear:
All changes in this PR are to output initializers in signature help only and that is, nothing more. If I chose the wrong path to do it, give me a hint.

Public API changes:

• I added initializer to JSDocParameterTag so I can use it later checker.ts to conditionally output initializer for signature help
• I added IncludeParameterInitializers to NodeBuilderFlags so I can use it later in src/services/signatureHelp.ts

and it looks like the barrier for this one was an uncertainty around some semantics, plus deciding whether it's OK to output the entire type when it's large.

I think either me or you might be missing something here, as initializers aren't types, and doesn't signature help already output types even when they are large? What considerations can be here? eg:

const a = (a: (a: (a: (a: (a: () => void) => void) => void) => void) => void) => {}

a(/* parameter type in signature help takes more than one line */)

I'd like to answer more questions here, but can point me to specific things that were unclean? Thanks!

[sandersn]

This was the semantic question I was thinking of:

Given default values don't appear in d.ts files, this doesn't seem like a good idea in general because what the tooltips show will end up differing depending on how the project is loaded, right?

What this means is: this feature will work for .ts files in your own project, but will not work with .d.ts files -- and the latter would be the ones that get the most benefit from it. I think @jakebailey 's concern is that, in addition, it's inconsistent, so people won't be able to guess. @jakebailey can you tell me if I understood your earlier comment?

Regarding size of signature help, the reason I'm concerned is: if some percentage of signature help results are already too big, doubling the size of signature help will make a higher percentage of results too big.

Also, I believe types have truncation past a certain size (although the limit is still too high in my opinion). Does this PR truncate initialiser values? How are values truncated?

[jakebailey]

Yes, that's my impression. That and this seems to pull info from JSDoc in TS files, which we generally do not do (see signatureHelpParameterDefaultDeclaration.ts).

Truncation is definitely another problem. As far as I'm aware, truncation works by adding up a total length as we go from left to right, and at some point we quit, meaning that the more text we add, the less we show to the right.

[zardoy]

Truncation is definitely another problem. As far as I'm aware, truncation works by adding up a total length as we go from left to right, and at some point we quit, meaning that the more text we add, the less we show to the right.

As far as I understand, the only truncation we have here is getReducedType function which reduces number of elements in union or intersection to be be displayed. But it doesn't seem to truncate super long string types for example (tested with 4k chars string type), so I'm not sure how truncation could happen for not types.

[jakebailey]

During typeToString and co, doing basically anything adds to length counter. Not sure how getReducedType is related to type printing.

[zardoy]

During typeToString and co, doing basically anything adds to length counter.

I see, you mean checkTruncationLength, which uses defaultMaximumTruncationLength in order to add ... after node, but as I see it is used when building type node only...

[jakebailey]

Truncation happens for diagnostics as well as for editors like tools e.g. signature help, so it definitely matters here.

[sandersn]

@zardoy this has 3 open questions, one about usability from .d.ts files, one about use of JSDoc in TS files and one about tradeoffs in how much gets displayed. My guess is that the first problem will not be easily solvable.

Do you want to keep working on this?

[zardoy]

Thank you so much for coming back to this one!

My guess is that the first problem will not be easily solvable.

I see a pretty big interest in this feature, so I would be happy to talk about these open questions

about usability from .d.ts files

obviously we can't do anything with existing libraries and other existing .d.ts, we just simply don't have this information, but in future, we can change the process of generating .d.ts files to include such information, it's already known that adding @default to properties and default using [...] syntax is a good way to make library documentation via .d.ts better and more informative, I don't see any downsides here. Basically, #52819 (comment) answers it

one about use of JSDoc in TS files

As I understand this one is related to the previous one

tradeoffs in how much gets displayed

any ideas? I think the most common case is a simple literal value such as true or "foo" if for some reason a "big" function call or long string literal value is used we can simply truncate the resulting output to let's say 20 characters. This way even with existing extremely complex function signatures we make function signatures just a bit longer. Overall don't see a reason to decline this feature for regular .ts users just because of the existence of .d.ts.

Do you want to keep working on this?

To be honest, I don't have much time now, but I would be happy to continue negotiating the current code changes & the general idea and maybe even continue developing it.

[sandersn]

Unfortunately, we haven't had time to work out the design of the feature, and this PR is quite old now. I think it makes most sense to close this PR and re-open it once the open questions have been resolved in discussion on the original issue.