fix(semantic): use exclusive upper bound in ExpansionOffset::mapped TextSpan.end is exclusive throughout the codebase, so an offset exactly at a mapping's span.end lies outside that mapping. Replace the inclusive <= span.end check with an exclusive - #10017
Conversation
TextSpan.end is exclusive throughout the codebase, so an offset exactly at a mapping's span.end lies outside that mapping. Replace the inclusive `<= span.end` check with an exclusive range so a boundary offset resolves to the mapping that actually contains it (or to none), instead of matching the preceding adjacent mapping. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
This change is |
PR SummaryLow Risk Overview That only changes behavior when an expansion offset sits exactly on a mapping’s end boundary—such positions no longer map through that mapping and macro hygiene may walk to a parent environment instead. Reviewed by Cursor Bugbot for commit 21290ce. Bugbot is set up for automated code reviews on this repo. Configure here. |
TomerStarkware
left a comment
There was a problem hiding this comment.
@TomerStarkware reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on eytan-starkware).
Summary
Replaces the inclusive range check
mapping.span.start <= self.0 && self.0 <= mapping.span.endwith an exclusive range check using(mapping.span.start..mapping.span.end).contains(&self.0)in theExpansionOffset::mappedmethod.Type of change
Please check one:
Why is this change needed?
The previous span containment check used
<=on both ends, making it an inclusive range on the end boundary. Using the standardRange::containsmethod with an exclusive upper bound is more idiomatic Rust and aligns with how span/offset ranges are typically handled (exclusive end).What was the behavior or documentation before?
The mapping lookup included
mapping.span.endas a valid position, treating the range as fully inclusive (start <= offset <= end).What is the behavior or documentation after?
The mapping lookup uses an exclusive upper bound (
start <= offset < end), expressed via(mapping.span.start..mapping.span.end).contains(&self.0).Related issue or discussion (if any)
N/A
Additional context
Care should be taken to verify that no existing behavior depended on the end offset being inclusive, as this is a subtle semantic change in the range boundary.