Skip to content

Add isTerminal and previousInChain getters to AstNode - #1788

Merged
TwitchBronBron merged 13 commits into
masterfrom
is-terminal
Sep 8, 2026
Merged

Add isTerminal and previousInChain getters to AstNode#1788
TwitchBronBron merged 13 commits into
masterfrom
is-terminal

Conversation

@TwitchBronBron

@TwitchBronBron TwitchBronBron commented Sep 4, 2026

Copy link
Copy Markdown
Member

Expression chains are stored inverted in the AST, so walking .parent can't tell when you've left one expression and landed in an unrelated enclosing one:

print doSomething(a.b)

PrintStatement
└─ CallExpression                  "doSomething(a.b)"
   ├─ callee: VariableExpression   "doSomething"
   └─ args[0]: DottedGetExpression "a.b"     ← its own expression
      └─ obj: VariableExpression   "a"

Walking .parent from a gives a.b, then the CallExpression — but that call isn't part of a.b, it's the thing a.b sits inside.

Each node in a chain spans a whole prefix of the source, so a node's previous step is the next-shorter prefix:

print a.b[2].c(9)
      ─                  a
      ───                a.b
      ──────             a.b[2]
      ────────           a.b[2].c
      ─────────────      a.b[2].c(9)

Which parent property holds the child is what decides membership, not node type. So each chaining node declares its previous step:

// DottedGetExpression
public get previousInChain() { return this.obj; }     // `a.b` -> `a`

// CallExpression
public get previousInChain() { return this.callee; }  // `a.b()` -> `a.b`, not the args

and isTerminal() walks up then back down — if it doesn't land on itself, nothing chains onto it:

public isTerminal(): boolean {
    return this.parent === undefined || this.parent.previousInChain !== this;
}

This is not parent reversed: args and index values have a parent, but are never anything's previousInChain, which is exactly what makes them boundaries.

Two members total. Nodes that chain onto nothing (literals, binary expressions, groupings) inherit previousInChain === undefined and are terminal by default, so boundaries need no code.

print a.b.c(1)
// a.b.c(1)  terminal - the whole expression
// a.b.c     no       - a.b.c(1) chains onto it
// a.b       no       - a.b.c chains onto it
// a         no       - a.b chains onto it
// 1         terminal - an argument, so its own expression

What changed

  • AstNode: previousInChain getter and isTerminal()
  • previousInChain overrides on CallExpression, CallfuncExpression, DottedGetExpression, IndexedGetExpression, XmlAttributeGetExpression, NamespacedVariableNameExpression, NewExpression

Verified — 3108 passing (upstream master is 3084; +24 tests covering each boundary type: call args, index values, groupings, binary/unary operands, array & AA literals, ternary, null-coalescing, template interpolations, set-statement operands). tsc --noEmit and npm run lint clean. Purely additive against master.

TwitchBronBron and others added 13 commits September 4, 2026 13:20
Expression chains like `alpha.beta.charlie` are stored inverted in the AST,
and walking `.parent` can't tell when you've left one chain and landed in an
unrelated enclosing expression: in `doSomething(alpha.beta)`, walking up from
`alpha` reaches the CallExpression even though `alpha.beta` is only an argument.

Chain membership is determined by which parent property holds the child, so
each chain-forming node now declares its single chain link via `chainChild`.
`chainParent` returns the parent only when this node is that parent's chain
link, which gives the boundary-aware walks (`getChainEnd`, `getChainStart`,
`getChain`) and the `isTerminal`/`isChainStart` predicates.

Boundaries need no code - the base returns undefined, so groupings, binary
operands, literals, call args and indexes stop a chain by default. That also
replaces the hardcoded `isTerminal` overrides, including the one on Statement.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts:
#	src/parser/AstNode.spec.ts
#	src/parser/AstNode.ts
The chain traversal helpers (chainParent, isChainStart, getChainEnd,
getChainStart, getChain) had no callers outside their own tests. Drop them and
keep the two members that answer the isTerminal question: `chainChild`, which
each wrapping node declares, and `isTerminal()`, which reads it off the parent.

Also rewrite the doc comments to lead with a concrete example rather than
prose.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The merge silently kept the stale side of several auto-resolved files, which
reverted upstream's findAncestor inference cleanup (#1785): the type-guard
overload signatures on AstNode.findAncestor, its test suite, and the now-
redundant explicit type arguments across 13 other files.

Rebuild AstNode.ts and AstNode.spec.ts from origin/master and re-apply only
the chain additions, so the branch is now purely additive against master.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Behavior is unchanged; `parent?.chainChild !== this` already returned true for
a parentless node. Naming that branch explicitly makes it clear the case is
deliberate rather than a side effect of optional chaining.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`chainChild` described the AST direction but read backwards: in `a.b.c`, the
"child" is `a.b`, which sits to the left in source and reads like a parent.
`previousInChain` is unambiguous, since each node spans a whole prefix and this
walks to the next-shorter one.

Also reword the docs around that framing, and note explicitly that this is not
`parent` reversed - args and index values have a parent but are never a
previousInChain.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`print a.b.c(1)` made the argument row hard to read, since the `1` in the
listing and the `(1)` in the source line look like the same thing. Use `arg`
for arguments and `i` for index values so each row names one distinct role,
and show that a longer argument is terminal too.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This test timed out at 20s on the macos CI runner. It spawns a worker thread
and talks to it over a socket, which is the same cold-boot cost the other
worker-thread tests in this file already budget 60s for, so match them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@TwitchBronBron TwitchBronBron changed the title Add expression chain traversal to AstNode Add isTerminal and previousInChain getters to AstNode Sep 8, 2026
@TwitchBronBron
TwitchBronBron enabled auto-merge (squash) September 8, 2026 17:56
@TwitchBronBron
TwitchBronBron merged commit 97a0f40 into master Sep 8, 2026
10 checks passed
@TwitchBronBron
TwitchBronBron deleted the is-terminal branch September 8, 2026 17:59
TwitchBronBron added a commit that referenced this pull request Sep 9, 2026
Ports every master commit since 0.73.1 (through 0.73.3). Where master's code
collided with v1's rewrites, v1's architecture wins and the change was
re-implemented against it rather than taken verbatim.

Features ported:
- `continue` transpiles to a goto label for firmware below 11.5 (#489)
- go-to-definition for file path strings in BRS/BS/XML (#1648)
- `isTerminal`/`previousInChain` on AstNode (#1788)
- nested curly braces in template strings (#1539)
- regex literals after `${` and `,` (#1789)
- wrong-cased XML tag diagnostic (#1793)
- duplicate/crashing "find all references" fix (#1791)
- duplicate sourceMappingURL fix (#1786)
- findAncestor type-guard inference (#1787)
- lexer token-text interning (#1712)
- memory-aware default for max worker threads (#1798)
- js-yaml override bumped to ^4.3.2 (#1796)

Notable adaptations:
- master's whitespace fast-path skipped `addToken`, which in v1 is also what
  routes a token into `leadingTrivia`. Kept the Token allocation (trivia depends
  on it) and took only the interning half of that optimization.
- #1798 rewrote WorkerPool around master's simpler worker tracking. v1 has an
  `isDead` crashed-worker feature master lacks, so only the
  `getDefaultMaxWorkerThreads` logic was ported, on top of v1's tracking.
- `no-unsafe-argument` is a warning, not an error: master enabled it after
  cleaning up v0's call sites, and v1's rewrites carry ~76 more that were never
  part of that cleanup.
- three tests from master assert v0 behavior v1 changed on purpose
  (Comment tokens are trivia, `getReferences` returns `[]` not null, and the
  NamespacedVariableNameExpression chain step is gone). Updated to v1's contract.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

1 participant