Use union types for all common classes of AST nodes #54148

weswigham · 2023-05-05T07:17:38Z

Like Node, Expression, TypeNode, and the like.

This allows us to make many more switch statements in our codebase exhaustive (a change not included in this PR, which just swaps over to unions and fixes resulting errors/lints).

This is like the Compiler-Unions perf test, except:

Totally complete, rather than just the compiler folder.
Totally up-to-date (this comment will age well 😂).

Incremental rebuilds are a rebuild-accelerating thing now, so I think this is more palatable nowadays (because a fresh build does still take about 2.7x longer than main today does). Especially since using unions for all these types has tangible style and safety advantages (no more casts everywhere!). Immediately, this found some dead code (which is now gone) and some incorrect types (which are now fixed). In addition to the ergonomic niceties of using unions like this, I do think using them more like this would help us dogfood our own typesystem more in scenarios we consider more "extreme", since we have 363 different node kinds now (and usually growing), so the Node type is what we should consider a "reasonable" but large type.

There are a handful of places where I had to introduce some new casts to work around some bogus complexity errors (opened #54146 to track the issue causing it, and flagged the offending locations), but other than that this actually works pretty well without significant refactoring (other than automatically removing a ton of useless casts). The big question is really just if we, as a team, think the tradeoffs are worth it at this point or not.

jakebailey · 2023-05-05T15:21:40Z

src/compiler/binder.ts

@@ -649,7 +639,7 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void {
    // unless it is a well known Symbol.
    function getDeclarationName(node: Declaration): __String | undefined {
        if (node.kind === SyntaxKind.ExportAssignment) {
-            return (node as ExportAssignment).isExportEquals ? InternalSymbolName.ExportEquals : InternalSymbolName.Default;
+            return (node).isExportEquals ? InternalSymbolName.ExportEquals : InternalSymbolName.Default;


Scrolling though the diff but noticed this leftover parens; not sure if it's also leftover anywhere else.

(Formatter when)

Yeah, it looks like the eslint --fix command left behind a lot of unneeded parenthesis (and some whitespace in other places!), and we have no other lint rule forbidding them.

@JoshuaKGoldberg

There are more than a few others in this file (L819, L1208, etc.)

Yup, @typescript-eslint/no-unnecessary-type-assertion follows in our standard practice of not trying to adjust so much for formatting. https://typescript-eslint.io/linting/troubleshooting/formatting

(I realize you all know the differences between the tools - just posting the page for context around why we consider this not a bug in the rule)

Looking forward to TypeScript using dprint 😄

Personally, I wouldn't call this a formatting concern though. Extra parenthesis are rarely something that people do on purpose and, in here, the original code required those parenthesis not for formatting purposes. I think it would be reasonable to strip those away while fixing as the original intent is pretty clear here.

jakebailey · 2023-05-05T16:14:07Z

Incremental rebuilds are a rebuild-accelerating thing now, so I think this is more palatable nowadays (because a fresh build does still take about 2.7x longer than main today does).

Definitely curious to do a differential profile on this one.

weswigham · 2023-05-05T16:51:36Z

src/compiler/binder.ts

@@ -374,7 +364,7 @@ function getModuleInstanceStateWorker(node: Node, visited: Map<number, ModuleIns
            break;
        // 4. Export alias declarations pointing at only uninstantiated modules or things uninstantiated modules contain
        case SyntaxKind.ExportDeclaration:
-            const exportDeclaration = node as ExportDeclaration;
+            const exportDeclaration = node ;


See this whitespace? This is almost certainly a bug in the eslint rule's --fix behavior. What's depressing is how many instances aren't caught by any of the whitespace-checking rules we have on, thoguh.

@JoshuaKGoldberg my hero

#54148 (comment) 🤝 ❤️

rbuckton

I'm still working through my review, but there are quite a few cases of excess parens and excess whitespace left over after this change. There also appear to be a number of potentially unnecessary type assertions remaining, so it might be worthwhile to look into usages of ForInOrOfStatement, AssertionExpression, BreakOrContinueStatement, AccessorDeclaration, and a few others as well.

rbuckton · 2023-05-05T16:59:40Z

src/compiler/binder.ts

                break;
            case SyntaxKind.VariableDeclaration:
-                bindVariableDeclarationFlow(node as VariableDeclaration);
+                bindVariableDeclarationFlow(node);
                break;
            case SyntaxKind.PropertyAccessExpression:
            case SyntaxKind.ElementAccessExpression:
                bindAccessExpressionFlow(node as AccessExpression);


Shouldn't this cast be unnecessary?

Yeah, I'm surprised it wasn't removed. Maybe because the lint rules uses object equality, and the aliased form of the union isn't === to the non-aliased form of the union?

Yes, that's exactly it:

https://github.com/typescript-eslint/typescript-eslint/blob/09c04fb0d5a5dee0dc266abb2e21aa9a1c489712/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts#L260

We're blocked on making more informed decisions on the existence of a public type assignability API: #9879. In theory we could try to be smart about breaking apart unions, but if we went down that rabbit hole it'd eventually end up with us reimplementing much of TypeScript's native type assignability. We've drawn the line at just === checking.

(We have so many great feature requests on the typescript-eslint side blocked on that issue, it's nice to also add a link to it in TypeScript itself 😄)

rbuckton · 2023-05-05T17:00:36Z

src/compiler/binder.ts

                break;
            case SyntaxKind.ReturnStatement:
            case SyntaxKind.ThrowStatement:
-                bindReturnOrThrow(node as ReturnStatement | ThrowStatement);
+                bindReturnOrThrow(node);
                break;
            case SyntaxKind.BreakStatement:
            case SyntaxKind.ContinueStatement:
                bindBreakOrContinueStatement(node as BreakOrContinueStatement);


Shouldn't this cast be unnecessary?

#54148 (comment)

(here and the other comments like this one)

rbuckton · 2023-05-05T17:00:51Z

src/compiler/binder.ts

                break;
            case SyntaxKind.ForStatement:
-                bindForStatement(node as ForStatement);
+                bindForStatement(node);
                break;
            case SyntaxKind.ForInStatement:
            case SyntaxKind.ForOfStatement:
                bindForInOrForOfStatement(node as ForInOrOfStatement);


Shouldn't this cast be unnecessary?

rbuckton · 2023-05-05T17:01:35Z

src/compiler/binder.ts

            case SyntaxKind.ParenthesizedExpression:
            case SyntaxKind.NonNullExpression:
-                return isNarrowingExpression((expr as ParenthesizedExpression | NonNullExpression).expression);
+                return isNarrowingExpression((expr).expression);


Excess parens.

rbuckton · 2023-05-05T17:01:41Z

src/compiler/binder.ts

            case SyntaxKind.PrefixUnaryExpression:
-                return (expr as PrefixUnaryExpression).operator === SyntaxKind.ExclamationToken && isNarrowingExpression((expr as PrefixUnaryExpression).operand);
+                return (expr).operator === SyntaxKind.ExclamationToken && isNarrowingExpression((expr).operand);


Excess parens.

rbuckton · 2023-05-05T17:21:57Z

src/compiler/emitter.ts

@@ -2170,11 +2169,11 @@ export function createPrinter(printerOptions: PrinterOptions = {}, handlers: Pri
                case SyntaxKind.JSDocSatisfiesTag:
                    return emitJSDocSimpleTypedTag(node as JSDocTypeTag | JSDocReturnTag | JSDocThisTag | JSDocTypeTag | JSDocThrowsTag | JSDocSatisfiesTag);


Is this cast still necessary?

rbuckton · 2023-05-05T17:24:17Z

src/compiler/factory/parenthesizerRules.ts

@@ -252,14 +251,14 @@ export function createParenthesizerRules(factory: NodeFactory): ParenthesizerRul
            return node.kind;
        }

-        if (node.kind === SyntaxKind.BinaryExpression && (node as BinaryExpression).operatorToken.kind === SyntaxKind.PlusToken) {
+        if (node.kind === SyntaxKind.BinaryExpression && (node).operatorToken.kind === SyntaxKind.PlusToken) {


Excess parens.

rbuckton · 2023-05-05T17:27:24Z

src/compiler/factory/nodeFactory.ts

@@ -7570,6 +7583,9 @@ export function createSourceMapSource(fileName: string, text: string, skipTrivia

 // Utilities

+/** @internal */ export function setOriginalNode<T extends Node>(node: Mutable<T>, original: Node | undefined): T; // TODO: This should probably always require a Mutable<T>, since it mutates `original`


The TODO isn't relevant since original isn't marked readonly on Node.

rbuckton · 2023-05-05T17:29:22Z

src/compiler/factory/nodeFactory.ts

@@ -7570,6 +7583,9 @@ export function createSourceMapSource(fileName: string, text: string, skipTrivia

 // Utilities

+/** @internal */ export function setOriginalNode<T extends Node>(node: Mutable<T>, original: Node | undefined): T; // TODO: This should probably always require a Mutable<T>, since it mutates `original`
+// eslint-disable-next-line @typescript-eslint/unified-signatures
+export function setOriginalNode<T extends Node>(node: T, original: Node | undefined): T; // inference is worse if the signatures are combined


Does this comment show up in the public API? It would be somewhat confusing if so, since the other overload isn't public.

rbuckton · 2023-05-05T17:31:12Z

src/compiler/parser.ts

@@ -10124,7 +10126,8 @@ namespace IncrementalParser {
        _children: Node[] | undefined;
    }

-    export interface IncrementalNode extends Node, IncrementalElement {
+    export type IncrementalNode = Node & IncrementalNodeBase;
+    export interface IncrementalNodeBase extends NodeBase, IncrementalElement {


Not for this PR, but this interface smells like megamorphism and we should probably find a different way to mark incrementally parsed nodes. I'm not sure how many other cases of "convert this interface to an intersection to add an extra property" there are, but this PR is surfacing polymorphic assignment. We should probably add comments to these cases so we can identify them later.

Separately, we might want to consider a ts-eslint rule that warns on any intersection with Node or its subtypes that introduces net-new properties.

Yeah, I thought the same thing when making the change. Luckily it wasn't that common.

gabritto · 2023-05-05T19:33:15Z

because a fresh build does still take about 2.7x longer than main today does

What's the impact on an editor scenario? Say, editing checker.ts. It's already pretty bad today (at least for me), and though I don't care about a fresh build being 3x as slow, I do care about editor perf.

jakebailey · 2023-05-05T23:36:24Z

A quick glance at a profile for this shows a bunch of suspicious new calls for getResolvedSignature which spend a lot of time in inferTypeArguments (a full second each time it happens). Seems like maybe there's one problematic function?

The 2.7x slowdown is definitely there, though.

pprof-time-92423.pb.gz

pprof-time-92885.pb.gz

jakebailey · 2023-05-05T23:45:07Z

I did a quick debug print to show which of these getResolvedSignatures took more than 500ms, and they are these expressions:

new (TokenConstructor || (TokenConstructor = objectAllocator.getTokenConstructor()))(kind, /*pos*/ -1, /*end*/ -1)
createToken(value)
new (TokenConstructor || (TokenConstructor = objectAllocator.getTokenConstructor()))(kind, -1, -1)
parseOptionalToken(t)

Ouch:

Also, editing in this branch unfortunately feels quite slow. Especially if you're me and have eslint running :(

JoshuaKGoldberg · 2023-05-06T03:16:53Z

src/compiler/factory/nodeFactory.ts

@@ -7570,6 +7583,9 @@ export function createSourceMapSource(fileName: string, text: string, skipTrivia

 // Utilities

+/** @internal */ export function setOriginalNode<T extends Node>(node: Mutable<T>, original: Node | undefined): T; // TODO: This should probably always require a Mutable<T>, since it mutates `original`
+// eslint-disable-next-line @typescript-eslint/unified-signatures


Is there a bugfix or feature request you'd want to file on typescript-eslint to make unified-signatures not complain on this case?

Use union types for all common classes of AST nodes

6ce78bc

weswigham requested review from sandersn, DanielRosenwasser, rbuckton, ahejlsberg, jakebailey and gabritto May 5, 2023 07:17

typescript-bot assigned weswigham May 5, 2023

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 5, 2023

jakebailey reviewed May 5, 2023

View reviewed changes

weswigham commented May 5, 2023

View reviewed changes

rbuckton requested changes May 5, 2023

View reviewed changes

JoshuaKGoldberg reviewed May 6, 2023

View reviewed changes

JoshuaKGoldberg mentioned this pull request Oct 11, 2023

Proposal: Type Relationship API #9879

Open

Andarist mentioned this pull request Oct 31, 2023

Replace Node interface with discriminated unions #56275

Open

5 tasks

jakebailey mentioned this pull request Nov 1, 2023

[Reference] feat: switched Node to a discriminated union #56274

Closed

jakebailey mentioned this pull request Feb 22, 2024

Infer type predicates from function bodies using control flow analysis #57465

Merged

danvk mentioned this pull request Feb 26, 2024

Single-expression only type predicates #57552

Closed

danvk mentioned this pull request Mar 6, 2024

Experimental variant on #57465 with two optimizations #57660

Closed

weswigham added the Experiment A fork with an experimental idea which might not make it into master label Aug 13, 2024

weswigham closed this Aug 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use union types for all common classes of AST nodes #54148

Use union types for all common classes of AST nodes #54148

weswigham commented May 5, 2023

jakebailey May 5, 2023

weswigham May 5, 2023

jakebailey May 5, 2023

rbuckton May 5, 2023

JoshuaKGoldberg May 5, 2023 •

edited

Loading

Andarist May 6, 2023

jakebailey commented May 5, 2023

weswigham May 5, 2023

jakebailey May 5, 2023

JoshuaKGoldberg May 5, 2023

rbuckton left a comment

rbuckton May 5, 2023

weswigham May 5, 2023

JoshuaKGoldberg May 6, 2023 •

edited

Loading

rbuckton May 5, 2023

JoshuaKGoldberg May 6, 2023

rbuckton May 5, 2023

rbuckton May 5, 2023

rbuckton May 5, 2023

rbuckton May 5, 2023

rbuckton May 5, 2023

rbuckton May 5, 2023

rbuckton May 5, 2023

rbuckton May 5, 2023

weswigham May 5, 2023

gabritto commented May 5, 2023

jakebailey commented May 5, 2023 •

edited

Loading

jakebailey commented May 5, 2023 •

edited

Loading

JoshuaKGoldberg May 6, 2023

		@@ -2170,11 +2169,11 @@ export function createPrinter(printerOptions: PrinterOptions = {}, handlers: Pri
		case SyntaxKind.JSDocSatisfiesTag:
		return emitJSDocSimpleTypedTag(node as JSDocTypeTag \| JSDocReturnTag \| JSDocThisTag \| JSDocTypeTag \| JSDocThrowsTag \| JSDocSatisfiesTag);

		@@ -7570,6 +7583,9 @@ export function createSourceMapSource(fileName: string, text: string, skipTrivia

		// Utilities

		/** @internal */ export function setOriginalNode<T extends Node>(node: Mutable<T>, original: Node \| undefined): T; // TODO: This should probably always require a Mutable<T>, since it mutates `original`

Use union types for all common classes of AST nodes #54148

Use union types for all common classes of AST nodes #54148

Conversation

weswigham commented May 5, 2023

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

JoshuaKGoldberg May 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakebailey commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuckton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg May 6, 2023 • 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

gabritto commented May 5, 2023

jakebailey commented May 5, 2023 • edited Loading

jakebailey commented May 5, 2023 • edited Loading

Choose a reason for hiding this comment

JoshuaKGoldberg May 5, 2023 •

edited

Loading

JoshuaKGoldberg May 6, 2023 •

edited

Loading

jakebailey commented May 5, 2023 •

edited

Loading

jakebailey commented May 5, 2023 •

edited

Loading