fix: compilation error when nested specialization of generic types #145

Shanks0224 · 2024-02-03T10:46:06Z

No description provided.

Shanks0224 · 2024-02-03T13:52:03Z

I am preparing to write a detailed documentation and will submit it when completed.

xujuntwt95329 · 2024-02-03T15:13:15Z

I am preparing to write a detailed documentation and will submit it when completed.

That would be great! Looking forward to it!

Signed-off-by: ganjing <[email protected]>

yviansu · 2024-02-22T07:24:57Z

src/utils.ts

+    const typeArgumentsSignature: Array<string> = [];
+    const _typeParameters = genericClassType.typeArguments;
+    if (_typeParameters) {
+        _typeParameters.forEach((type) => {
+            const index = typeParameters.findIndex((t) => {
+                return t.name === type.name;
+            });
+            if (index == -1) {
+                throw new UnimplementError(
+                    `${type.name} not found in typeParameters`,
                );
+            }
+            typeArgumentsSignature.push(`${typeArguments[index].kind}`);
+        });
+    }


I noticed that these codes are aimed to get the typeArgumentsSignature, and used in several functions, why not encapsulate these into a separate function(like getTypeSignatureByTypeArguments)?

yviansu · 2024-02-22T07:28:13Z

src/utils.ts

+    const typeSignature =
+        typeArgumentsSignature.length > 0
+            ? '<' + typeArgumentsSignature.join(',') + '>'
+            : '';


And typeArgumentsSignature is only used to get the typeSignature string, maybe we can put this operation in the separate function(getTypeSignatureByTypeArguments), and use the typeSignature as return value.

yviansu · 2024-02-22T07:42:19Z

src/utils.ts

+    const cache = TypeResolver.specializedTypeCache.get(genericOwner);
+    let found: Type | undefined;
+    if (cache) {
+        cache.forEach((v) => {
+            if (v.has(genericClassType.className + typeSignature)) {
+                found = v.get(genericClassType.className + typeSignature);
+            }
+        });
+    }
+    if (found) return found as TSClass;


This operation seems is used in several functions too,
Maybe we can set a separate function:

function findTypeInSpecialiezedTypeCache(typeName: string): Type { xxx }

yviansu · 2024-02-22T07:53:38Z

src/utils.ts

+            const reverse = genericMangledName.split('|').reverse();
+            reverse[0] = specializedName;
+            newClassType.mangledName = reverse.reverse().join('|');


These operations seems a little complex, how about

newClassType.mangledName = genericMangledName.substring(0, genericMangledName.lastIndexOf("|") + 1) + specializedName;

yviansu · 2024-02-22T08:06:46Z

src/utils.ts

+        // update specializedTypeCache
+        if (TypeResolver.specializedTypeCache.has(genericOwner)) {
+            const value = new Map();
+            value.set(specializedName, newClassType);
+            TypeResolver.specializedTypeCache.get(genericOwner)!.push(value);
+        } else {
+            const value = new Map();
+            value.set(specializedName, newClassType);
+            TypeResolver.specializedTypeCache.set(genericOwner, [value]);
+        }


Maybe the operation update specializedTypeCache can be put in a function too?

yviansu · 2024-02-22T08:15:04Z

src/utils.ts

+    // specialized function type need to reset belongedClass and belongedScope
+    newFuncType.belongedClass = undefined;
+    newFuncType.setBelongedScope(undefined);


In this function, the belongedClass and belongedScope are both be set to undefined, where are they set to new values?
What's more, for function, why does it have belongedClass?

In processGenericType() , if it is processing a specialization of a generic, genericTypeSpecialization() will be called first to generate a new specialized type and its corresponding Scope will first be set to undefined, as marked by this comment. The createScopeBySpecializedType() will then be called to create a new Scope.

For the default function, there is no belongedClass property, and the code newFuncType.belongedClass = undefined; is redundant.

yviansu · 2024-02-22T09:32:26Z

src/utils.ts

