Skip to content

refactor(expr): split path/string equality or-pattern - #306

Closed
leongdl wants to merge 1 commit into
mainfrom
fix/equals-path-string-orpattern
Closed

refactor(expr): split path/string equality or-pattern#306
leongdl wants to merge 1 commit into
mainfrom
fix/equals-path-string-orpattern

Conversation

@leongdl

@leongdl leongdl commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Follow-up from the review of #305.

What was the problem/requirement? (What/Why)

ExprValue::equals uses the same combined or-pattern with swapped bindings that caused the path/string ordering reversal fixed in #305:

(Self::String(a), Self::Path { value: b, .. })
| (Self::Path { value: b, .. }, Self::String(a)) => a == b,

In the second alternative, a binds to the right operand. Equality is symmetric, so this is harmless today — but it is exactly the pattern that silently reversed comparisons in compare() (issue #290), and keeping it invites copying it back into an ordering context.

What was the solution? (How)

Split into two arms with position-consistent bindings, matching the fixed compare(), with a comment explaining why the split matters.

What is the impact of this change?

None at runtime. Pure refactor; no behavior or API change.

How was this change tested?

  • cargo test -p openjd-expr: 308 unit + 3138 integration + 8 doc tests pass, with no test modified (refactor contract).
  • cargo clippy -p openjd-expr --all-features --all-targets -- -D warnings clean.
  • cargo fmt --all -- --check clean.

Was this change documented?

The new inline comment documents the rationale. No spec change needed — behavior is unchanged.

Is this a breaking change?

No.

Does this change impact security?

No.


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

Split the combined or-pattern in ExprValue::equals into two arms with
position-consistent bindings, so the left operand always binds to a.
Equality is symmetric, so behavior is unchanged — but the identical
swapped-binding pattern in compare() silently reversed path/string
ordering (issue #290), and keeping the pattern here invites copying it
back into an ordering context.

Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
@leongdl
leongdl requested a review from a team as a code owner August 11, 2026 02:54
// the identical pattern in an ordering context silently reverses
// the comparison (see `compare`).
(Self::String(a), Self::Path { value: b, .. }) => a == b,
(Self::Path { value: a, .. }, Self::String(b)) => a == b,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment added here correctly identifies a real bug — but the bug it describes is still live in compare and was not fixed by this PR.

At value.rs:1341-1342:

(Self::String(a), Self::Path { value: b, .. })
| (Self::Path { value: b, .. }, Self::String(a)) => Ok(a.cmp(b)),

In the second alternative, self is the Path (binding its value to b) and other is the String (binding to a), so a.cmp(b) evaluates other.cmp(self) — the ordering is reversed whenever the Path is the left operand. So:

  • String("a").compare(Path("b"))Less
  • Path("a").compare(String("b"))Greater ❌ (should be Less)

This leaks into the evaluator: $(path) < "b" and "b" > $(path) will disagree, and any sort/min/max over a mixed String/Path list becomes order-dependent and non-transitive.

Suggested fix, mirroring the split applied to equals_charged in this PR:

(Self::String(a), Self::Path { value: b, .. }) => Ok(a.cmp(b)),
(Self::Path { value: a, .. }, Self::String(b)) => Ok(a.cmp(b)),

Worth adding a test asserting Path(x).compare(String(y)) is the reverse of String(y).compare(Path(x)) for x != y, since the existing coverage evidently only exercises the String-on-the-left direction.

@leongdl leongdl closed this Aug 11, 2026
@leongdl
leongdl deleted the fix/equals-path-string-orpattern branch August 11, 2026 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant