Skip to content

use default type parameter rather than unknown when show quick help #46792

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 9, 2021
Merged

use default type parameter rather than unknown when show quick help #46792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d08dfba
use default type parameter rather than `unknown` when show quick help
ShuiRuTian Nov 12, 2021
a0aa803
prefer default type parameter than constraint for quick info
ShuiRuTian Nov 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30248,7 +30248,7 @@ namespace ts {
typeArguments.pop();
}
while (typeArguments.length < typeParameters.length) {
typeArguments.push(getConstraintOfTypeParameter(typeParameters[typeArguments.length]) || getDefaultTypeArgumentType(isJs));
typeArguments.push(getDefaultFromTypeParameter(typeParameters[typeArguments.length]) || getConstraintOfTypeParameter(typeParameters[typeArguments.length]) || getDefaultTypeArgumentType(isJs));
Copy link
Member

Choose a reason for hiding this comment

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

In what cases does this loop still run?

Copy link
Contributor Author

@ShuiRuTian ShuiRuTian Nov 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

like this(the example is from instantiateGenericClassWithWrongNumberOfTypeArguments.ts):

class D<T, U> {
    x: T
    y: U
}

var d = new D<number>();
// when hover on `d`, it shows `var d: D<number, unknown>`
//                                               ^^^^^^ here is added by this loop, and if `U` has a constraint, it would be the constraint.

fillMissingTypeArguments only fills typeArgument when there are valid default type parameter, which means the number of typeArgument is between minTypeParameterCount and typeParameterCount. It does not always fill the type argument to full.

Copy link
Contributor Author

@ShuiRuTian ShuiRuTian Nov 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

And, we would better to mention this case:

interface A { a: number }
interface B extends A { b: string }

class D<T, U, V extends A = B> {
    x: T
    y: U
    z: V
}

var d = new D<number>();
// when hover on `d`, it shows `var d: D<number, unknown, A>`
// `A` is its constraint rather than the default type parameter `B`, this PR does not change this behavior now

However, it is a little different: there is an error on the number of type arguments. So I feel that both methods are OK? Or the default type parameter is better than constraint?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I feel like using the default here makes a little more sense, but I don’t know that it’s super critical since it’s an error anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now prefer default type parameter than constraint for quick info. Correspond tests are added.

Copy link
Member

Choose a reason for hiding this comment

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

It should be noted that this change doesn’t just affect quick info; it actually affects the type instantiated in these kinds of error scenarios. I’m surprised there weren’t any existing baselines that needed updating. In your example above, you can check the type of d.z and see that in main, the type is A, and this branch, the type is B.

Copy link
Member

Choose a reason for hiding this comment

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

The new behavior is more intuitive to me, but others should weigh in. The most conservative thing would be to leave it as it was, against my earlier advice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing it out, I did not notice it! This does make sense. And, I am surprised too :)
But do we really need to revert to last commit? For:

  1. we are doing things in the right way
  2. anyway, it is just an error condition which finally need be fixed by user

Again, I feel both methods are OK. The final decision is up to you~

}
return typeArguments;
}
Expand Down
35 changes: 28 additions & 7 deletions tests/cases/fourslash/signatureHelpExplicitTypeArguments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,37 @@
/// <reference path='fourslash.ts'/>

////declare function f<T = boolean, U = string>(x: T, y: U): T;
////f<number, string>(/*1*/);
////f(/*2*/);
////f<number>(/*3*/);
////f<number, string, boolean>(/*4*/);
//// declare function f<T = boolean, U = string>(x: T, y: U): T;
//// f<number, string>(/*1*/);
//// f(/*2*/);
//// f<number>(/*3*/);
//// f<number, string, boolean>(/*4*/);

//// interface A { a: number }
//// interface B extends A { b: string }
//// declare function g<T, U, V extends A = B>(x: T, y: U, z: V): T;
//// declare function h<T, U, V extends A>(x: T, y: U, z: V): T;
//// declare function j<T, U, V = B>(x: T, y: U, z: V): T;
//// g(/*5*/);
//// h(/*6*/);
//// j(/*7*/);
//// g<number>(/*8*/);
//// h<number>(/*9*/);
//// j<number>(/*10*/);

verify.signatureHelp(
{ marker: "1", text: "f(x: number, y: string): number" },
{ marker: "2", text: "f(x: boolean, y: string): boolean" },
// too few -- fill in rest with unknown
{ marker: "3", text: "f(x: number, y: unknown): number" },
// too few -- fill in rest with default
{ marker: "3", text: "f(x: number, y: string): number" },
// too many -- ignore extra type arguments
{ marker: "4", text: "f(x: number, y: string): number" },

// not matched signature and no type arguments
{ marker: "5", text: "g(x: unknown, y: unknown, z: B): unknown" },
{ marker: "6", text: "h(x: unknown, y: unknown, z: A): unknown" },
{ marker: "7", text: "j(x: unknown, y: unknown, z: B): unknown" },
// not matched signature and too few type arguments
{ marker: "8", text: "g(x: number, y: unknown, z: B): number" },
{ marker: "9", text: "h(x: number, y: unknown, z: A): number" },
{ marker: "10", text: "j(x: number, y: unknown, z: B): number" },
);