+    return newFuncType;
+}
+
+export function genericClassMethodSpecialization(


This function is only slightly different from genericFunctionSpecialization. Is it possible to merge them into one function?

It is possible to merge them together. genericMethodSpecialization will have some logic for processing Class. I encapsulated them separately, just to distinguish method and function.

Yeah, how about use a parameter to distinguish method and function, like isMethod, and set the default value to true or false, so that we can reuse the common logic.

yviansu · 2024-02-22T09:42:58Z

src/utils.ts

-    parent: Scope,
-    classType: TSClass,
-    newName?: string,
+export function genericMethodSpecialization(


This function is aim to generate functionScope and set its type for class' methods, right?

The other genericXXSpecialization functions are aim to generate specialization types, which may be cause some misunderstandings, so this function can rename as other names, like methodSpecialize etc.

Yes, this function is designed for the following situation:

class A { a: number; echo<T>(param: T) {...}; } const a = new A(); a.echo(1); a.echo('hello');

yviansu · 2024-02-22T09:51:36Z

src/utils.ts

+ * @param context the parser context
+ * @returns a new generic type
+ */
+export function genericTypeTransformation(


What's the difference between genericTypeSpecialization and genericTypeTransformation?

When does genericTypeSpecialization need to be called, and when does genericTypeTransformation need to be called?

Generic specialization refers to using specific types (such as number, string) to specialize the generic T. In order to realize this feature, we designed the genericTypeSpecialization().

Generics have chain relationships:

class A<T> {...} class B<X, Y> extends A<Y> {...}

In this case, we need to generate a new generic type B_A<Y>, and its specialization operation will not affect its original type A<T>. genericTypeTransformation()handles this situation.

In the function processGenericType, we can decide whether to perform specialization of the generic type or create a new generic type by whether typeArguments is a generic types.

I still think there is not much difference between genericTypeSpecialization and genericTypeTransformation. Their logic is exactly the same, and their inner logic, like genericFunctionSpecialization and genericFuncTransformation is close too.

In genericFunctionSpecialization:

get the new type list

find if there already exists the specialized type, if found, return

generate new type

do sepecialization

return the new type

In genericFuncTransformation:

get the new type list

generate new type

return the new type

We can use a parameter doSpecialization to choose which action we require, the codes existed now is much duplicated.

yviansu · 2024-02-22T10:10:54Z

src/utils.ts

+ * @param context the parser context
+ * @returns a new specialized FunctionScope
+ */
+function createFunctionScope(


There is not big difference between createFunctionScope and createMethodScope, can we use a common function to represent?

It is possible to merge them together. createMethodScope will have some logic for processing Class. I encapsulated them separately, just to distinguish method and function.

Yeah, and how about merge them, and use a parameter to distinguish them? since their inner logic is very close.

yviansu · 2024-02-26T07:00:23Z

src/type.ts

+            baseClassType = genericClassTransformation(
+                baseClassType,
+                typeArguments,
+                baseClassType.typeArguments!,
+                this.parserCtx,
+                classType.className,
+            ) as TSClass;


Why not use processGenericType?
I see that genericClassTransformation is used in processGenericType, in this situation, if invoke processGenericType directly, can it go to genericClassTransformation path?

Yes, we should call processGenericType directly

yviansu · 2024-03-06T03:49:52Z

src/expression.ts

-                    res.setExprType(this.typeResolver.generateNodeType(node));
+                    const newSuperExpression = new SuperExpression(args);
+                    newSuperExpression.tsNode = (
+                        expr as SuperExpression


In the outer function visitNode, the tsNode will be recorded in the expression, so I think there is no need to record the tsNode here again.

Wasmnizer-ts/src/expression.ts

Lines 489 to 493 in 0fb80b5

visitNode(node: ts.Node): Expression {

const expr = this.visitNodeInternal(node);

expr.tsNode = node;

return expr;

}

yviansu · 2024-03-06T03:51:31Z

src/expression.ts

+                                            ).propertyAccessExpr,
+                                            newPropertyIdentifier,
+                                        );
+                                        expr.tsNode = tsNode;


same as above

yviansu · 2024-03-06T03:51:45Z

src/expression.ts

                                    );
-                                    res = classType.getMethod(newPropertyName);
+                                    expr.tsNode = tsNode;


same as above

yviansu · 2024-03-06T03:52:01Z

src/expression.ts

+                      )
+                    : exprType;
+                newSuperExpression.setExprType(newExprType);
+                newSuperExpression.tsNode = superExpression.tsNode;


same as above

yviansu · 2024-03-06T03:57:22Z

src/utils.ts

+    return newFuncType;
+}
+
+export function genericClassMethodSpecialization(


Yeah, how about use a parameter to distinguish method and function, like isMethod, and set the default value to true or false, so that we can reuse the common logic.

yviansu · 2024-03-06T04:05:34Z

src/utils.ts

+ * @param context the parser context
+ * @returns a new generic type
+ */
+export function genericTypeTransformation(


I still think there is not much difference between genericTypeSpecialization and genericTypeTransformation. Their logic is exactly the same, and their inner logic, like genericFunctionSpecialization and genericFuncTransformation is close too.

In genericFunctionSpecialization:

get the new type list

find if there already exists the specialized type, if found, return

generate new type

do sepecialization

return the new type

In genericFuncTransformation:

get the new type list

generate new type

return the new type

We can use a parameter doSpecialization to choose which action we require, the codes existed now is much duplicated.

yviansu · 2024-03-06T04:06:48Z

src/utils.ts

+ * @param context the parser context
+ * @returns a new specialized FunctionScope
+ */
+function createFunctionScope(


Yeah, and how about merge them, and use a parameter to distinguish them? since their inner logic is very close.

yviansu

LGTM

xujuntwt95329

LGTM

xujuntwt95329 · 2024-03-12T01:49:34Z

Thanks @Shanks0224 and @yviansu

Shanks0224 force-pushed the main branch 4 times, most recently from 0a67d7b to ec8d1bb Compare February 5, 2024 09:28

fix: compilation error when nested specialization of generic types

49fded7

Signed-off-by: ganjing <[email protected]>

Shanks0224 force-pushed the main branch from ec8d1bb to 49fded7 Compare February 20, 2024 02:37

yviansu reviewed Feb 22, 2024

View reviewed changes

yviansu reviewed Feb 26, 2024

View reviewed changes

Shanks0224 force-pushed the main branch from ae6576a to 0fb80b5 Compare March 6, 2024 03:04

yviansu reviewed Mar 6, 2024

View reviewed changes

Shanks0224 force-pushed the main branch from 0fb80b5 to 66030df Compare March 7, 2024 03:53

Merge branch 'main' into main

6543bb3

Shanks0224 force-pushed the main branch from 66030df to 6543bb3 Compare March 7, 2024 04:15

yviansu approved these changes Mar 8, 2024

View reviewed changes

xujuntwt95329 approved these changes Mar 12, 2024

View reviewed changes

xujuntwt95329 merged commit 8688156 into web-devkits:main Mar 12, 2024
19 checks passed

	visitNode(node: ts.Node): Expression {
	const expr = this.visitNodeInternal(node);
	expr.tsNode = node;
	return expr;
	}

fix: compilation error when nested specialization of generic types #145

fix: compilation error when nested specialization of generic types #145

Conversation

Shanks0224 commented Feb 3, 2024

Shanks0224 commented Feb 3, 2024

xujuntwt95329 commented Feb 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shanks0224 Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shanks0224 Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shanks0224 Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shanks0224 Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yviansu left a comment

Choose a reason for hiding this comment

xujuntwt95329 left a comment

Choose a reason for hiding this comment

xujuntwt95329 commented Mar 12, 2024

Shanks0224 Mar 5, 2024 •

edited

Loading

Shanks0224 Mar 5, 2024 •

edited

Loading

Shanks0224 Mar 5, 2024 •

edited

Loading

Shanks0224 Mar 5, 2024 •

edited

Loading