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

Default type argument handling added in 53fa22e is buggy #2823

Open
hoodmane opened this issue Dec 30, 2024 · 2 comments
Open

Default type argument handling added in 53fa22e is buggy #2823

hoodmane opened this issue Dec 30, 2024 · 2 comments
Labels
bug Functionality does not match expectation

Comments

@hoodmane
Copy link

hoodmane commented Dec 30, 2024

The changes in #2820 have a couple of shortcomings. The new logic removes a type argument if it matches the default value of any type parameter. What we have to do is work from the last type argument backwards and check each one in turn for whether it matches the default. We need to include all arguments before the last nondefault argument even if they are defaults.

interface T<A, B=number, C=string> {a: A, b: B, c: C}

// Expected:    f0(a: T<number, number, boolean>): T<number, number, boolean>
// Got     :    f0(a: T<boolean>): T<number, number, boolean>
export function f0(a: T<number, number, boolean>): T<number, number, boolean> { return a; }

// Expected:    f1(a: T<string>): T<string>
// Got     :    f1(a: T): T<string>
export function f1(a: T<string>): T<string> { return a; } 

// Expected:    f2(a: T<number>): T<number>
// Got     :    f2(a: T): T<number>
export function f2(a: T<number>): T<number> { return a; }

// Expected:    f3(a: T<number>): T<number>
// Got     :    f3(a: T): T<number, number>
export function f3(a: T<number, number>): T<number, number> { return a; }

// Expected:    f4(a: T<number, string>): T<number, string>
// Got     :    f4(a: T): T<number, string>
export function f4(a: T<number, string>): T<number, string> { return a; }

// Expected:    f5(a: T<string, string>): T<string, string>
// Got     :    f5(a: T): T<string, string>
export function f5(a: T<string, string>): T<string, string> { return a; }

// Expected:    f6(a: T<number, string>): T<number, string>
// Got     :    f6(a: T): T<number, string, string>
export function f6(a: T<number, string, string>): T<number, string, string> { return a; }

// Expected:    f7(a: T<number>): T<number>
// Got     :    f7(a: T): T<number, number, string>
export function f7(a: T<number, number, string>): T<number, number, string> { return a; }

The following patch mostly fixes it, though it leaves the very minor inconsistency that the return type always shows exactly what the source code contains whereas the parameter types will omit defaults that were unnecessarily included in the source code.

--- a/src/lib/converter/types.ts
+++ b/src/lib/converter/types.ts
@@ -824,23 +824,24 @@ const referenceConverter: TypeConverter<
                 convertType(context, (type as ts.StringMappingType).type),
             ];
         } else {
-            // Default type arguments are filled with a reference to the default
-            // type. As TS doesn't support specifying earlier defaults, we know
-            // that this will only filter out type arguments which aren't specified
-            // by the user.
-            let ignoredArgs: ts.Type[] | undefined;
-            if (isTypeReference(type)) {
-                ignoredArgs = type.target.typeParameters
-                    ?.map((p) => p.getDefault())
-                    .filter((x) => !!x);
-            }
-
             const args = type.aliasSymbol
                 ? type.aliasTypeArguments
                 : (type as ts.TypeReference).typeArguments;
 
+            let idx;
+            if (args && isTypeReference(type)) {
+                idx = args.length - 1;
+                while (
+                    idx >= 0 &&
+                    args?.at(idx) ===
+                        type.target.typeParameters?.at(idx)?.getDefault()
+                ) {
+                    idx--;
+                }
+            }
+
             ref.typeArguments = args
-                ?.filter((ref) => !ignoredArgs?.includes(ref))
+                ?.slice(0, idx! + 1)
                 .map((ref) => convertType(context, ref));
         }
         return ref;
@hoodmane hoodmane added the bug Functionality does not match expectation label Dec 30, 2024
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Dec 30, 2024

Shoot, I completely forgot about intrinsic types which share identity with others... I'm pretty sure your proposed patch doesn't fix some cases as well. Will try to find the time to look at this again soon

@hoodmane
Copy link
Author

Thanks @Gerrit0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Functionality does not match expectation
Projects
None yet
Development

No branches or pull requests

2 participants