Skip to content

feat: add legacy variable reference rewriter - #21

Merged
panayotisk merged 5 commits into
mainfrom
feat/legacy-variable-reference-rewriter
Sep 28, 2026
Merged

panayotisk merged 5 commits into
mainfrom
feat/legacy-variable-reference-rewriter

Conversation

@panayotisk

Copy link
Copy Markdown
Collaborator

Summary

  • add a parser-backed rewriteLegacyVariableReferences() API for legacy names that are not valid Arcscript identifiers
  • preserve original source formatting and encoded entities while excluding strings and mentions from rewrites
  • block ambiguous expressions, invalid replacements, duplicate names, and ambiguous scopes instead of applying unsafe changes
  • make the generated TypeScript package directly consumable as ESM and retain malformed legacy characters for rewrite analysis

Downstream validation

  • dry-ran the Arcweave repair against all 80 pending production-snapshot projects
  • planned all 159 variable renames with zero blocked variables
  • reviewed 134 source-field changes across 13 projects; no project data was updated
  • verified rich-text extraction separately uses htmlparser2, passing only raw <pre><code> ranges to this API

Testing

  • pnpm run build
  • pnpm test -- --run (226 tests)
  • pnpm run lint
  • packed-package install and direct Node ESM import smoke test
  • git diff --check

@pkg-pr-new

pkg-pr-new Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@arcweave/arcscript@21

commit: f79ff0b

@Zanzavar Zanzavar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed against freshly fetched main (1091fe0). The TypeScript build, lint, all 226 tests, and a direct Node ESM import check passed on Linux. Targeted reproductions exposed the five issues below.

Comment on lines +445 to +450
const offsets = this.codePointOffsets(source);
const ranges: SourceRange[] = [];
let mentionStart: number | null = null;
for (const token of tokens) {
const start = (offsets[token.start] ?? source.length) - prefix.length;
const end = (offsets[token.stop + 1] ?? source.length) - prefix.length;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P1] Keep string protection ranges in the lexer's offset units

createLenientLexer() constructs new CharStream(code), whose default offsets are UTF-16 code units. Mapping those offsets through codePointOffsets() shifts string and mention ranges whenever the preceding source contains supplementary Unicode characters. For example, with a variable named é and replacement é → e, show("😀😀", "é", é) becomes show("😀😀", "e", e) with no blocked IDs: the quoted text is changed too. Use the token offsets directly, or explicitly configure the stream to use code-point offsets consistently.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 8229219 by using the lexer UTF-16 offsets directly. Added the supplementary-Unicode/string regression case.

Comment on lines +391 to +395
return this.parsesAsArcscript(
candidate.scope === null
? candidate.oldName
: `${candidate.scope}.${candidate.oldName}`
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P1] Check ambiguity in the surrounding expression

Parsing the legacy name by itself misses names that consume part of otherwise valid Arcscript syntax. With variables named score and score), requesting the latter's rename to score_value turns valid show(score) into show(score_value, returning blockedVariableIds: []. The closing parenthesis is consumed because score) does not parse in isolation. Validate each occurrence in its source context, or conservatively block matches that can consume surrounding operators or delimiters.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 8229219 by conservatively blocking legacy names whose first or last token is an Arcscript operator or delimiter. Added the score) / show(score) regression case.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Rechecked 8229219: the original score) example is fixed, but the surrounding-expression ambiguity remains when the delimiter is inside the legacy name.

With existing variables named score, bonus, and a legacy variable named score, bonus, requesting the latter's rename to score_bonus transforms valid max(score, bonus) into max(score_bonus), returning blockedVariableIds: []. I verified that the original expression parses and that the rewritten expression fails with Invalid number of arguments for function max.

The first and last tokens are both identifiers, so the new guard does not catch the comma; parsing score, bonus alone also misses its valid interpretation as an argument list. This still needs occurrence-level context validation, or conservative handling of internal delimiters.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in d9653eb. Delimiter tokens are now treated conservatively anywhere in a legacy name, not only at its edges. The reported max(score, bonus) case is covered by a regression test.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Confirmed again on the current head, d9653eb: the delimiter and escape follow-up cases are fixed, but an unmatched quote inside a legacy name can still consume real syntax.

With a normal string variable named message and a legacy variable named message = "hello (without a closing quote), requesting the legacy variable's rename to message_hello transforms this valid assignment:

message = "hello"

into:

message_hello"

The result has blockedVariableIds: []. I verified that the original assignment parses, and the rewritten code raises ParseError after the variable is renamed.

In maskLegacyMatches(), the unmatched quote is a LEGACY_CHARACTER, not a STRING token, so the candidate is masked before protected ranges are computed. This removes the real opening quote from the lexer's input; parsing the legacy name in isolation also fails to detect its valid interpretation in context. Please keep this ambiguity case blocked, or preserve the original string boundaries before masking.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We will not be handling this case. If there is a double quote inside a variable name the corruption will not be fixed automatically, we would need to fix it manually.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in db7bd99. Legacy names containing an unescaped double quote are now blocked up front for manual repair, while escaped quotes remain supported. The reported assignment case is covered by a regression test.

Comment thread grammar/ArcscriptLexer.g4

// Keep tokenizing malformed legacy code so source-rewrite tooling can inspect
// the complete code block. The parser still rejects this token.
LEGACY_CHARACTER: .;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] Preserve strict handling in the existing rename APIs

This catch-all token also changes replaceVariables() and replaceScope(), which obtain tokens without parser validation. With separate variables named éscore and score, replaceVariables('<pre><code>éscore + score</code></pre>', { score: 'points' }) now returns <pre><code>époints + points</code></pre>, changing part of the unrelated legacy variable's name. I confirmed that the lexer on main raises a ParseError for the same input. Keep lenient tokenization isolated to the new rewriter, or reject LEGACY_CHARACTER tokens in the existing rename paths before applying replacements.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 8229219. The existing token-based rename APIs now reject LEGACY_CHARACTER tokens with ParseError before applying replacements; the lenient behavior remains available to the legacy rewriter.

Comment on lines +490 to +491
for (let position = match.start; position < match.end; position += 1) {
masked[position] = 'x';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] Preserve string escapes while masking candidate names

Candidate matches are masked before string ranges are identified, so a match ending in a backslash can remove the escape protecting a quote. With pending renames for variables named foo\ and round, the valid expression show("foo\"round") has its quoted round rewritten to the replacement name, despite containing no variable references. The foo\ match is replaced with x characters, making the escaped quote look like the end of the string. Preserve original string/escape boundaries when masking candidates.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 8229219 by preserving backslashes while masking candidate text, so masking cannot change quote escaping. Added the reported foo\ / escaped-quote regression case.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Rechecked 8229219: the original escaped-quote example is fixed, but preserving only the backslash still allows masking to invalidate a supported escape sequence and hide real references later in the code.

With a variable named nöm and replacement nom, the input below contains a literal backslash followed by n:

show("\nöm", nöm)

The rewriter returns the source unchanged with blockedVariableIds: []. The expected result is show("\nöm", nom), which I verified parses after the variable is renamed.

The match inside the string masks nöm to xxx, turning valid \n into invalid \x. The lexer then treats the string as malformed, and protection extends over the remaining source, including the actual variable reference. A caller proceeding with the rename is left with an unrepaired reference. Preserve complete escape sequences, or determine the original string boundaries before masking candidates.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in d9653eb. Masking now retains backslashes and replaces an escaped payload character with a valid neutral escape payload, preserving string tokenization without creating quote delimiters. The reported \nöm case and an escaped-quote-inside-name boundary case are covered.

Comment thread generate.sh Outdated
java -Xmx500M -cp ../antlr4.jar org.antlr.v4.Tool -Dlanguage=TypeScript ArcscriptLexer.g4 ArcscriptParser.g4 -visitor -no-listener -o ./TypeScript
mkdir -p ../TypeScript/src/Generated
cp TypeScript/*.ts ../TypeScript/src/Generated/.
sed -i "s|'./ArcscriptParserBase'|'./ArcscriptParserBase.js'|" ../TypeScript/src/Generated/ArcscriptParser.ts

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] Use an in-place replacement compatible with BSD sed

On macOS, sed -i requires a backup-extension argument, so this invocation fails and leaves the generated ArcscriptParserBase import without .js. The PR's macOS CI log already contains sed: 1: "../TypeScript/src/Gener ...": invalid command code .. The script continues and the C++ tests pass, masking the error; TypeScript packages generated on macOS retain the import that breaks direct Node ESM loading. Use a portable replacement command, such as writing to a temporary file and moving it into place.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 8229219 with a portable temporary-file replacement and fail-fast generation. bash generate.sh completes locally.

@Zanzavar Zanzavar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed d9653eb against freshly fetched main (1091fe0). Build, lint, and all 233 existing TypeScript tests pass. Targeted reproductions confirmed the four issues below; each returns blockedVariableIds: [].

Comment on lines +541 to +545
const semicolon = source.indexOf(';', position + 1);
if (semicolon !== -1 && semicolon - position <= 64) {
const encoded = source.slice(position, semicolon + 1);
const decoded = decodeHTMLStrict(encoded);
if (decoded !== encoded) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P1] Decode only the entity at the current source position

Scanning from any & to the next semicolon can include raw operators and a variable before a later entity. For example, with a variable named é, renaming it to e in true && é &gt; 1 returns true e 1, deleting both operators, with blockedVariableIds: []. decodeHTMLStrict() decodes the later &gt;, and every character in the entire substring is then mapped to the same source range. Recognize a single entity at the current position and map only the characters it actually consumes. The expected result, true && e &gt; 1, parses successfully.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in db7bd99. Entity decoding now uses the entities package's streaming decoder at the current ampersand, so only the recognized entity and its exact source span are consumed. The reported true && é > 1 case is covered by a regression test.

Comment on lines +386 to +387
if (!hasOperator) {
return false;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] Block padded names that also resolve to existing syntax

Returning early when there is no operator misses whitespace-padded names that tokenize as a different variable or a literal. With separate variables named score and score (one trailing space), requesting only the latter's rename to legacy_score transforms valid score + 1 into legacy_score+ 1, with no blocked IDs. This changes a reference to the existing score variable. Similarly, a legacy name true can capture a boolean literal. Check the trimmed/tokenized name for these ambiguities before accepting the no-operator case.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in db7bd99. Whitespace-padded names are checked against their trimmed Arcscript form and reserved literals before the no-operator path. Regression coverage includes both score / score-space and true-space.

Comment on lines +155 to +159
const decoded = this.decodeSource(code);
const possibleMatches = candidates
.filter(candidate => !blocked.has(candidate.id))
.flatMap(candidate => this.candidateMatches(decoded.text, candidate));
const protectedRanges = this.protectedRanges(decoded.text, possibleMatches);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] Preserve original string boundaries when decoding entities

Decoding entities before identifying protected strings can turn literal content into quote delimiters. With a variable named round and replacement round_variable, the valid source show("hello &quot;round&quot;") becomes show("hello &quot;round_variable&quot;"), with blockedVariableIds: [], even though it contains no variable reference. The decoded source looks like show("hello "round""), so round falls outside the protected ranges. Preserve the original literal ranges when decoding entities inside strings; the single-quoted equivalent with &#39; fails in the same way.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in db7bd99. Protection ranges are now computed in both decoded and original source coordinates, preserving raw string boundaries when encoded quotes occur inside a literal while retaining support for entity-encoded delimiters. Double- and single-quoted regressions also verify a real reference after the literal is still rewritten.

Comment on lines +399 to +402
for (const other of candidates) {
if (
other.id !== candidate.id &&
this.containsLegacyOperand(candidate.oldName, other.oldName)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] Include unselected legacy variables in ambiguity checks

candidates contains only IDs present in the replacement map, so whether a reference is considered ambiguous depends on which variables the caller requests to rename. With stored variables föo and föo+1, requesting only the latter's rename to foo_1 rewrites föo+1 to foo_1 with no blocked IDs. Requesting both renames correctly blocks the compound, as the existing regression test demonstrates. The föo operand exists in either case and still makes the reference ambiguous. Check operands against the full interpreter state, not just the selected replacement candidates.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in db7bd99. Compound-name ambiguity checks now inspect all variables in interpreter state, not only selected replacement candidates. The unselected föo plus selected föo+1 case is covered.

@Zanzavar Zanzavar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Rechecked db7bd99 against freshly fetched main (1091fe0). The four findings from the previous review are fixed. Build, lint, and all 236 existing TypeScript tests pass; the two cases below were reproduced separately.

decoded.text,
possibleMatches
);
const sourceProtectedRanges = this.protectedRanges(code, sourceMatches);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] Encoded strings can hide later variable references

The new raw-source protection pass interprets the apostrophe inside an entity-delimited string as an unmatched quote, then protects the rest of the source. With a normal string variable message and a legacy variable é, renaming é to e in message == &quot;Bob's cat&quot; and é > 0 returns the source unchanged with blockedVariableIds: []. The same input was correctly rewritten on d9653eb. After the caller renames the variable, the remaining é reference fails parsing; the expected output with e > 0 parses after entity decoding. Reconcile the raw and decoded string boundaries so an apostrophe inside a decoded literal cannot hide references after that literal.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in f79ff0b. The decoded pass now exposes its string ranges to the raw-source pass, which neutralizes only quote characters inside those decoded string interiors before lexing the raw source. This prevents apostrophes inside entity-delimited strings from extending protection over later references while preserving raw string boundaries. The regression also verifies a later Amise'sPower reference is still rewritten.

Comment on lines +525 to +528
if (
match.candidate.oldName.startsWith('"') ||
match.candidate.oldName.startsWith("'") ||
tokens.tokenNames.includes('STRING')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] Single-quoted string delimiters can still be consumed

A candidate containing an unmatched apostrophe is still masked before string ranges are identified: it neither starts with a quote nor produces a complete STRING token. With a stored variable named true is 'hello (no closing quote), renaming it to true_is_hello transforms valid true is 'hello' into true_is_hello', returning blockedVariableIds: []. I verified that the original executes with a false condition and the rewritten source raises a parse error. The new unescaped-double-quote guard does not cover this case. Block matches that consume an actual single-quoted literal opener, while retaining support for apostrophes within legacy names.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in f79ff0b. Candidates containing an unescaped single quote in delimiter position are now blocked before matching and masking. Apostrophes embedded in legacy identifiers, including Amise'sPower, remain supported. The reported true is 'hello case is covered by a regression test.

@Zanzavar Zanzavar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved with the remaining P2 finding documented inline. Reviewed f79ff0b against freshly fetched main (1091fe0): both previous findings are fixed, and build, lint, and all 238 existing TypeScript tests pass.

Comment on lines +182 to +184
!decodedProtectedRanges.all.some(range =>
this.rangesOverlap(range, match)
) &&

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] An encoded quote inside a raw string still hides later references

Decoded protection ranges are still applied unconditionally, even when decoding creates an unmatched quote inside an otherwise complete raw string. With a variable named é, renaming it to e in show("hello &quot;é", é) returns unchanged code with blockedVariableIds: []. The decoded unmatched quote protects the remaining source, including the actual variable reference. The expected result is show("hello &quot;é", e), which I verified parses after renaming; the returned source fails. The single-quoted equivalent, show('hello &#39;é', é), fails in the same way. Reconcile decoded unmatched-quote ranges with the original literal boundaries, or report the variable as blocked instead of silently skipping its reference.

@panayotisk panayotisk Sep 28, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Valid edge case, but intentionally not addressed in this remediation-focused PR. The production integrity inventory contains 159 invalid-name findings; its only quote-bearing names are ordinary internal apostrophes (What the dog doin'? and Amise'sPower), which are covered. I also checked the current 80-project validation snapshot: none of those project payloads contains " or ', and none of the 134 planned source changes contains encoded quote delimiters. Adding another raw/decoded range-reconciliation layer would therefore add risk without helping the projects this repair must handle. This can be handled separately if broader general-purpose rewrite support is needed.

@panayotisk
panayotisk merged commit 506e292 into main Sep 28, 2026
6 checks passed
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.

2 participants