Typescript: Unexpected parameter hint showing up for `log` in js file

[mjbvz]

From https://github.com/microsoft/vscode/issues/80565

TypeScript Version: 3.7.20190922

Search Terms:

• javascript
• parameter hints / signature help

Code
In a js file:

//@ts-check

log(

Trigger parameter hints on log

Expected behavior:
Nothing shows up. log is undefined here and you can't run go to definition on it

Actual behavior:

Playground Link:

Related Issues:

[RyanCavanaugh]

wat

[a-tarasyuk]

I investigated the issue, and this error happens because

https://github.com/microsoft/TypeScript/blob/1bb6ea038f8ca7e3b2037f8c3209a19c097f1e3b/src/services/signatureHelp.ts#L125

returns es5.d.ts source file and

https://github.com/microsoft/TypeScript/blob/1bb6ea038f8ca7e3b2037f8c3209a19c097f1e3b/src/services/signatureHelp.ts#L126

returns sourceFile.getNamedDeclarations() all declarations and ignore namespaces. For instance log it is a name from Math object :).

Is there any reason why sourceFile.getNamedDeclarations() is used here?

cc @DanielRosenwasser @RyanCavanaugh

[DanielRosenwasser]

I think the idea is to try a best-effort signature help given that we're in JavaScript and we won't always have accurate types around.

If you're in ts-check, this behavior is probably extremely surprising because you're also getting an error that the identifier can't be found. Maybe the solution is to turn it off just there.

[a-tarasyuk]

@DanielRosenwasser Do you mean disable/remove createJSSignatureHelpItems?

[DanielRosenwasser]

Yeah, if

• the current file belongs to a tsconfig.json
• the current file is checked
• the current file belongs to a project with checkJs

I think we should probably disable the heuristic. Does that sound reasonable?

[a-tarasyuk]

@DanielRosenwasser It seems in this case, users who use JavaScript without TS checks will get weird suggestions. Or am I wrong?

https://github.com/microsoft/TypeScript/blob/ad8feb5f90e97bb9326133464968927254927fbb/src/services/signatureHelp.ts#L48

Returns correct info for global functions (parseInt, eval, etc), or Objects (Math.log, Date etc). I'm trying to understand where this heuristic can help :smiley: :eyes:

[mjbvz]

Would it be ok to only return signature help when we can properly resolve a symbol? This is what we do for quickInfo for example.

The majority of VS Code users are on untyped JS so I’m wary of a fix that is specific to checkJs

[a-tarasyuk]

createJSSignatureHelpItems was added in this PR https://github.com/microsoft/TypeScript/pull/2641, and changed in this https://github.com/microsoft/TypeScript/pull/26025. Both PRs don't have tests - and it is difficult to understand which cases these changes cover, however, without createJSSignatureHelpItems tests work fine.

[DanielRosenwasser]

My personal take is that in a checked JS file, or in a TypeScript project, we should not apply the heuristic. Otherwise, I think the heuristic is fine.

[DanielRosenwasser]

Ah, I just looked up and saw that reread the convo.

Uh, maybe this is something we should try to understand better - would it be easy to conduct a user study on this? Let's chat about this tomorrow @mjbvz

[mjbvz]

Sure. I'm still of the opinion that the behavior of parameter hints should match hover. It seems confusing to show parameter hints on symbols that we can't resolve

[RyanCavanaugh]

Notes from meeting today:

• Disable this behavior entirely for bare identifiers
• Add some description of which declaration the description is coming from

[a-tarasyuk]

@RyanCavanaugh Thanks for the update.

What does mean _bare identifiers_? 😕

Add some description of which declaration the description is coming from

Do you mean to add an additional field to the server response?

[DanielRosenwasser]

@a-tarasyuk Ryan can comment on the second part.

To answer your first question, the idea is foo should not try to search for members named foo anywhere, but foo.bar should still look for declarations named bar. That seems fairly reasonable to me.