Fix location tracking and remove static caching to prevent parallel test failures#24
Merged
kjonescertinia merged 5 commits intoMar 27, 2026
Conversation
Changes: - Added optional location field to Modifier case class - Added optional location field to Annotation case class - Updated tokenToModifier() to preserve token locations - Updated parseModifiersAndAnnotations() to create spanned locations for compound modifiers (e.g., 'with sharing') - Updated parseAnnotation() to capture full annotation span from @ through parameters Both location fields default to None for backward compatibility and exclude location from equality/hash checks to preserve existing semantics. All 6,302 tests pass with these changes. Resolves #19
nawforce
force-pushed
the
add-location-tracking-to-modifier-and-annotation
branch
from
01ad5b8 to
8efc3ca
Compare
…est failures This commit completes issue #19 by implementing proper location tracking for Modifier and Annotation types and fixing several related issues: **Location Tracking:** - Changed lineOffset from 1-based to 0-based indexing to match ANTLR conventions - Made end positions exclusive ([start, end) half-open interval) for both columns and byte offsets - Enabled location validation in tests by comparing ANTLR vs outline parser locations **Caching Bug Fix:** - Removed intern() methods and static caches from Annotation and Modifier companion objects - Removed all intern() calls from factory methods in Types.scala - The static caches were causing parallel test failures because ArrayInternCache uses equality that ignores location field, returning cached instances with wrong locations **Test Infrastructure:** - Added syncNodeModules task to ensure node_modules are available for ScalaJS tests - Optimized ANTLR location extraction by skipping byte offset calculation (too expensive) - Updated Compare.scala to ignore byte offsets when comparing locations - Added UndefOr[Token] handling for ScalaJS where context.stop might be undefined **Dependency Updates:** - Upgraded apex-ls to 6.0.2 to fix lexer errors with malformed files - Upgraded Scala.js to 1.18.2 for compatibility with apex-ls 6.0.2 All 6,303 tests pass successfully with location tracking fully enabled. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
nawforce
force-pushed
the
add-location-tracking-to-modifier-and-annotation
branch
from
8efc3ca to
86530c7
Compare
| @@ -1,4 +1,4 @@ | |||
| addSbtPlugin("org.scala-js" % "sbt-scalajs" % "1.12.0") | |||
| addSbtPlugin("org.scala-js" % "sbt-scalajs" % "1.18.2") | |||
| addSbtPlugin("org.portable-scala" % "sbt-scalajs-crossproject" % "1.2.0") | |||
Contributor
There was a problem hiding this comment.
Should we update the other dependencies here
Addresses PR comment requesting dependency updates. Upgraded sbt to 1.12.8, Scala to 2.13.17 for compatibility, and all sbt plugins to their latest versions. Removed deprecated Sonatype resolver settings that were sunset in 2025. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
kjonescertinia
requested changes
Mar 27, 2026
| startByteOffset, | ||
| endLine, | ||
| endLineOffset + 1, | ||
| endByteOffset + 1 |
Contributor
There was a problem hiding this comment.
This looks possibly dangerous for multi-byte character handling?
| val startPosition = tokens.head.location.startPosition | ||
| tokens.clear() | ||
| consumeClassBody(classTypeDeclaration) | ||
| // Consume trailing newline to match ANTLR's inclusive behavior |
Contributor
There was a problem hiding this comment.
We are repeating this block a few times, a helper funcion may be better
| } | ||
|
|
||
| private def tokenToModifier(token: Token): Modifier = Modifier(token.contents) | ||
| private def tokenToModifier(token: Token): Modifier = |
Contributor
There was a problem hiding this comment.
This looks a pointless, see also tokenToId below
PR Review Fixes: 1. Extract helper function for repeated trailing newline consumption - Added consumeTrailingNewline() to reduce code duplication - Replaces 4 instances of the same pattern in class/interface/enum parsing 2. Inline pointless wrapper functions in Types.scala - Removed tokenToModifier (1 usage) and tokenToId (7 usages) - Directly construct Modifier and LocatableIdToken objects at call sites Multi-byte Character Fixes: 3. Fix byte offset calculation for multi-byte UTF-8 characters - Pass charByteLength to buffer.append() to correctly track UTF-8 byte offsets - Calculate byte length for ASCII (1 byte), multi-byte chars (2-3 bytes), and surrogate pairs (4 bytes) - Ensures endByteOffset points past the last character for all encodings 4. Fix surrogate pair handling in Buffer - Added charCount parameter to buffer.append() (default=1, surrogate pairs=2) - Correctly calculates endCharOffset for emoji and other surrogate pairs - Ensures substring extraction captures both chars of surrogate pairs 5. Add comprehensive unit tests for Buffer byte offset calculation - Tests ASCII, 2-byte UTF-8, 3-byte UTF-8, 4-byte UTF-8 (emoji), and mixed content - Verifies byte offsets match actual UTF-8 encoding - Tests surrogate pair handling (Java UTF-16 strings with 2-char emojis) All 6,310 tests pass (including 7 new BufferTest cases). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
kjonescertinia
previously approved these changes
Mar 27, 2026
kjonescertinia
approved these changes
Mar 27, 2026
kjonescertinia
deleted the
add-location-tracking-to-modifier-and-annotation
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #19
Summary
This PR implements proper location tracking for Modifier and Annotation types and fixes parallel test failures caused by static caching.
Changes
Location Tracking
lineOffsetfrom 1-based to 0-based indexing to match ANTLR conventions[start, end)half-open interval) for both columns and byte offsetsCaching Bug Fix
intern()methods and static caches fromAnnotationandModifiercompanion objectsintern()calls from factory methods inTypes.scalaArrayInternCacheuses equality that ignores location field, returning cached instances with wrong locationsTest Infrastructure
syncNodeModulestask to ensure node_modules are available for ScalaJS testsCompare.scalato ignore byte offsets when comparing locationsUndefOr[Token]handling for ScalaJS wherecontext.stopmight be undefinedDependency Updates
Testing
All 6,303 tests pass successfully with location tracking fully enabled.
Related Issues