Partially reuse type nodes #58516

dragomirtitian · 2024-05-13T10:48:07Z

jakebailey · 2024-05-13T18:53:29Z

tests/baselines/reference/callbackTagNamespace.types

@@ -16,7 +16,7 @@ var x = 1;
 /** @type {NS.Nested.Inner} */
 function f(space, peace) {
 >f : (space: any, peace: any) => string | number
->  : ^     ^^   ^^     ^^   ^^^^^^^^^^^^^^^^^^^^
+>  : ^     ^^^^^^^     ^^^^^^^^^^^^^^^^^^^^^^^^^


How does reuse end up being lowered here?

The original version for JsDoc did fall back to type printing, but it also set the original node on this synthetic type node counting it as a reuse. Falling back to type printing should not count the node as reused in my opinion.

if (isTypeReferenceNode(node) && isInJSDoc(node) && (!existingTypeNodeIsNotReferenceOrIsReferenceWithCompatibleTypeArgumentCount(node, getTypeFromTypeNode(node)) || getIntendedTypeFromJSDocTypeReference(node) || unknownSymbol === resolveTypeReferenceName(node, SymbolFlags.Type, /*ignoreErrors*/ true))) { return setOriginalNode(typeToTypeNodeHelper(getTypeFromTypeNode(node), context), node); }

src/compiler/checker.ts

weswigham · 2024-05-13T19:50:15Z

src/compiler/checker.ts

+        function serializeTypeName(context: NodeBuilderContext, node: EntityName, isTypeOf?: boolean, typeArguments?: readonly TypeNode[]) {
+            const meaning = isTypeOf ? SymbolFlags.Value : SymbolFlags.Type;
+            const symbol = resolveEntityName(node, meaning, /*ignoreErrors*/ true);
+            if (!symbol) return undefined;


The default behavior here is to fall back to serializing the type node when the name doesn't resolve, and usually I'm inclined to approve of that approach, but the transpileDeclaration issues we've been getting have made me keenly aware that most names won't resolve when transpiled under that mode, and you probably want the default behavior to be copy-the-node when the name doesn't resolve rather than print-any (at least if the appropriate node builder context flag is set). This is somewhat reasonable since name-that-does-not-resolve is just any with more steps to get there.

This actually isn't an issue the way this function is currently used. trackExistingEntityName will already not error if the symbol is not found and will just return the original name. serializeTypeName is always used as a fallback, when trackExistingEntityName fails so it will never be called if the symbol is not resolvable.

I added a test that shows that the types are reused even if the symbol is not found.

src/compiler/checker.ts

weswigham · 2024-05-13T20:17:25Z

@dragomirtitian Do you also wanna copy your tests to the tests/cases/transpile folder to run them through the transpileDeclaration API endpoint, to ensure the output is actually what you want when global/other file context isn't present? That's particularly relevant for some of these examples that generate import type nodes (whose generation would seem to rely on the symbol actually resolving).

dragomirtitian · 2024-05-14T16:30:08Z

@dragomirtitian Do you also wanna copy your tests to the tests/cases/transpile folder to run them through the transpileDeclaration API endpoint, to ensure the output is actually what you want when global/other file context isn't present? That's particularly relevant for some of these examples that generate import type nodes (whose generation would seem to rely on the symbol actually resolving).

I can copy them, but these tests are not actually isolated declarations compliant and I would not expect these changes to actually work with transpileDeclaration. The use of import types and symbol resolution during node reuse will only work with the full type checker. This will only really kick in when a declaration is reused in a new location, and this should never happen with isolated declarations as if the type is not specified you get an error in isolated declarations.

weswigham · 2024-05-14T18:30:25Z

I can copy them, but these tests are not actually isolated declarations compliant and I would not expect these changes to actually work with transpileDeclaration.

Yeah, I would just suggest the test be added to ensure there's still an --isolatedDeclarations error issued by the declaration emitter for these cases, though, and that the error itself isn't dependent on symbol or type information being available to be issued.

weswigham

Thanks for the extra tests - from the issues we've been getting in thus far, it seems like it's probably a good idea going forward to test both the full typecheck behavior and the transpileDeclaration behavior for anything isolatedDeclarations-adjacent, so we know the two are consistent.

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 13, 2024

jakebailey reviewed May 13, 2024

View reviewed changes

jakebailey requested review from weswigham and jakebailey May 13, 2024 18:54

weswigham reviewed May 13, 2024

View reviewed changes