Skip to content

Commit 64c72a5

Browse files
committed
Keep SourceFile synchronization state out of accessor copies
SourceFile owns mutable metadata and synchronization primitives even though ordinary AST nodes are value handles. Copying the file for a field access races with concurrent binding and cache initialization, including when the requested arena field itself is stable. The focused regression reproduces the race before the fix. Both exact race-enabled smoke commands and npx hereby validate --all now pass.
1 parent 3ad21be commit 64c72a5

4 files changed

Lines changed: 76 additions & 20 deletions

File tree

‎notes/arena-ast.md‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -107,10 +107,14 @@ metadata operations even when the requested arena field was stable.
107107
The race was reported by the parallel race-enabled smoke job for
108108
[microsoft/TypeScript#64698](https://github.com/microsoft/TypeScript/pull/64698),
109109
[run 37849859691](https://github.com/microsoft/TypeScript/actions/runs/37849859691/job/113559811242).
110-
The required correction is pointer receivers for source-file field accessors,
111-
base conversions, and subtree-fact forwarding. Other concrete/base views should
112-
remain values. Record the implementation and regression evidence here with the
113-
fix.
110+
The fix generates pointer receivers for source-file field accessors, base
111+
conversions, and subtree-fact forwarding. Other concrete/base views remain
112+
values.
113+
114+
Regression coverage checks the public generated method sets and concurrently
115+
reads source-file accessors while its metadata mutex is used. The regression
116+
reproduced the race before the fix. Both exact CI smoke commands pass with a
117+
race-enabled compiler after the fix.
114118

115119
## Performance observations
116120

@@ -165,8 +169,10 @@ therefore also a generator input. Forced regeneration must be deterministic.
165169

166170
The initial migration passed full validation with unchanged compiler baselines,
167171
focused race/checkptr tests, 32-bit tests, and a WASI AST test compilation.
168-
The CI receiver race needs its own regression and race-enabled smoke validation;
169-
the initial focused arena tests did not exercise concurrent metadata copies.
172+
The source-file receiver fix passes its focused race regression, both
173+
race-enabled smoke commands, and `npx hereby validate --all`, with unchanged
174+
compiler baselines. The initial focused arena tests did not exercise concurrent
175+
metadata copies.
170176

171177
Relevant commands:
172178

‎tools/scripts/tsc/go-arena.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -165,13 +165,14 @@ function baseFieldOffset(w: Writer, base: NodeType, field: MemberInfo): string {
165165
}
166166

167167
export function generateArenaView(w: Writer, node: NodeType): void {
168+
const receiver = node.name === "SourceFile" ? "*SourceFile" : node.name;
168169
if (!node.handWritten && node.name !== "NodeBase") {
169170
w.write(`type ${node.name} struct { NodeDefault }`);
170171
}
171172
const fields = arenaFields(node);
172173
for (const [i, field] of fields.entries()) {
173174
const offset = node.isBase ? baseFieldOffset(w, node, field) : String(arenaHeaderWords + i);
174-
emitField(w, node.name, { name: field.name, type: arenaFieldType(field), storage: storageType(field) }, offset);
175+
emitField(w, receiver, { name: field.name, type: arenaFieldType(field), storage: storageType(field) }, offset);
175176
}
176177
const bases = new Set<string>();
177178
const visit = (type: NodeType) => {
@@ -183,14 +184,14 @@ export function generateArenaView(w: Writer, node: NodeType): void {
183184
visit(node);
184185
for (const base of bases) {
185186
if (base === "NodeBase") continue;
186-
w.write(`func (node ${node.name}) ${base}() ${base} { return ${base}{NodeDefault{node.Node}} }`);
187+
w.write(`func (node ${receiver}) ${base}() ${base} { return ${base}{NodeDefault{node.Node}} }`);
187188
}
188189
for (const method of ["computeSubtreeFacts", "propagateSubtreeFacts", "subtreeFactsWorker"]) {
189190
const owner = methodOwner(node, method);
190191
if (owner === node.name || owner === "NodeDefault" || (method === "subtreeFactsWorker" && owner !== "CompositeBase")) continue;
191192
const params = method === "subtreeFactsWorker" ? "self Node" : "";
192193
const args = method === "subtreeFactsWorker" ? "self" : "";
193-
w.write(`func (node ${node.name}) ${method}(${params}) SubtreeFacts { return (${owner}{NodeDefault{node.Node}}).${method}(${args}) }`);
194+
w.write(`func (node ${receiver}) ${method}(${params}) SubtreeFacts { return (${owner}{NodeDefault{node.Node}}).${method}(${args}) }`);
194195
}
195196
w.write("");
196197
}

‎tsc/internal/ast/ast_generated.go‎

Lines changed: 11 additions & 11 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎tsc/internal/ast/nodeaccessors_test.go‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
package ast_test
22

33
import (
4+
"reflect"
45
"runtime"
6+
"sync"
57
"testing"
68

79
"github.com/microsoft/TypeScript/tsc/internal/ast"
@@ -11,6 +13,53 @@ import (
1113
"gotest.tools/v3/assert"
1214
)
1315

16+
func TestSourceFileAccessorsUsePointerReceivers(t *testing.T) {
17+
t.Parallel()
18+
value := reflect.TypeFor[ast.SourceFile]()
19+
pointer := reflect.TypeFor[*ast.SourceFile]()
20+
for _, name := range []string{
21+
"Symbol", "SetSymbol", "Locals", "SetLocals", "NextContainer", "SetNextContainer",
22+
"DeclarationBase", "LocalsContainerBase", "CompositeBase",
23+
} {
24+
if _, ok := value.MethodByName(name); ok {
25+
t.Errorf("%s must not copy SourceFile metadata through a value receiver", name)
26+
}
27+
if _, ok := pointer.MethodByName(name); !ok {
28+
t.Errorf("*SourceFile is missing %s", name)
29+
}
30+
}
31+
}
32+
33+
func TestSourceFileAccessorsConcurrentMetadata(t *testing.T) {
34+
t.Parallel()
35+
factory := ast.NewNodeFactory(ast.NodeFactoryHooks{})
36+
file := factory.NewSourceFile(ast.SourceFileParseOptions{}, "", nil, ast.Node{}).AsSourceFile()
37+
key := ast.NewSourceFileDataKey[int]()
38+
compute := func(*ast.SourceFile) int { return 1 }
39+
file.GetOrComputeData(key, compute)
40+
start := make(chan struct{})
41+
var wait sync.WaitGroup
42+
wait.Go(func() {
43+
<-start
44+
for range 1024 {
45+
assert.Equal(t, file.GetOrComputeData(key, compute), 1)
46+
}
47+
})
48+
wait.Go(func() {
49+
<-start
50+
for range 1024 {
51+
assert.Equal(t, file.Symbol(), (*ast.Symbol)(nil))
52+
assert.Assert(t, file.Locals() == nil)
53+
assert.Assert(t, file.NextContainer().IsNil())
54+
assert.Equal(t, file.DeclarationBase().AsNode(), file.AsNode())
55+
assert.Equal(t, file.LocalsContainerBase().AsNode(), file.AsNode())
56+
assert.Equal(t, file.CompositeBase().AsNode(), file.AsNode())
57+
}
58+
})
59+
close(start)
60+
wait.Wait()
61+
}
62+
1463
func TestNodeAccessorsCastLayout(t *testing.T) {
1564
t.Parallel()
1665
factory := ast.NewNodeFactory(ast.NodeFactoryHooks{})

0 commit comments

Comments
 (0)