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.

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

Jump to bottom

Node builder reuses type predicate nodes, more often reuses return position nodes #57990

Merged
merged 1 commit into from
Mar 28, 2024
Merged

Node builder reuses type predicate nodes, more often reuses return position nodes #57990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
89 changes: 53 additions & 36 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,6 +855,7 @@ import {
ModuleKind,
ModuleResolutionKind,
ModuleSpecifierResolutionHost,
Mutable,
NamedDeclaration,
NamedExports,
NamedImportsOrExports,
Expand Down Expand Up @@ -6469,6 +6470,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
typePredicateToTypePredicateNode: (typePredicate: TypePredicate, enclosingDeclaration?: Node, flags?: NodeBuilderFlags, tracker?: SymbolTracker) => withContext(enclosingDeclaration, flags, tracker, context => typePredicateToTypePredicateNodeHelper(typePredicate, context)),
expressionOrTypeToTypeNode: (expr: Expression | JsxAttributeValue | undefined, type: Type, addUndefined?: boolean, enclosingDeclaration?: Node, flags?: NodeBuilderFlags, tracker?: SymbolTracker) => withContext(enclosingDeclaration, flags, tracker, context => expressionOrTypeToTypeNode(context, expr, type, addUndefined)),
serializeTypeForDeclaration: (type: Type, symbol: Symbol, addUndefined?: boolean, enclosingDeclaration?: Node, flags?: NodeBuilderFlags, tracker?: SymbolTracker) => withContext(enclosingDeclaration, flags, tracker, context => serializeTypeForDeclaration(context, type, symbol, enclosingDeclaration, /*includePrivateSymbol*/ undefined, /*bundled*/ undefined, addUndefined)),
serializeReturnTypeForSignature: (signature: Signature, enclosingDeclaration?: Node, flags?: NodeBuilderFlags, tracker?: SymbolTracker) => withContext(enclosingDeclaration, flags, tracker, context => serializeReturnTypeForSignature(context, signature)),
indexInfoToIndexSignatureDeclaration: (indexInfo: IndexInfo, enclosingDeclaration?: Node, flags?: NodeBuilderFlags, tracker?: SymbolTracker) => withContext(enclosingDeclaration, flags, tracker, context => indexInfoToIndexSignatureDeclarationHelper(indexInfo, context, /*typeNode*/ undefined)),
signatureToSignatureDeclaration: (signature: Signature, kind: SignatureDeclaration["kind"], enclosingDeclaration?: Node, flags?: NodeBuilderFlags, tracker?: SymbolTracker) => withContext(enclosingDeclaration, flags, tracker, context => signatureToSignatureDeclarationHelper(signature, kind, context)),
symbolToEntityName: (symbol: Symbol, meaning: SymbolFlags, enclosingDeclaration?: Node, flags?: NodeBuilderFlags, tracker?: SymbolTracker) => withContext(enclosingDeclaration, flags, tracker, context => symbolToName(symbol, context, meaning, /*expectsIdentifier*/ false)),
Expand Down Expand Up @@ -7649,8 +7651,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

function signatureToSignatureDeclarationHelper(signature: Signature, kind: SignatureDeclaration["kind"], context: NodeBuilderContext, options?: SignatureToSignatureDeclarationOptions): SignatureDeclaration {
const suppressAny = context.flags & NodeBuilderFlags.SuppressAnyReturnType;
if (suppressAny) context.flags &= ~NodeBuilderFlags.SuppressAnyReturnType; // suppress only toplevel `any`s
const flags = context.flags;
context.flags &= ~NodeBuilderFlags.SuppressAnyReturnType; // SuppressAnyReturnType should only apply to the signature `return` position
context.approximateLength += 3; // Usually a signature contributes a few more characters than this, but 3 is the minimum
let typeParameters: TypeParameterDeclaration[] | undefined;
let typeArguments: TypeNode[] | undefined;
Expand Down Expand Up @@ -7780,21 +7782,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
if (thisParameter) {
parameters.unshift(thisParameter);
}
context.flags = flags;
jakebailey marked this conversation as resolved.
Show resolved Hide resolved

const returnTypeNode = serializeReturnTypeForSignature(context, signature, options?.privateSymbolVisitor, options?.bundledImports);

let returnTypeNode: TypeNode | undefined;
const typePredicate = getTypePredicateOfSignature(signature);
if (typePredicate) {
returnTypeNode = typePredicateToTypePredicateNodeHelper(typePredicate, context);
}
else {
const returnType = getReturnTypeOfSignature(signature);
if (returnType && !(suppressAny && isTypeAny(returnType))) {
returnTypeNode = serializeReturnTypeForSignature(context, returnType, signature, options?.privateSymbolVisitor, options?.bundledImports);
}
else if (!suppressAny) {
returnTypeNode = factory.createKeywordTypeNode(SyntaxKind.AnyKeyword);
}
}
let modifiers = options?.modifiers;
if ((kind === SyntaxKind.ConstructorType) && signature.flags & SignatureFlags.Abstract) {
const flags = modifiersToFlags(modifiers);
Expand Down Expand Up @@ -8572,7 +8563,25 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return false;
}

function serializeReturnTypeForSignature(context: NodeBuilderContext, type: Type, signature: Signature, includePrivateSymbol?: (s: Symbol) => void, bundled?: boolean) {
function serializeReturnTypeForSignature(context: NodeBuilderContext, signature: Signature, includePrivateSymbol?: (s: Symbol) => void, bundled?: boolean) {
const suppressAny = context.flags & NodeBuilderFlags.SuppressAnyReturnType;
const flags = context.flags;
if (suppressAny) context.flags &= ~NodeBuilderFlags.SuppressAnyReturnType; // suppress only toplevel `any`s
let returnTypeNode: TypeNode | undefined;
const returnType = getReturnTypeOfSignature(signature);
if (returnType && !(suppressAny && isTypeAny(returnType))) {
returnTypeNode = serializeReturnTypeForSignatureWorker(context, signature, includePrivateSymbol, bundled);
}
else if (!suppressAny) {
returnTypeNode = factory.createKeywordTypeNode(SyntaxKind.AnyKeyword);
}
context.flags = flags;
return returnTypeNode;
}

function serializeReturnTypeForSignatureWorker(context: NodeBuilderContext, signature: Signature, includePrivateSymbol?: (s: Symbol) => void, bundled?: boolean) {
const typePredicate = getTypePredicateOfSignature(signature);
const type = getReturnTypeOfSignature(signature);
if (!isErrorType(type) && context.enclosingDeclaration) {
const annotation = signature.declaration && getEffectiveReturnTypeNode(signature.declaration);
const enclosingDeclarationIgnoringFakeScope = getEnclosingDeclarationIgnoringFakeScope(context.enclosingDeclaration);
Expand All @@ -8585,7 +8594,11 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
}
}
return typeToTypeNodeHelper(type, context);
if (typePredicate) {
return typePredicateToTypePredicateNodeHelper(typePredicate, context);
}
const expr = signature.declaration && getPossibleTypeNodeReuseExpression(signature.declaration);
return expressionOrTypeToTypeNode(context, expr, type);
}

function trackExistingEntityName<T extends EntityNameOrEntityNameExpression>(node: T, context: NodeBuilderContext, includePrivateSymbol?: (s: Symbol) => void) {
Expand Down Expand Up @@ -8633,7 +8646,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
return transformed === existing ? setTextRange(factory.cloneNode(existing), existing) : transformed;

function visitExistingNodeTreeSymbols(node: Node): Node {
function visitExistingNodeTreeSymbols(node: Node): Node | undefined {
// We don't _actually_ support jsdoc namepath types, emit `any` instead
if (isJSDocAllType(node) || node.kind === SyntaxKind.JSDocNamepathType) {
return factory.createKeywordTypeNode(SyntaxKind.AnyKeyword);
Expand All @@ -8642,16 +8655,16 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return factory.createKeywordTypeNode(SyntaxKind.UnknownKeyword);
}
if (isJSDocNullableType(node)) {
return factory.createUnionTypeNode([visitNode(node.type, visitExistingNodeTreeSymbols, isTypeNode), factory.createLiteralTypeNode(factory.createNull())]);
return factory.createUnionTypeNode([visitNode(node.type, visitExistingNodeTreeSymbols, isTypeNode)!, factory.createLiteralTypeNode(factory.createNull())]);
}
if (isJSDocOptionalType(node)) {
return factory.createUnionTypeNode([visitNode(node.type, visitExistingNodeTreeSymbols, isTypeNode), factory.createKeywordTypeNode(SyntaxKind.UndefinedKeyword)]);
return factory.createUnionTypeNode([visitNode(node.type, visitExistingNodeTreeSymbols, isTypeNode)!, factory.createKeywordTypeNode(SyntaxKind.UndefinedKeyword)]);
}
if (isJSDocNonNullableType(node)) {
return visitNode(node.type, visitExistingNodeTreeSymbols);
}
if (isJSDocVariadicType(node)) {
return factory.createArrayTypeNode(visitNode(node.type, visitExistingNodeTreeSymbols, isTypeNode));
return factory.createArrayTypeNode(visitNode(node.type, visitExistingNodeTreeSymbols, isTypeNode)!);
}
if (isJSDocTypeLiteral(node)) {
return factory.createTypeLiteralNode(map(node.jsDocPropertyTags, t => {
Expand Down Expand Up @@ -8743,15 +8756,24 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
node.isTypeOf,
);
}
if (isParameter(node)) {
if (!node.type && !node.initializer) {
return factory.updateParameterDeclaration(node, /*modifiers*/ undefined, node.dotDotDotToken, visitEachChild(node.name, visitExistingNodeTreeSymbols, /*context*/ undefined), node.questionToken, factory.createKeywordTypeNode(SyntaxKind.AnyKeyword), /*initializer*/ undefined);
}
if (isNamedDeclaration(node) && node.name.kind === SyntaxKind.ComputedPropertyName && !isLateBindableName(node.name)) {
return undefined;
Copy link
Member

Choose a reason for hiding this comment

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

This is the only new return of undefined; is there a way to avoid it to prevent all of the extra !?

Copy link
Member Author

Choose a reason for hiding this comment

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

...for now. There will be more.

Copy link
Member Author

Choose a reason for hiding this comment

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

(Right now the node builder's existing node tree reuse is somewhat poor at eliding erroneous nodes from the input - this is something that the top-level declaration emitter currently does more consistently than this. Honestly, ideally this function and visitDeclarationSubtree in the declaration emitter would be largely shared (since they do the same thing: walk an existing node tree and make it fit to emit), but unifying them is going to take a bit - one needs to subsume the other's feature set entirely first, rather than them having overlap, but also unique parts)

}
if (isPropertySignature(node)) {
if (!node.type && !node.initializer) {
return factory.updatePropertySignature(node, node.modifiers, node.name, node.questionToken, factory.createKeywordTypeNode(SyntaxKind.AnyKeyword));
if (
(isFunctionLike(node) && !node.type)
|| (isPropertyDeclaration(node) && !node.type && !node.initializer)
|| (isPropertySignature(node) && !node.type && !node.initializer)
|| (isParameter(node) && !node.type && !node.initializer)
) {
let visited = visitEachChild(node, visitExistingNodeTreeSymbols, /*context*/ undefined);
if (visited === node) {
visited = setTextRange(factory.cloneNode(node), node);
}
(visited as Mutable<typeof visited>).type = factory.createKeywordTypeNode(SyntaxKind.AnyKeyword);
if (isParameter(node)) {
(visited as Mutable<ParameterDeclaration>).modifiers = undefined;
Comment on lines +8772 to +8774
Copy link
Member

Choose a reason for hiding this comment

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

Are these guaranteed to be copies? This is the first time we're pulling Mutable into the checker and it feels odd to have to do this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - right above we say "if it's not a copy, copy it so we can add a .type safely".

Honestly, this is just the bare minimum needed to keep the tests looking correct while reusing the current set of input nodes. As I mentioned in the last comment, this scenario, of transforming the node to no longer have some kinds of error states in them (erroneous modifiers, missing required type nodes), this function is actually pretty lacking in compared to the declaration emitter's visitDeclarationSubtree which is its' dual. It needs a proper cleanup in that regard, but I don't think we're quite ready for that just yet - testing it won't be easy until we're actually reusing way more nodes via the node builder (which is the plan!).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I guess I didn't read that code that way; wasn't sure if one could return an entirely different but not synthesized node. But, makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

For additional context: visitDeclarationSubtree in declarations.ts and tryReuseExistingTypeNodeHelper in checker.ts have the same goal: take in a type node (or part thereof), and make it fit for emit, per our declaration file rules. Except tryReuseExistingTypeNodeHelper has only really seen extensive use for JS emit - so today it mostly just copies JSDoc type nodes and transforms them, while also doing some reference and visibility tracking. Meanwhile visitDeclarationSubtree doesn't handle JSDoc at all, but beacuse it's been in use longer for declaration emit, handles more edge cases around correcting erroneous input patterns when building the output (and also does some of that same reference tracking).

Eventually we'll merge the two into one implementation used in both places (ideally), but until then, I'm just fixing the things tryReuseExistingTypeNodeHelper is obviously demonstrated as lacking when it starts returning the input node in our tests.

}
return visited;
}

if (isEntityName(node) || isEntityNameExpression(node)) {
Expand Down Expand Up @@ -48664,6 +48686,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {

type DeclarationWithPotentialInnerNodeReuse =
| SignatureDeclaration
| JSDocSignature
| AccessorDeclaration
| VariableLikeDeclaration
| PropertyAccessExpression
Expand Down Expand Up @@ -48712,13 +48735,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
if (!signatureDeclaration) {
return factory.createToken(SyntaxKind.AnyKeyword) as KeywordTypeNode;
}
const signature = getSignatureFromDeclaration(signatureDeclaration);
const typePredicate = getTypePredicateOfSignature(signature);
if (typePredicate) {
// Inferred type predicates
return nodeBuilder.typePredicateToTypePredicateNode(typePredicate, enclosingDeclaration, flags | NodeBuilderFlags.MultilineObjectLiterals, tracker);
}
return nodeBuilder.expressionOrTypeToTypeNode(getPossibleTypeNodeReuseExpression(signatureDeclaration), getReturnTypeOfSignature(signature), /*addUndefined*/ undefined, enclosingDeclaration, flags | NodeBuilderFlags.MultilineObjectLiterals, tracker);
return nodeBuilder.serializeReturnTypeForSignature(getSignatureFromDeclaration(signatureDeclaration), enclosingDeclaration, flags | NodeBuilderFlags.MultilineObjectLiterals, tracker);
}

function createTypeOfExpression(exprIn: Expression, enclosingDeclaration: Node, flags: NodeBuilderFlags, tracker: SymbolTracker) {
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/arrayEvery.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ const foo: (number | string)[] = ['aaa'];

const isString = (x: unknown): x is string => typeof x === 'string';
>isString : (x: unknown) => x is string
> : ^^^^ ^^^^^^^^^^^^^^^^
> : ^^^^ ^^^^^
>(x: unknown): x is string => typeof x === 'string' : (x: unknown) => x is string
> : ^^^^ ^^^^^^^^^^^^^^^^
> : ^^^^ ^^^^^
>x : unknown
> : ^^^^^^^
>typeof x === 'string' : boolean
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/arrayFind.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
// test fix for #18112, type guard predicates should narrow returned element
function isNumber(x: any): x is number {
>isNumber : (x: any) => x is number
> : ^^^^ ^^^^^^^^^^^^^^^^
> : ^^^^ ^^^^^
>x : any

return typeof x === "number";
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/arrowFunctionWithObjectLiteralBody1.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
=== arrowFunctionWithObjectLiteralBody1.ts ===
var v = a => <any>{}
>v : (a: any) => any
> : ^^^^^^^^^^^^^^^
> : ^^^^^^^^^^^^
>a => <any>{} : (a: any) => any
> : ^^^^^^^^^^^^^^^
> : ^^^^^^^^^^^^
>a : any
><any>{} : any
>{} : {}
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/arrowFunctionWithObjectLiteralBody2.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
=== arrowFunctionWithObjectLiteralBody2.ts ===
var v = a => <any><any>{}
>v : (a: any) => any
> : ^^^^^^^^^^^^^^^
> : ^^^^^^^^^^^^
>a => <any><any>{} : (a: any) => any
> : ^^^^^^^^^^^^^^^
> : ^^^^^^^^^^^^
>a : any
><any><any>{} : any
><any>{} : any
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/arrowFunctionWithObjectLiteralBody3.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
=== arrowFunctionWithObjectLiteralBody3.ts ===
var v = a => <any>{}
>v : (a: any) => any
> : ^^^^^^^^^^^^^^^
> : ^^^^^^^^^^^^
>a => <any>{} : (a: any) => any
> : ^^^^^^^^^^^^^^^
> : ^^^^^^^^^^^^
>a : any
><any>{} : any
>{} : {}
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/arrowFunctionWithObjectLiteralBody4.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
=== arrowFunctionWithObjectLiteralBody4.ts ===
var v = a => <any><any>{}
>v : (a: any) => any
> : ^^^^^^^^^^^^^^^
> : ^^^^^^^^^^^^
>a => <any><any>{} : (a: any) => any
> : ^^^^^^^^^^^^^^^
> : ^^^^^^^^^^^^
>a : any
><any><any>{} : any
><any>{} : any
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/arrowFunctionWithObjectLiteralBody5.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
=== arrowFunctionWithObjectLiteralBody5.ts ===
var a = () => <Error>{ name: "foo", message: "bar" };
>a : () => Error
> : ^^^^^^^^^^^
> : ^^^^^^
>() => <Error>{ name: "foo", message: "bar" } : () => Error
> : ^^^^^^^^^^^
> : ^^^^^^
><Error>{ name: "foo", message: "bar" } : Error
> : ^^^^^
>{ name: "foo", message: "bar" } : { name: string; message: string; }
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/arrowFunctionWithObjectLiteralBody6.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
=== arrowFunctionWithObjectLiteralBody6.ts ===
var a = () => <Error>{ name: "foo", message: "bar" };
>a : () => Error
> : ^^^^^^^^^^^
> : ^^^^^^
>() => <Error>{ name: "foo", message: "bar" } : () => Error
> : ^^^^^^^^^^^
> : ^^^^^^
><Error>{ name: "foo", message: "bar" } : Error
> : ^^^^^
>{ name: "foo", message: "bar" } : { name: string; message: string; }
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/assertionFunctionWildcardImport1.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export { Debug };
=== src/core/debug.ts ===
export declare function assert(expression: unknown): asserts expression;
>assert : (expression: unknown) => asserts expression
> : ^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^
> : ^^^^^^^^^^^^^ ^^^^^
>expression : unknown
> : ^^^^^^^

Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/assertionFunctionWildcardImport2.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
=== asserts.ts ===
function isNonNullable<T>(obj: T): asserts obj is NonNullable<T> {
>isNonNullable : <T>(obj: T) => asserts obj is NonNullable<T>
> : ^ ^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> : ^ ^^^^^^^ ^^^^^
>obj : T
> : ^

Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/assertionFunctionsCanNarrowByDiscriminant.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ type Animal = Cat | Dog;

declare function assertEqual<T>(value: any, type: T): asserts value is T;
>assertEqual : <T>(value: any, type: T) => asserts value is T
> : ^ ^^^^^^^^^ ^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^
> : ^ ^^^^^^^^^ ^^^^^^^^ ^^^^^
>value : any
>type : T
> : ^
Expand Down
Loading