fix(expr): Centralize the multi-line offset shift and fix the operator caret

[leongdl]

Picks up the three review comments on #321 that arrived shortly before it merged and were never answered, and fixes #339, which came out of the second one.

Fixes #339 — the caret pointed one column right of the operator

Multi-line expressions are wrapped in parens for implicit line continuation, shifting every parser-reported offset by one. Four consumers each undid that themselves, and compute_caret_offset did not undo it at all: it indexed the unwrapped source with still-shifted AST offsets.

There is now one definition of the rule, next to the wrap that causes it:

pub(crate) fn parser_offset_shift(source: &str) -> usize {
    usize::from(source.contains('\n'))
}

All four consumers go through it — the caret formatter, Display for ExpressionError, the eval_number float passthrough, and the keyword-rename retry. Three open-coded contains('\n') tests are gone.

compute_caret_offset now unshifts before indexing. Its return value is a difference between two shifted offsets, so the shift already cancelled there; only the byte reads depended on it, which is exactly why the BinOp arm was wrong while Attribute/Call/Subscript looked fine. Those three arms are unshifted too, so the rule is applied uniformly rather than relying on the cancellation holding.

That alone wasn't sufficient. With correct offsets, the byte between a multi-line operator and its right operand is the newline, and the backwards operator scan skipped spaces, tabs and ( but not line breaks — so it stopped on the newline and never tested for a two-character operator. The skip set now includes \n and \r. Both halves are needed: unshifted offsets to read the right bytes, and a skip set that gets past the line break.

Diagnostic output changes

Four existing tests pinned the off-by-one, so this is the behaviour change #339 predicted. Verified mechanically rather than by eye — in every case the old caret column landed on the space after the operator:

test	operator col	old caret	new caret
multiline_type_error_in_parens	6	7 (a space)	6 (+)
multiline_type_error_in_list	6	7 (a space)	6 (+)
multiline_type_error_on_first_line	4	5 (a space)	4 (+)
multiline_type_error_deeply_nested	8	9 (a space)	8 (+)

The new output also agrees with bare_multiline_error_shows_correct_line and the single-line cases, which already expected caret-on-operator — so those four were the odd ones out.

The #[ignore]d test for this is un-ignored. Added single_line_two_char_operator_caret_is_unchanged as a control (no shift to undo, must render identically) and multiline_single_char_operator_caret_points_at_the_operator, since single-character operators go through the same scan without the two-character branch.

renames was a HashMap searched linearly

rename_keyword_at needed keyword → placeholder, but keyword_renames is placeholder → keyword — which is the direction every reader wants, since build_symbol_name, collect_symbols and the evaluator all do renames.get(placeholder). So the write path scanned the map:

let replacement = renames
    .iter()
    .find(|(_, original)| original.as_str() == token)

Both directions are genuinely used, so there is now an index for each. Kept the HashMap rather than switching to Vec<(String, String)> because the read direction is a real hash lookup; only the reverse was pretending.

rename_keyword_at_reuses_one_placeholder_per_keyword pins what that lookup is for: two accesses of the same keyword share a placeholder, so renames holds one entry rather than one per occurrence. Nothing covered it before.

To be clear about what it is and isn't: this is a regression guard, not a fix. The base's linear scan was itself a reverse lookup, so reuse already worked — verified by running the equivalent test against ad94e73, where it passes. An earlier draft of this description claimed the base picked a fresh placeholder; that was wrong. The test earns its place because reuse now depends on placeholders staying an inverse of renames, and nothing else would notice them drifting apart.

Splitting one map into two adds an invariant, so there is a debug_assert on the write path: a placeholder already present in renames means make_replacement handed out one belonging to another keyword, which would overwrite that keyword's entry while placeholders kept both. That is reachable through the unchecked fallback in #333, and it is worse post-split — pre-split the bad mapping flip-flopped and could self-correct, now the stale placeholder is durable. debug_assert rather than a hard error, so release behaviour is unchanged and #333 keeps the decision about the correct response.

make_replacement's comment was wrong, and implied a check it doesn't do

// Use a deterministic scheme: prefix with underscores to match length
// e.g., "if" → "xf", "def" → "xef", "else" → "xlse"

No underscores, and the examples don't match the code — it replaces the first character with a..=z in turn, so "if" becomes "af", not "xf". Corrected, and it now says the fallback is unchecked and points at #333, the defect @mwiebe filed off the back of that comment. Not fixing #333 here; it wants its own tests.

