Skip to content

Fix correctness bugs and reduce hot-path allocations found in code review - #43

Merged
R-unic merged 3 commits into
rbx-loom:masterfrom
AlternativeLua:fix/code-review-findings
Jul 22, 2026
Merged

Fix correctness bugs and reduce hot-path allocations found in code review#43
R-unic merged 3 commits into
rbx-loom:masterfrom
AlternativeLua:fix/code-review-findings

Conversation

@AlternativeLua

Copy link
Copy Markdown
Contributor

Summary

A code review pass over the compiler surfaced a batch of correctness bugs and hot-path performance issues. This PR lands them in two commits: one fix: commit for behavior bugs and one perf: commit for allocation/overhead reductions.

Correctness (fix: commit)

  • Location.Equals compared only file + column, so positions on different lines with the same column were "equal", and GetHashCode disagreed with Equals. Now both use file + position.
  • TypeChecker.VisitIf read the else branch's exit state before visiting it, so a terminating else branch (else { return }) was invisible to definite-assignment merging.
  • Generated Luau strings didn't escape \ or the quote delimiter, producing broken output; long-bracket strings also lost a leading newline (Luau swallows the first one after [[).
  • RenderState.PopIndent hardcoded 2 characters, breaking any non-default Indent.
  • The parser silently dropped mut in expression position when not followed by an array literal; it now reports a diagnostic (for x : mut arr still parses).
  • TypeSolver.CheckCircular lost its ref writes through a lambda copy, so circular union/intersection members were never replaced with never.
  • The lexer's unterminated-string-at-newline check was dead code — the scan loop never stopped at \n. Strings now terminate at newlines and the existing diagnostic fires (matches the existing LexerTest expectation).
  • NodeId.Map retained every AST node ever constructed in an unsynchronized static dictionary. Removed; MustImportRuntimeLibrary records what it needs at reference time in the resolver instead.
  • VisitAssignmentOperator could throw NRE/InvalidCastException on malformed nested assignments instead of leaving a diagnostic.
  • Orphaned <=/>= comparisons were emitted as bare expression statements (invalid Luau) because the assignment check only excluded ==/~=.
  • Fail-fast exit code was a randomized string hash (nondeterministic per process); now a stable 1.
  • Parser.Expect/PeekKind could index out of range at position 0 / past the token list.

Performance (perf: commit)

  • BindType ran TypeSimplifier.Simplify, formatted a message, and hashed a debug diagnostic (forcing line/character resolution) for every node bind. Now gated behind TypeChecker.EmitDebugDiagnostics (default off) — this restores the benefit of the recent TextSpan work for the type-checking stage.
  • Type.IsNever/IsUnknown are pattern matches instead of Equals calls through the static visited-set guard (two HashSet operations per call).
  • The empty MacroContext is cached and invocation-macro classification runs once per visit instead of twice per identifier/property access.
  • Node construction uses plain loops + List.Sort instead of LINQ Where/Cast/OrderBy/ToList chains.
  • Diagnostic.SourceLines is cached instead of re-splitting the whole file per access; DiagnosticBag.ContainsErrors no longer allocates intermediate bags.
  • Removed dead lexer code left over from the operator-trie migration.

Test plan

  • Full suite passes: 1701/1701 (up from 1695 — six new tests cover the Location equality contract, the VisitIf exit-state fix, string escaping, and the orphaned-mut diagnostic).
  • The json_stringify Luau snapshot was updated for the now-correct \\n escaping.
  • Each commit builds independently.

🤖 Generated with Claude Code

AlternativeLua and others added 2 commits July 21, 2026 19:14
- Location.Equals/GetHashCode now compare File + Position (was column-only,
  which made different lines with the same column compare equal and violated
  the equals/hash contract)
- TypeChecker.VisitIf visits the else branch before reading its exit state,
  so definite-assignment sees terminating else branches correctly
- Escape backslashes and string delimiters in generated Luau strings; emit
  an extra leading newline in long-bracket strings (Luau swallows the first)
- RenderState.PopIndent uses Indent.Length instead of hardcoded 2
- Parser no longer silently swallows 'mut' when not followed by an array
  literal; 'for x : mut arr' still parses via a guard in ParseFor
- TypeSolver.CheckCircular rebuilds unions/intersections so circular members
  are actually replaced with never (ref writes were lost through the lambda)
- Lexer terminates string scanning at newlines so the unterminated-string
  diagnostic fires instead of silently spanning lines
- Remove NodeId.Map (unbounded static node retention, unsynchronized Add);
  MustImportRuntimeLibrary now records intrinsic-ness at reference time
- VisitAssignmentOperator bails gracefully on malformed nested assignments
  instead of throwing NRE/InvalidCastException
- Orphaned <=/>= comparisons are no longer emitted as bare expression
  statements (invalid Luau)
- DiagnosticBag fail-fast exits with a stable code instead of a randomized
  string hash
- Guard Parser.Expect/PeekKind against out-of-range token indexing

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…stics

- Gate BindType's per-node debug diagnostics (Simplify + formatted message +
  diagnostic hashing, which forced line/character resolution) behind the new
  TypeChecker.EmitDebugDiagnostics flag, default off
- Type.IsNever/IsUnknown are pattern matches instead of full Equals calls
  through the static visited-set guard
- Cache the empty MacroContext and classify invocation macro references once
  per visit instead of twice per identifier/property access
- Replace LINQ Where/Cast/OrderBy chains in the Node constructor with plain
  loops and List.Sort (runs for every node constructed during parsing)
- Cache Diagnostic.SourceLines instead of re-splitting the source per access
- DiagnosticBag.ContainsErrors no longer allocates intermediate bags
- Remove dead lexer code left over from the operator-trie migration
  (GetLiteralMatch, MatchesPatternAt, LiteralRulesByFirstCharacter)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@R-unic
R-unic merged commit 01886d8 into rbx-loom:master Jul 22, 2026
1 of 2 checks passed
@github-project-automation github-project-automation Bot moved this from Backlog to Done in Loom Release 1.0.0 Jul 22, 2026
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