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

IntelliSense shows incorrect parameter names (TypeScript) #31845

Closed
korkje opened this issue Jun 9, 2019 · 7 comments · Fixed by #32056
Closed

IntelliSense shows incorrect parameter names (TypeScript) #31845

korkje opened this issue Jun 9, 2019 · 7 comments · Fixed by #32056
Labels
Bug A bug in TypeScript
Milestone

Comments

@korkje
Copy link

korkje commented Jun 9, 2019

I am writing a helper function that provides functionality often associated with the traditional switch statement, but in a more functional style. In doing so, I have come across a bug.

Consider the following code, which is a simplification of the helper I'm writing:

const match = <T>(value: T) => ({
    on: <U>(prediction: T, outcome: U) => prediction === value
        ? matched(outcome)
        : match(value),
    result: () => undefined
});

const matched = <T>(outcome: T) => ({
    on: () => matched(outcome),
    result: () => outcome
});

const num = Math.random() > 0.5 ? 1 : 2;

const numString = match(num)
    .on(1, 'one')
    .on(2, 'two')
    .result();

When hovering the first on call, we see the expected information:

Screenshot 2019-06-09 at 20 06 00

However, when hovering the second on call, which is a call to the on property of the result of either match or matched, we get:

Screenshot 2019-06-09 at 20 06 12

As we can see, the types are just fine, but the argument names are replaced with 'arg0' and 'arg1'. This in itself could be intentional for some reason, and hence not a bug, but here's what leads me to consider this a bug:

When removing the last argument to the second on call, IntelliSense gives us an error as expected, but with an error text containing one of the previously replaced argument names, 'outcome':

Screenshot 2019-06-09 at 20 06 38

The same goes for 'prediction' if both arguments are removed.

I suspect this might be a result of trying to derive the parameter names from the second definition of on (as a result of the matched function), which would be inconsistent as the rest of the call signature is clearly derived from the first definition (as a result of the match function). Or I am completely wrong, which is also very much possible.

Anyway, this is misleading. What I expect is for both the call signature and error text to contain 'prediction' and 'outcome' as the parameter names, or at the very least for these to be consistent.

PS: The full thing is at https://github.com/korkje/match/blob/master/src/index.ts, and exhibits the same weirdness.

@korkje
Copy link
Author

korkje commented Jun 9, 2019

Also, weirdly, if you hover .on before typing the parentheses, the correct parameter names are shown.

@mjbvz mjbvz transferred this issue from microsoft/vscode Jun 10, 2019
@mjbvz
Copy link
Contributor

mjbvz commented Jun 10, 2019

Confirmed with TS 3.6.0-dev.20190608

@mjbvz mjbvz removed their assignment Jun 10, 2019
@fatcerberus
Copy link

Possibly related: Playground link

function frob<T, A extends any[]>(f: (...args: A) => T, ...args: A)
{
    return f(...args);
}

function blub(foo: string, bar: number) {}

frob(blub, "banana slamma", 812);

image

Parameter names are correct while typing out the call, but...

image

once everything is totally filled out it reverts to generic arg_0 etc.

@dragomirtitian
Copy link
Contributor

dragomirtitian commented Jun 13, 2019

@RyanCavanaugh can I have a look at this ? I dug around previously in the area (#30084)

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Jun 13, 2019
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Jun 13, 2019
@RyanCavanaugh
Copy link
Member

Sure!

@dragomirtitian
Copy link
Contributor

dragomirtitian commented Jun 22, 2019

@RyanCavanaugh A smaller repro of this would be:

let fn: (<U>(a1: U) => void) | (() => void) 
fn(1) //  quick info: let fn: <number>(arg0: number) => void

The issue is that when the signatures in the union are combined, instead of using the only available name (ex: a1) the fallback for when parameter names don't match is used (arg${i}). This does not manifest for non-generic functions as there the signature is just cloned.

Possible Solutions:

  1. Targeted fix for just this case: Fix the combination algorithm to use the available name when one of the combined signatures has fewer parameters. This should be a quick easy fix.
  2. Improve on arg${i}: arg${i} is not particularly useful or friendly, and this is what is used when signatures have conflicting parameter names. Perhaps we could concatenate parameter names, ideally to something meaningful like p1 & p2 (where p1 and p2 come from the original signatures). While this is not normally a valid name for a parameter, it would make clear to the user that this is an amalgamation of the two original parameter and will probably not have any negative impact in other places since this is just a signature used for type checking (but I have not investigated this).

Ex:

let fn2: ((p1:  { b: string }) => void) | ((p2:  { a: number }) => void) 
fn2({}) // quick info: let fn2(p1 & p2: { b: string; } & { a: number; }): void

This is a bit of a whacky idea but it might be useful 🤷‍♀️

Should I just proceed with 1 ? Or experiment with 2 ?

@fatcerberus The issue you found has a different root cause to the one in the original post, it might merit a ticket of its own.

@fatcerberus
Copy link

fatcerberus commented Jun 22, 2019

For my part I kind of like the second idea, when you have a union of functions it ends up requiring an intersection of arguments (by way of contravariance) which tends to throw people off especially if they're not used to seeing intersections in everyday code. So it might be good to provide a hint that the signature on display was in fact synthesized from two (or more) different functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants