Repository navigation
feat: add legacy variable reference rewriter - #21
Conversation
commit: |
Zanzavar
left a comment
There was a problem hiding this comment.
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.
| 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; |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
Fixed in 8229219 by using the lexer UTF-16 offsets directly. Added the supplementary-Unicode/string regression case.
| return this.parsesAsArcscript( | ||
| candidate.scope === null | ||
| ? candidate.oldName | ||
| : `${candidate.scope}.${candidate.oldName}` | ||
| ); |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| // Keep tokenizing malformed legacy code so source-rewrite tooling can inspect | ||
| // the complete code block. The parser still rejects this token. | ||
| LEGACY_CHARACTER: .; |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
| for (let position = match.start; position < match.end; position += 1) { | ||
| masked[position] = 'x'; |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
Fixed in 8229219 by preserving backslashes while masking candidate text, so masking cannot change quote escaping. Added the reported foo\ / escaped-quote regression case.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
Fixed in 8229219 with a portable temporary-file replacement and fail-fast generation. bash generate.sh completes locally.
Zanzavar
left a comment
There was a problem hiding this comment.
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: [].
| 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) { |
There was a problem hiding this comment.
[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 && é > 1 returns true e 1, deleting both operators, with blockedVariableIds: []. decodeHTMLStrict() decodes the later >, 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 > 1, parses successfully.
There was a problem hiding this comment.
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.
| if (!hasOperator) { | ||
| return false; |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
[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 "round"") becomes show("hello "round_variable""), 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 ' fails in the same way.
There was a problem hiding this comment.
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.
| for (const other of candidates) { | ||
| if ( | ||
| other.id !== candidate.id && | ||
| this.containsLegacyOperand(candidate.oldName, other.oldName) |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
[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 == "Bob's cat" 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.
There was a problem hiding this comment.
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.
| if ( | ||
| match.candidate.oldName.startsWith('"') || | ||
| match.candidate.oldName.startsWith("'") || | ||
| tokens.tokenNames.includes('STRING') |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
| !decodedProtectedRanges.all.some(range => | ||
| this.rangesOverlap(range, match) | ||
| ) && |
There was a problem hiding this comment.
[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 "é", é) returns unchanged code with blockedVariableIds: []. The decoded unmatched quote protects the remaining source, including the actual variable reference. The expected result is show("hello "é", e), which I verified parses after renaming; the returned source fails. The single-quoted equivalent, show('hello 'é', é), 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.
There was a problem hiding this comment.
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.
Summary
rewriteLegacyVariableReferences()API for legacy names that are not valid Arcscript identifiersDownstream validation
htmlparser2, passing only raw<pre><code>ranges to this APITesting
pnpm run buildpnpm test -- --run(226 tests)pnpm run lintgit diff --check