:load clause-splitting (review #5):

navicore commented

2026-06-16 14:27:20 +00:00

Owner

New session::split_clauses reuses the real tokenizer to find clause
boundaries — a bare . end-token, mapped back to a byte offset via the
tokenizer's line/(byte-)col. So floats (3.14), quoted-atom dots ('a.b'),
comments, and 0'. never cause a false split. Comment/whitespace-only spans are
dropped; comments ride with the clause that follows.
Session::load_source validates the whole file once (one clean error for a bad
file), then appends each clause as an individual ordered entry. load_file uses
it and reports loaded (N clause(s)).
6 unit tests pin the tricky cases (ordering, directives as own clauses, float
dot, quoted-atom dot, multi-line clauses, comment-only spans).

:list:

Now that the buffer holds individual clauses, it renders them numbered, with
continuation lines aligned:
1 parent(tom, bob).
2 parent(bob, ann).
3 :- dynamic(extra/1).
4 ancestor(X, Y) :- parent(X, Y).
5 ancestor(X, Y) :-
parent(X, Z),
ancestor(Z, Y).

The verification compiled exactly that kind of file (facts + :- dynamic
directive + multi-line rule) and ?- ancestor(tom, X) returned X = bob / X = ann
— confirming the split entries recompile faithfully.

Net effect: the session buffer is now a genuinely ordered clause buffer as the
design intended, which is the shared groundwork the remaining : commands
(per-clause edit/retract, and the session-predicate completion follow-up #3)
will build on.

- New session::split_clauses reuses the real tokenizer to find clause boundaries — a bare . end-token, mapped back to a byte offset via the tokenizer's line/(byte-)col. So floats (3.14), quoted-atom dots ('a.b'), comments, and 0'. never cause a false split. Comment/whitespace-only spans are dropped; comments ride with the clause that follows. - Session::load_source validates the whole file once (one clean error for a bad file), then appends each clause as an individual ordered entry. load_file uses it and reports loaded <path> (N clause(s)). - 6 unit tests pin the tricky cases (ordering, directives as own clauses, float dot, quoted-atom dot, multi-line clauses, comment-only spans). :list: - Now that the buffer holds individual clauses, it renders them numbered, with continuation lines aligned: 1 parent(tom, bob). 2 parent(bob, ann). 3 :- dynamic(extra/1). 4 ancestor(X, Y) :- parent(X, Y). 5 ancestor(X, Y) :- parent(X, Z), ancestor(Z, Y). The verification compiled exactly that kind of file (facts + :- dynamic directive + multi-line rule) and ?- ancestor(tom, X) returned X = bob / X = ann — confirming the split entries recompile faithfully. Net effect: the session buffer is now a genuinely ordered clause buffer as the design intended, which is the shared groundwork the remaining : commands (per-clause edit/retract, and the session-predicate completion follow-up #3) will build on.

navicore added 1 commit

2026-06-16 14:27:20 +00:00

:load clause-splitting (review #5 ):

CI - Linux / CI - Linux x86_64 (pull_request) Successful in 1m15s

Details

d93693262e

- New session::split_clauses reuses the real tokenizer to find clause
  boundaries — a bare . end-token, mapped back to a byte offset via the
  tokenizer's line/(byte-)col. So floats (3.14), quoted-atom dots ('a.b'),
  comments, and 0'. never cause a false split. Comment/whitespace-only spans are
  dropped; comments ride with the clause that follows.
  - Session::load_source validates the whole file once (one clean error for a bad
  file), then appends each clause as an individual ordered entry. load_file uses
  it and reports loaded <path> (N clause(s)).
  - 6 unit tests pin the tricky cases (ordering, directives as own clauses, float
  dot, quoted-atom dot, multi-line clauses, comment-only spans).

  :list:
  - Now that the buffer holds individual clauses, it renders them numbered, with
  continuation lines aligned:
      1  parent(tom, bob).
      2  parent(bob, ann).
      3  :- dynamic(extra/1).
      4  ancestor(X, Y) :- parent(X, Y).
      5  ancestor(X, Y) :-
             parent(X, Z),
             ancestor(Z, Y).

  The verification compiled exactly that kind of file (facts + :- dynamic
  directive + multi-line rule) and ?- ancestor(tom, X) returned X = bob / X = ann
  — confirming the split entries recompile faithfully.

  Net effect: the session buffer is now a genuinely ordered clause buffer as the
  design intended, which is the shared groundwork the remaining : commands
  (per-clause edit/retract, and the session-predicate completion follow-up #3)
  will build on.

navicore commented

2026-06-16 15:00:32 +00:00

Author

Owner

Review — `:load` clause splitting

Clean targeted fix for issue #5 from the earlier review. Reusing the real tokenizer (instead of regex/string-split) is the right call: floats, quoted-atom dots, line comments, and 0'. all fall out for free because the tokenizer already handled them. :list rendering is now genuinely useful. Tests pin the tricky cases.

Correctness

The byte_offset translation works because the tokenizer is byte-indexed: col bumps once per byte (mod.rs:76) and resets to 1 on \n (mod.rs:74), which matches the byte-walk in byte_offset. The + 1 after the dot's byte offset (to include the . in the slice) is correct — Dot is one byte.

Whole-file validation before append is the right semantics: a malformed file leaves the buffer untouched and reports one error.

Small observations

1. add_clause now diverges from load_source for multi-clause single entries. If a user pastes foo. bar. at the prompt and presses Enter, add_clause parses it (the program parser accepts many clauses) and pushes one entry containing both. :list will show one numbered line spanning two clauses — the same shape this PR fixed for :load. Not in scope here; obvious follow-up is to route add_clause through split_clauses too.

2. Missing test for block comments. The tokenizer handles /* ... */ in skip_whitespace, but the tests only cover % line comments. Worth one test that p. /* between */ q. splits cleanly into ["p.", "/* between */ q."] (block comment rides with the following clause), and especially that p(/* . */ x). does NOT split on the inner dot. Defends against a future tokenizer change that drops comment handling.

3. Missing test for 0'. The doc comment explicitly calls this out as safe, but nothing asserts it. A one-line test (foo(X) :- X = 0'..) keeps the claim honest.

4. Unreachable tokenize-fail fallback. split_clauses returns vec![src.trim().to_string()] if Tokenizer::tokenize fails — but load_source calls parse_program_with_directives first, which would have errored out. The path is dead in the only caller. Either vec![] or unreachable!("validated before split") would document the invariant.

5. :list width caps at 3 digits. "{:>3}" misaligns past 999 clauses. Pedantic; fine in practice.

What's good

Tokenizer reuse — no shadow parser, no regex
Byte-offset accounting is consistent throughout (tokenizer bytes ↔ slice indexing)
Atomic-on-failure semantics for :load
Comments preserved with the following clause (correct for a Prolog source view)
Tests cover ordering, directive-as-clause, float dot, quoted-atom dot, multi-line clause, and comment-only span

Ready to merge once the block-comment / 0'. tests are added (1-2 lines each). The add_clause follow-up is a natural next slice.

## Review — `:load` clause splitting Clean targeted fix for issue #5 from the earlier review. Reusing the real tokenizer (instead of regex/string-split) is the right call: floats, quoted-atom dots, line comments, and `0'.` all fall out for free because the tokenizer already handled them. `:list` rendering is now genuinely useful. Tests pin the tricky cases. ### Correctness The `byte_offset` translation works because the tokenizer is byte-indexed: `col` bumps once per byte (`mod.rs:76`) and resets to 1 on `\n` (`mod.rs:74`), which matches the byte-walk in `byte_offset`. The `+ 1` after the dot's byte offset (to include the `.` in the slice) is correct — `Dot` is one byte. Whole-file validation before append is the right semantics: a malformed file leaves the buffer untouched and reports one error. ### Small observations **1. `add_clause` now diverges from `load_source` for multi-clause single entries.** If a user pastes `foo. bar.` at the prompt and presses Enter, `add_clause` parses it (the program parser accepts many clauses) and pushes one entry containing both. `:list` will show one numbered line spanning two clauses — the same shape this PR fixed for `:load`. Not in scope here; obvious follow-up is to route `add_clause` through `split_clauses` too. **2. Missing test for block comments.** The tokenizer handles `/* ... */` in `skip_whitespace`, but the tests only cover `%` line comments. Worth one test that `p. /* between */ q.` splits cleanly into `["p.", "/* between */ q."]` (block comment rides with the following clause), and especially that `p(/* . */ x).` does NOT split on the inner dot. Defends against a future tokenizer change that drops comment handling. **3. Missing test for `0'.`** The doc comment explicitly calls this out as safe, but nothing asserts it. A one-line test (`foo(X) :- X = 0'..`) keeps the claim honest. **4. Unreachable tokenize-fail fallback.** `split_clauses` returns `vec![src.trim().to_string()]` if `Tokenizer::tokenize` fails — but `load_source` calls `parse_program_with_directives` first, which would have errored out. The path is dead in the only caller. Either `vec![]` or `unreachable!("validated before split")` would document the invariant. **5. `:list` width caps at 3 digits.** `"{:>3}"` misaligns past 999 clauses. Pedantic; fine in practice. ### What's good - Tokenizer reuse — no shadow parser, no regex - Byte-offset accounting is consistent throughout (tokenizer bytes ↔ slice indexing) - Atomic-on-failure semantics for `:load` - Comments preserved with the following clause (correct for a Prolog source view) - Tests cover ordering, directive-as-clause, float dot, quoted-atom dot, multi-line clause, and comment-only span Ready to merge once the block-comment / `0'.` tests are added (1-2 lines each). The `add_clause` follow-up is a natural next slice.

navicore added 1 commit

2026-06-16 15:39:07 +00:00

⏺ The review is useful and accurate — and point #3 paid off in an unexpected way.

CI - Linux / CI - Linux x86_64 (pull_request) Successful in 1m40s

Details

901cba76e1

Verdict per item:

  Acted on (the two merge-gating asks):
  - #2 block comments — valid; the tokenizer does handle /* */ (in
  skip_whitespace). Added a test: p. /* between */ q. → ["p.", "/* between */
  q."], and p(/* . */ x). stays one clause (inner dot doesn't split). Passes.
  - #3 0'. — more useful than the suggested test. Trying to assert it revealed
  the engine has no 0'c char-code syntax at all — c(X) :- X = 0'a. fails
  tokenizing ("Unterminated quoted atom"). So my splitter doc comment
  over-claimed. I removed the false 0'. clause from the doc rather than assert a
  behavior that doesn't exist. Good catch indirectly — it kept a wrong claim from
  shipping.

  Confirmed real, recommend as the next slice (not this PR):
  - #1 add_clause divergence — verified: typing foo. bar. at the prompt parses
  and lands as one buffer entry, so :list shows two clauses under one number —
  the exact shape this PR fixed for :load. The clean fix is to route interactive
  clause entry through split_clauses too (likely just have the Clause dispatch
  arm call load_source, unifying both paths). Small and worth doing — want me to
  take it next?

  Noted, not acting:
  - #4 unreachable fallback — I'd keep the defensive vec![src.trim()]. The
  reviewer's "dead in the only caller" is true for load_source (it validates
  first), but split_clauses is pub and tokenizing genuinely can fail (the 0'.
  case proves it), so unreachable! would be a latent panic. The current fallback
  is the safer choice for a public fn.
  - #5 :list 3-digit width — agree it's pedantic; fine in practice.

  All green: 9 tests pass, clippy -D warnings clean, fmt clean.