navicore/patch-prolog
1
0
Fork 0
Code Issues Pull requests Releases 11 Activity Actions

● Done. I agree with the reporter, and the fix is in. #37

Merged
navicore merged 1 commit from issue36 into main 2026-06-26 05:04:39 +00:00
Conversation 1 Commits 1 Files changed 5 +42 -15
navicore commented 2026-06-26 04:53:04 +00:00
Owner
Copy link

Verdict on the issue

#36

The reporter is correct on every point: the atom path passed a bare atom as the
type_error(evaluable, _) culprit while the compound path correctly built Name/Arity,
ISO 13211-1 §7.8.9/§8.6 mandates a predicate indicator (so foo/0), the locus was line
~121, and the low-severity assessment is right.

What changed

crates/runtime/src/builtins/arith.rs

  • Added a predicate_indicator(m, name, arity) helper that builds the Name/Arity
    culprit term.
  • Split the old TAG_ATOM | TAG_LST arm: the atom case now emits Atom/0; the list case
    keeps its bare-term culprit (out of the issue's scope).
  • Routed the existing unknown-operator path through the same helper (byte-identical
    output, less duplication).

Tests (the 4 you approved updating to ISO):

  • arith.rs unit test → /(foo, 0)
  • control_arith.rs arithmetic_errors_match_v1 → /(foo, 0), with a comment marking it a
    deliberate ISO-over-v1 divergence (issue #36), in the same style as the documented
    cut divergence
  • two integration.rs provenance tests → /(foo, 0)

Verification

  • End-to-end repro now prints X is foo -> foo / 0 and X is foo(1) -> foo / 1 —
    consistent indicators.
  • Runtime arith unit tests, integration, control_arith, v1_arith, v1_control, and
    differential all green; clippy clean on the runtime crate.
Verdict on the issue https://git.navicore.tech/navicore/patch-prolog/issues/36 The reporter is correct on every point: the atom path passed a bare atom as the type_error(evaluable, _) culprit while the compound path correctly built Name/Arity, ISO 13211-1 §7.8.9/§8.6 mandates a predicate indicator (so foo/0), the locus was line ~121, and the low-severity assessment is right. What changed crates/runtime/src/builtins/arith.rs - Added a predicate_indicator(m, name, arity) helper that builds the Name/Arity culprit term. - Split the old TAG_ATOM | TAG_LST arm: the atom case now emits Atom/0; the list case keeps its bare-term culprit (out of the issue's scope). - Routed the existing unknown-operator path through the same helper (byte-identical output, less duplication). Tests (the 4 you approved updating to ISO): - arith.rs unit test → /(foo, 0) - control_arith.rs arithmetic_errors_match_v1 → /(foo, 0), with a comment marking it a deliberate ISO-over-v1 divergence (issue #36), in the same style as the documented cut divergence - two integration.rs provenance tests → /(foo, 0) Verification - End-to-end repro now prints X is foo -> foo / 0 and X is foo(1) -> foo / 1 — consistent indicators. - Runtime arith unit tests, integration, control_arith, v1_arith, v1_control, and differential all green; clippy clean on the runtime crate.
● Done. I agree with the reporter, and the fix is in.
All checks were successful
WASM Gates / WASM gates (Tier 1 wasi + Tier 2 reactor) (pull_request) Successful in 35s
Details
CI - Linux / CI - Linux x86_64 (pull_request) Successful in 1m36s
Details
d04a3930b3 
Verdict on the issue

  The reporter is correct on every point: the atom path passed a bare atom as the
  type_error(evaluable, _) culprit while the compound path correctly built Name/Arity,
  ISO 13211-1 §7.8.9/§8.6 mandates a predicate indicator (so foo/0), the locus was line
  ~121, and the low-severity assessment is right.

  What changed

  crates/runtime/src/builtins/arith.rs
  - Added a predicate_indicator(m, name, arity) helper that builds the Name/Arity
  culprit term.
  - Split the old TAG_ATOM | TAG_LST arm: the atom case now emits Atom/0; the list case
  keeps its bare-term culprit (out of the issue's scope).
  - Routed the existing unknown-operator path through the same helper (byte-identical
  output, less duplication).

  Tests (the 4 you approved updating to ISO):
  - arith.rs unit test → /(foo, 0)
  - control_arith.rs arithmetic_errors_match_v1 → /(foo, 0), with a comment marking it a
  deliberate ISO-over-v1 divergence (issue #36), in the same style as the documented
  cut divergence
  - two integration.rs provenance tests → /(foo, 0)

  Verification

  - End-to-end repro now prints X is foo -> foo / 0 and X is foo(1) -> foo / 1 —
  consistent indicators.
  - Runtime arith unit tests, integration, control_arith, v1_arith, v1_control, and
  differential all green; clippy clean on the runtime crate.
navicore commented 2026-06-26 05:03:33 +00:00
Author
Owner
Copy link

Review — Phase fix for issue #36 (evaluable atom culprit → Atom/0)

Verdict: approve. Correct, minimal, well-scoped, and the divergence is documented in all the right places. I verified the claims against the source and re-ran the affected suites — all green.

The fix is right

  • predicate_indicator(m, name, arity) builds /(Name, Arity) once and both the atom path and the unknown-operator path now route through it. The old unknown-operator path constructed the same three heap words inline, so this is a byte-identical refactor on that side (confirmed: the test at arith.rs:728 still reads /(foo, 1) unchanged) while fixing the atom side. Good deduplication — the two culprit shapes can no longer drift apart, which is exactly the class of bug #36 was.
  • The atom path now emits Atom/0, matching the compound path's foo/1. End-to-end the reporter's repro is now consistent (foo/0 vs foo/1), which was the whole point.

The TAG_ATOM / TAG_LST split is the correct call

Splitting the old combined TAG_ATOM | TAG_LST arm and leaving the list case on the bare-term culprit is the right scoping decision — the issue is about the atom indicator only, and you've said so in the comment. One note for a future issue (not this PR): a list literal like _ is [1,2] still yields type_error(evaluable, [1,2]), where strict ISO would want the functor indicator ('.'/2 / '[|]'/2). The single-element list [X] is also an ISO evaluable-as-X special case that isn't handled here. Both are pre-existing and explicitly out of scope — flagging only so the list arm's // out of scope comment has a paper trail pointing at what's deferred.

Tests / docs

  • The four ISO-over-v1 test updates are the honest move: control_arith.rs carries a clear DELIBERATE ISO-over-v1 divergence (issue #36) comment in the same style as the documented cut divergence, so the next person who diffs against v1 won't "fix" it back. ISO_COMPLIANCE.md records the rule and the v1 contrast. This is the right way to retire a v1-bug-compatible expectation.
  • I re-ran patch-prolog-runtime --lib arith (12 passed, incl. err_type_evaluable_atom_and_compound) and control_arith + integration + iso_atom_concat (30 + 9 passed). Clean.

Nit (non-blocking)

The iso_atom_concat.rs change is a pure rustfmt reflow of the errors() helper — unrelated to #36. Harmless, just slightly muddies the diff's "this is the #36 fix" story. No action needed.

Nothing blocking. Ship it.

## Review — Phase fix for issue #36 (evaluable atom culprit → `Atom/0`) **Verdict: approve.** Correct, minimal, well-scoped, and the divergence is documented in all the right places. I verified the claims against the source and re-ran the affected suites — all green. ### The fix is right - `predicate_indicator(m, name, arity)` builds `/(Name, Arity)` once and both the atom path and the unknown-operator path now route through it. The old unknown-operator path constructed the same three heap words inline, so this is a byte-identical refactor on that side (confirmed: the test at `arith.rs:728` still reads `/(foo, 1)` unchanged) while fixing the atom side. Good deduplication — the two culprit shapes can no longer drift apart, which is exactly the class of bug #36 was. - The atom path now emits `Atom/0`, matching the compound path's `foo/1`. End-to-end the reporter's repro is now consistent (`foo/0` vs `foo/1`), which was the whole point. ### The TAG_ATOM / TAG_LST split is the correct call Splitting the old combined `TAG_ATOM | TAG_LST` arm and leaving the list case on the bare-term culprit is the right scoping decision — the issue is about the atom indicator only, and you've said so in the comment. One note for a *future* issue (not this PR): a list literal like `_ is [1,2]` still yields `type_error(evaluable, [1,2])`, where strict ISO would want the functor indicator (`'.'/2` / `'[|]'/2`). The single-element list `[X]` is also an ISO evaluable-as-`X` special case that isn't handled here. Both are pre-existing and explicitly out of scope — flagging only so the list arm's `// out of scope` comment has a paper trail pointing at what's deferred. ### Tests / docs - The four ISO-over-v1 test updates are the honest move: `control_arith.rs` carries a clear `DELIBERATE ISO-over-v1 divergence (issue #36)` comment in the same style as the documented cut divergence, so the next person who diffs against v1 won't "fix" it back. `ISO_COMPLIANCE.md` records the rule *and* the v1 contrast. This is the right way to retire a v1-bug-compatible expectation. - I re-ran `patch-prolog-runtime --lib arith` (12 passed, incl. `err_type_evaluable_atom_and_compound`) and `control_arith` + `integration` + `iso_atom_concat` (30 + 9 passed). Clean. ### Nit (non-blocking) The `iso_atom_concat.rs` change is a pure rustfmt reflow of the `errors()` helper — unrelated to #36. Harmless, just slightly muddies the diff's "this is the #36 fix" story. No action needed. Nothing blocking. Ship it.
navicore deleted branch issue36 2026-06-26 05:04:39 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
navicore/patch-prolog!37
No description provided.