Verification

cargo test --workspace (7234 tests, up 3), cargo fmt --all -- --check and cargo clippy --workspace --all-targets all clean.

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[github-actions]

The comment here — and by extension the test's premise — describes behavior the base code did not have. Before this PR, rename_keyword_at reused the placeholder via a linear scan over the map's values:

let replacement = renames
    .iter()
    .find(|(_, original)| original.as_str() == token)
    .map(|(placeholder, _)| placeholder.clone())
    .unwrap_or_else(|| { ... });

That scan already found the existing placeholder for a repeated keyword, so A.class + B.class produced one renames entry and a shared placeholder on the base commit too. This test passes with or without the placeholders map, so it does not pin the behavior the comment says it pins ("Without the reverse index the second call picks a fresh placeholder"). The unwrap_or_else branch that calls make_replacement was only reached when no entry for the keyword existed, so make_replacement never saw the already-substituted source for that keyword.

The change is still worthwhile as an O(n) → O(1) lookup on the write path, but framing it as a correctness fix is misleading. Suggest either rewording the comment to say what it actually is (a regression guard for the reuse invariant, which the new inverse map must preserve), or dropping the "without the reverse index" claim.

[leongdl]

You're right and I was wrong. Verified it rather than argue: transplanted the equivalent test onto ad94e73 with the base signature and it passes.

test eval::parse::tests::base_reuses_one_placeholder_per_keyword ... ok

The base's linear scan was a reverse lookup — it just did it in O(n) — so A.class + B.class already produced one entry and a shared placeholder, and make_replacement never saw the already-substituted source. My "without the reverse index the second call picks a fresh placeholder" was simply false.

Worth naming how I fooled myself, since it's the more useful part: I "verified" the test by mutating the lookup to always miss, which fails as advertised. But that mutation isn't the base behaviour — it's a third thing that never existed. Checking against the actual base commit was the step I skipped.

Reworded in aefd557 to say what it is:

// A regression guard, not a fix: the lookup this pins has always reused. It is here
// because the reuse now depends on `placeholders` staying an inverse of `renames`,
// and nothing else would notice the two drifting apart.

PR description corrected too — it repeated the same claim.

[github-actions]

The new test is inserted between the module-level explanatory comment and the test that comment describes. The block ending ...the wrong keyword gets recorded as renamed), it is just invisible downstream, so it is pinned here. is about the assert-vs-as token-delimiting defect, i.e. rename_keyword_at_delimits_the_whole_token immediately below. With the reuse test spliced in between, that comment now reads as documentation for the reuse test, which has nothing to do with prefix matching.

Moving the new test to after rename_keyword_at_delimits_the_whole_token (or above the block comment) keeps the comment attached to the test it explains.

[leongdl]

Good catch, and an easy one to miss when inserting by anchor rather than reading the surrounding block. Moved in aefd557 to sit after rename_keyword_at_delimits_the_whole_token, so the assert-vs-as explanation is adjacent to the test it explains again:

1183:    // renamed), it is just invisible downstream, so it is pinned here.
1184-    #[test]
1185-    fn rename_keyword_at_delimits_the_whole_token() {

[github-actions]

Splitting one map into two introduces an invariant that only the test helper enforces. Nothing on this write path checks that renames and placeholders stay inverses, and the unchecked make_replacement fallback (#333) can break it: if two same-length keywords both fall back to "x".repeat(len), renames.insert(placeholder, ...) silently overwrites the first keyword's entry while placeholders retains both, so the maps desync in length and one keyword resolves back under the other's name.

Worth noting this changes behavior versus the base in that path. Pre-PR, a later occurrence of the overwritten keyword fell through to make_replacement again and re-inserted its own renames entry, so the map flip-flopped. Post-PR, placeholders reuses the stale placeholder and renames never recovers — the wrong mapping is now durable. Both are wrong, so this isn't a regression in correct output, but it makes the #333 collision harder to notice.

A cheap guard that would surface it in tests:

debug_assert!(
    !renames.contains_key(&placeholder),
    "placeholder {placeholder:?} already maps to {:?}",
    renames.get(&placeholder)
);

[leongdl]

This is the sharpest of the three, and the durability point is one I'd missed: pre-PR the bad mapping flip-flopped and could self-correct, post-PR the stale placeholder is reused and renames never recovers. Same wrong output either way, but harder to notice — which is a real cost of splitting the map, not just a theoretical one.

Took your guard verbatim in aefd557, with the reasoning attached:

let placeholder = make_replacement(token, source);
// The two maps have to stay inverses. A placeholder already present in
// `renames` means `make_replacement` handed out one that belongs to a
// different keyword, which would overwrite that keyword's entry here while
// `placeholders` kept both -- the surviving mapping then resolves one keyword
// under the other's name. `make_replacement`'s fallback does not check for
// collisions, so this is reachable: see issue #333.
debug_assert!(
    !renames.contains_key(&placeholder),
    "placeholder {placeholder:?} is already taken by {:?}",
    renames.get(&placeholder)
);

debug_assert rather than a hard error deliberately: in release it should not change behaviour relative to today, and #333 is the place to decide what the correct response to exhausting the candidate space is. My guess is make_replacement should return Option and the rename should fail, letting the original parse error surface rather than parsing something the user didn't write — but that's the discussion for #333, and this at least makes the collision loud under test.

cargo test --workspace is green with the assert in place, so nothing in the suite currently trips it.

[github-actions]

This test is a strictly weaker duplicate of single_line_two_char_operator_caret_points_at_first_char twenty lines above, which already asserts the same two expressions:

assert_err("1 ** 'a'", &["  1 ** 'a'\n", "  ~~^~~~~~"]);
assert_err("1 // 'a'", &["  1 // 'a'\n", "  ~~^~~~~~"]);

assert_err is contains-based, so " ~~^" here matches a prefix of the caret line and the trailing ~~~~~ goes unchecked. As a "control for the above" it therefore verifies less than the test it is controlling for, and would keep passing if the span width regressed.

The same truncation applies to the two multi-line tests just above. multiline_two_char_operator_caret_points_at_first_char asserts " ~~^" while the actual render is " ~~^~" (span is 4 wide for 1 **\n'a'), and multiline_single_char_operator_caret_points_at_the_operator asserts " ~~^" against a 3-wide span. Those do still discriminate against the ~~~^ regression they exist for, so they are not broken — but AGENTS.md asks these tests to pin the full caret line, and spelling out the exact width is what makes the caret column and the span width both regression-proof.

Suggest dropping single_line_two_char_operator_caret_is_unchanged (the existing test is the control) and writing the full caret strings in the two multi-line tests.

[leongdl]

All three points correct. Fixed in 0722237.

Dropped single_line_two_char_operator_caret_is_unchanged. You're right that it was strictly weaker than single_line_two_char_operator_caret_points_at_first_char twenty lines up, which already asserts the full " ~~^~~~~~" — mine asserted a prefix and so verified less than the test it claimed to be a control for.

Captured the real widths rather than guessing them, via a throwaway probe test that dumps the render line by line:

EXPR "1 **\n'a'"  ->  "  ~~^~"
EXPR "1 //\n'a'"  ->  "  ~~^~"
EXPR "1 +\n'a'"   ->  "  ~~^"
EXPR "1 *\n'a'"   ->  "  ~~^"

So the two-char multi-line assertions now spell out " ~~^~". The single-char ones were already complete at " ~~^" — I've noted that in the comment so the next reader doesn't "fix" them into something longer.

Also added the reason to the two-char test, since a truncated expectation looks deliberate otherwise:

// Full caret line, not a prefix: `assert_err` is contains-based, so a truncated
// expectation would pin the column while letting the span width drift.

[github-actions]

parser_offset_shift is documented as "how far every parser-reported offset is shifted", but Display applies it to every attached span, and not all spans come from the parser. with_span is also called with plain byte offsets into a source that was never paren-wrapped:

• format_string.rs:482,498,509,518,529 — with_span(input, op, be), where input is the whole format string and op/be are true byte offsets into it.
• range_expr.rs:88 — with_span(&e.expr, pos, pos + 1).

For any of those whose source contains a newline, Display subtracts a shift that was never applied, and the caret lands one column left of the character being reported. A format string like "a\nb }}" hits the "Missing opening braces" path with with_span(input, 4, 6) and shows this.

Not a regression — the pre-PR if is_multiline { col.saturating_sub(1) } behaved identically. The concern is specific to this refactor: centralizing under a name that says "parser offset" makes the mismatch read as handled, while the subtraction still happens at the display site, where the provenance of col/end_col is no longer knowable. Normalizing at attach time instead — subtract inside with_node, leave with_span alone since its callers already pass unwrapped offsets — puts the adjustment where provenance is known and makes this class of mismatch unrepresentable.

[github-actions]

The doc here (and the rewritten parse_inner comment) says "anything converting a parser offset back to a position in source goes through it", but two consumers still use raw parser offsets and neither was updated:

1. eval/evaluator.rs:32-35 — append_sub_error reads err.col_offset() / err.end_col_offset() straight into write_caret_line. Those were set by with_node, so they are shifted AST offsets, while err.expr() printed on the line above is the unwrapped source. Reachable via eval_ifexps both-branches-fail path: the sub-error carets sit one column right for a multi-line expression. (This path bypasses Display, so the fix in error.rs:313 does not cover it.)

2. openjd-model/src/template/validate_v2023_09/format_strings.rs:279-288 — sub.col_offset() / sub.end_col_offset() are copied verbatim into DiagnosticSpan { start, end, .. } alongside source: expr.to_string(). Same mismatch, and it crosses a crate boundary, so parser_offset_shift being pub(crate) means the model crate cannot apply it even if it wanted to.

Both are pre-existing and diagnostic-only. Flagging because the stated invariant of this refactor is "one place owns the rule", and these two are exactly the sites that invariant would have to cover to hold. Either extend them, or narrow the doc wording to the three call sites actually converted — otherwise the next reader has the same false assurance the deleted NOTE block was written to prevent.

Normalizing inside with_node (see the other comment on error.rs) would fix all three at once, since every one of these spans originates there.

[github-actions]

The comment claims the full caret line pins the span width — it does not. assert_err (line 26-30) concatenates the slice and does e.contains(&joined), so the expectation is a substring match with no right anchor. " ~~^~" is a prefix of " ~~^~~", so if the span widened by one the assertion would still pass. The stated distinction between this and a truncated expectation does not exist under a contains-based helper.

Same applies to multiline_single_char_operator_caret_points_at_the_operator below ("Caret line is complete here").

If the intent is to pin the trailing edge, the caret line has to be anchored — e.g. append the trailing "\n" if one follows, or assert on the exact caret line by splitting the rendered error. Otherwise the comments should just drop the claim, so a future reader does not trust a guard that is not there.

[github-actions]

This turns the #333 collision from silent wrong output into a panic on user-controlled input in any debug build.

The collision is reachable, and the comment above says so. make_replacement reaches the unchecked "x".repeat(len) fallback when all 26 candidates for a keyword are either already substrings of source or are themselves keywords — and source is the expression text, so a string literal is enough to force it. Two same-length keywords both falling back both yield "xx", and this assert fires. Shape of it:

X.in + Y.is + [a string literal containing an,bn,...,zn and as,bs,...,zs]

Expressions come from job templates, i.e. untrusted input. debug_assert! is compiled out under --release, so the published CLI is unaffected — but openjd-expr is a published library, and anything depending on it that builds in dev/debug (plain cargo build, cargo test, most downstream dev workflows) would abort on such a template instead of producing the wrong-but-recoverable answer it produces today. AGENTS.md also states "Prefer Result types over panicking."

The invariant is real and worth enforcing, but the trigger is input-driven rather than a programming error, so the enforcement belongs in make_replacement: have the fallback keep searching (widen beyond first-character substitution, or append a disambiguating suffix while holding the length) and return Option, so an exhausted search surfaces as a parse error on the one expression instead of a process abort. That also closes #333 rather than instrumenting it.

If the intent is only to document the invariant for now, a debug_assert on input-reachable state is the wrong tool — a comment carries the same information without the abort.

[github-actions]

The is_multiline guard is now redundant: parser_offset_shift(&source) already returns 0 when there is no newline, and source keeps its newlines across renames (replacements are same-length), so is_multiline and parser_offset_shift(&source) == 1 are always in agreement. The whole conditional collapses to:

let kw_start = error_offset.saturating_sub(parser_offset_shift(&source));

Same pattern at error.rs:312-314, where is_multiline is computed and then shift is computed separately from the identical contains(NEWLINE) test. is_multiline is still genuinely needed there for the line-splitting below, but deriving one from the other (let is_multiline = shift == 1;) or dropping the redundant branch here would leave one expression of the rule instead of two sitting next to each other — which is the stated point of the change.