Skip to content

Set parents during parse, not bind #1251

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

jakebailey
Copy link
Member

When loading files, Program construction needs to look a file's imports and resolve them. This requires parent pointers as determining what to do depends on context like the resolution mode, if it's in a require, etc.

This is a problem because binding can happen concurrently on the same AST as the same time as file loading, since ASTs can be reused between Programs.

We've worked around this by setting parents early in the parser just on the imports (was done in strada too), yet that still isn't enough to make the code not race (#970, #1031, #1224).

A simple way to solve this problem is to just make the parser the one in charge of setting pointers, rather than doing it during bind. This is slower, since the binder is already walking the AST and so usually has thing cached. But, it is a lot simpler to reason about.

Below are benchmarks which show the change; the real time itself is the critical number, showing that while in the checker, we save a millisecond in bind, we pay some 4ms for it during parse, likely due to caching / walking overhead.

If this seems undesirable, there are other fixes that we could try:

  • Instead of collecting specifiers during parse to later resolve, instead collect the imports themselves, then walk down the tree during file loading to get the specifier, never using parent pointers. That avoids the race, but is a more extensive change to get the helpers to not use parents.
  • Force binding to happen immediately after file parse before looking at imports. Realistically, we need every file to have been bound anyway, so this would be "no slower", but would make it a challenge to track how much time was spent in bind.
                                                     │   old.txt   │               new2.txt               │
                                                     │   sec/op    │    sec/op     vs base                │
Bind/empty.ts-20                                       301.2n ± 6%   278.1n ± 32%        ~ (p=0.123 n=10)
Bind/checker.ts-20                                     19.52m ± 2%   18.60m ±  1%   -4.72% (p=0.000 n=10)
Bind/dom.generated.d.ts-20                             8.208m ± 0%   7.142m ±  1%  -12.99% (p=0.000 n=10)
Bind/Herebyfile.mjs-20                                 330.2µ ± 1%   310.5µ ±  0%   -5.95% (p=0.000 n=10)
Bind/jsxComplexSignatureHasApplicabilityError.tsx-20   152.6µ ± 1%   137.3µ ±  1%  -10.03% (p=0.000 n=10)
geomean                                                300.1µ        275.1µ         -8.32%
                                                             │   old.txt   │              new2.txt               │
                                                             │   sec/op    │   sec/op     vs base                │
Parse/empty.ts/tsc-20                                          482.3n ± 1%   518.2n ± 1%   +7.44% (p=0.000 n=10)
Parse/empty.ts/server-20                                       482.3n ± 1%   520.8n ± 1%   +7.99% (p=0.000 n=10)
Parse/checker.ts/tsc-20                                        43.25m ± 1%   48.52m ± 1%  +12.18% (p=0.000 n=10)
Parse/checker.ts/server-20                                     44.46m ± 3%   49.59m ± 2%  +11.54% (p=0.000 n=10)
Parse/dom.generated.d.ts/tsc-20                                15.01m ± 2%   16.15m ± 0%   +7.59% (p=0.000 n=10)
Parse/dom.generated.d.ts/server-20                             22.50m ± 1%   24.12m ± 2%   +7.20% (p=0.000 n=10)
Parse/Herebyfile.mjs/tsc-20                                    820.0µ ± 1%   871.3µ ± 1%   +6.25% (p=0.000 n=10)
Parse/Herebyfile.mjs/server-20                                 814.1µ ± 1%   871.0µ ± 1%   +6.99% (p=0.000 n=10)
Parse/jsxComplexSignatureHasApplicabilityError.tsx/tsc-20      246.9µ ± 2%   261.9µ ± 2%   +6.09% (p=0.000 n=10)
Parse/jsxComplexSignatureHasApplicabilityError.tsx/server-20   383.9µ ± 1%   409.9µ ± 2%   +6.77% (p=0.000 n=10)
geomean                                                        628.1µ        678.3µ        +7.99%

Fixes #1224

@Copilot Copilot AI review requested due to automatic review settings June 20, 2025 22:12
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors how parent pointers are set by moving that responsibility from the binder and test setup to the parser. The key changes include removing redundant calls to ast.SetParentInChildren in various test, binder, and helper files and adding a centralized parent-setting in the parser’s finishSourceFile function.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/testutil/parsetestutil/parsetestutil.go Removed redundant setting of parent pointers in test helper.
internal/printer/printer_test.go Cleaned up unnecessary ast.SetParentInChildren calls in tests.
internal/parser/references.go Removed per-node parent setting in dynamic import callbacks.
internal/parser/parser.go Added centralized setting of parent pointers after parsing.
internal/parser/jsdoc.go Removed redundant parent pointer settings in JSDoc parsing.
internal/execute/tsc.go Removed redundant parent setting in formatter-related code.
internal/binder/binder.go Removed explicit parent-setting calls that are now handled by the parser.
internal/ast/utilities.go Adjusted parent-setting routine to unconditionally assign the parent.
Comments suppressed due to low confidence (2)

internal/binder/binder.go:580

  • The fallback logic that assigns b.parent to node.Parent was removed. Please verify that the parser always sets a valid parent pointer for every node before the binder runs, to avoid potential nil parent references during binding.
	if node == nil {

internal/binder/binder.go:843

  • The explicit call to set the parent for the export clause was removed. Confirm that export declarations have their parent pointers correctly initialized by the parser to ensure that binder operations relying on those pointers function as expected.
	} else if ast.IsNamespaceExport(decl.ExportClause) {

@jakebailey

This comment was marked as outdated.

@jakebailey jakebailey marked this pull request as draft June 20, 2025 22:22
@jakebailey jakebailey marked this pull request as ready for review June 20, 2025 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data race between resolveImportsAndModuleAugmentations and binding
1 participant