Fix #3170: Reject (|) when | declared as operator [SUPERSEDED] #3172

jjtolton · 2025-11-21T22:48:10Z

Status: SUPERSEDED by #3141

This PR attempted to fix #3170 (LR artifacts) by rejecting (|) when | is declared as an operator. However, this approach was incorrect per ISO TC2 C2, which specifies the opposite behavior.

See #3141 for the correct implementation.

Original description:

Fix #3170: Reject (|) when | declared as operator

Priority-based detection: when op(1105,xfy,'|') declared, '|' has priority > 1000.
Pattern (|) = operator without operands → syntax_error per ISO §6.3.4.

Defense-in-depth: checks in Token::Close handler + reduce_brackets().

101 test cases validate fix across all contexts (curly braces, lists, nested, valid atoms).

Test case from issue: {!*!(|)/} incorrectly parses as {/} Expected: {!*!(|)/} Actual: {/} The parser discards !*!(|) during error recovery. - J.J.'s Robot

Detects invalid (|) in {} context by checking for OpenCurly on stack. Valid (|) uses outside {} still work (e.g., permission_error). All 36 tests pass. - J.J.'s Robot

Defense-in-depth approach with priority-based detection: - Token::Close checks (|) pattern before reduce methods - reduce_brackets also checks and returns error - Uses priority > 1000 to detect when | is operator vs delimiter - Preserves (|) as atom when | not declared as operator All 36 tests pass. - J.J.'s Robot

Adds 78 additional test cases across three test suites to thoroughly validate the fix for issue mthom#3170 (parser artifacts when | is declared as an operator): - issue_3170_curly: 32 tests for (|) in curly brace contexts - issue_3170_list: 21 tests for (|) in list contexts (includes valid cases) - issue_3170_nested: 25 tests for (|) in nested and mixed contexts All tests verify that (|) patterns correctly throw syntax_error when op(1105,xfy,'|') is declared, preventing silent parser artifacts.

Document syntax semantics with standard references: - §6.3.3.1: Arguments must have priority ≤999 - §6.3.4: Operator notation and requirements - §6.3.5: List notation with ht_sep vs operator distinction - §6.3.6: Curly bracketed terms Clarifies why (|) with op(1105,xfy,'|') must fail: operator without operands violates §6.3.4 in all contexts.

jjtolton · 2025-11-22T23:39:28Z

ok feeling pretty good about this now.

22 test cases verify (|) works as regular atom without op declaration. Ensures fix only affects op(1105,xfy,'|') case, not normal atom usage. ISO §6.3.1.3: '|' is valid atom name.

{term} == '{}'(term), commas are comma operator (priority 1000). Examples: {a} == '{}'(a), {a,b} == '{}'(','(a,b)).

jjtolton · 2025-11-23T11:17:11Z

@UWN this is available in the combined-testing branch

UWN · 2025-11-24T20:21:48Z

ok feeling pretty good about this now.

@UWN this is available in the combined-testing branch

ulrich@gupu:/opt/gupu/jj-combined$ cargo build --release
   Compiling scryer-prolog v0.10.0 (/opt/gupu/jj-combined)
error[E0004]: non-exhaustive patterns: `lexer::Token::DoubleBar` not covered
   --> src/parser/parser.rs:383:24
    |
383 |         let tt = match token {
    |                        ^^^^^ pattern `lexer::Token::DoubleBar` not covered
    |
note: `lexer::Token` defined here
   --> src/parser/lexer.rs:33:10
    |
33  | pub enum Token {
    |          ^^^^^
...
45  |     DoubleBar,         // '||'
    |     --------- not covered
    = note: the matched value is of type `lexer::Token`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
    |
429 ~             Token::End => TokenType::End,
430 ~             lexer::Token::DoubleBar => todo!(),
    |

error[E0004]: non-exhaustive patterns: `lexer::Token::DoubleBar` not covered
   --> src/parser/parser.rs:975:15
    |
975 |         match token {
    |               ^^^^^ pattern `lexer::Token::DoubleBar` not covered
    |
note: `lexer::Token` defined here
   --> src/parser/lexer.rs:33:10
    |
33  | pub enum Token {
    |          ^^^^^
...
45  |     DoubleBar,         // '||'
    |     --------- not covered
    = note: the matched value is of type `lexer::Token`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
    |
1090~             },
1091~             lexer::Token::DoubleBar => todo!(),
    |

For more information about this error, try `rustc --explain E0004`.
error: could not compile `scryer-prolog` (lib) due to 2 previous errors
ulrich@gupu:/opt/gupu/jj-combined$ 
ulrich@gupu:/opt/gupu/jj-combined$ git log -1
commit 86249dae25c786d68df5d53190304374fa3097d4 (HEAD -> combined-testing, origin/combined-testing)
Merge: 7141efbe 0743a230
Author: J.J. Tolton <jjtolton@gmail.com>
Date:   Sat Nov 22 18:48:05 2025 -0500

    Merge branch 'fix-issue-3170-lr-artifacts' into combined-testing
ulrich@gupu:/opt/gupu/jj-combined$ git describe
fatal: No names found, cannot describe anything.
ulrich@gupu:/opt/gupu/jj-combined$

UWN · 2025-11-24T20:23:41Z

What do you get for:

ulrich@gupu:/opt/gupu/jj-combined$ target/release/scryer-prolog
?- {!*!(|)*} = T.
   T = {*}, unexpected.

UWN · 2025-11-24T20:33:06Z

FWIW now with:

ulrich@gupu:/opt/gupu/jj-combined$ rustc --version
rustc 1.91.1 (ed61e7d7e 2025-11-07)

But the error remains.

UWN · 2025-11-24T20:52:34Z

?- read_from_chars("_/_(|).",T).
   true, unexpected.

jjtolton · 2025-11-24T22:36:24Z

jay@Combinatus-Mk-XI:~/programs/scryer-prolog-issue-3151$ ./target/debug/scryer-prolog --version
v0.10.0-23-ge38d5e75-modified
jay@Combinatus-Mk-XI:~/programs/scryer-prolog-issue-3151$ ./target/debug/scryer-prolog -f --no-add-history
?- {!*!(|)*} = T.
   error(syntax_error(incomplete_reduction),read_term/3:1).
?- use_module(library(charsio)), read_from_chars("_/_(|).",T).
   error(syntax_error(incomplete_reduction),read_term_from_chars/3:0).
?-

there seems to have been merge errors that have given injury to the compiler in combined-testing branch, working them out now

jjtolton · 2025-11-25T08:40:48Z

fixed, pushed to combined testing

jay@Combinatus-Mk-XI:~/programs/scryer-prolog$ ./target/release/scryer-prolog --version
v0.10.0-100-g07fcbb64
jay@Combinatus-Mk-XI:~/programs/scryer-prolog$ ./target/release/scryer-prolog -f
?-  {!*!(|)*} = T.
   error(syntax_error(incomplete_reduction),read_term/3:1).
?- use_module(library(charsio)), read_from_chars("_/_(|).",T).
   error(syntax_error(incomplete_reduction),read_term_from_chars/3:0).

UWN · 2025-11-25T09:37:42Z

Now:

?- read_from_chars("{([)}.",T).
   T = {'['}, unexpected.
   syntax_error(...).

UWN · 2025-11-25T09:38:36Z

Even:

?- read_from_chars("([).",T).
   T = '[', unexpected.

UWN · 2025-11-25T09:41:52Z

Similar to s#294 which was discovered (and fixed) in Scryer prior to its inclusion in the table.

Parser was incorrectly accepting mismatched bracket patterns: - ([) was returning '[' instead of syntax_error - {([)} was returning {[} instead of syntax_error Root cause: reduce_brackets() would find any Open/OpenCT on the stack and use it to close with ), even if there was an unclosed OpenList or OpenCurly between them. Fix: Add check in reduce_brackets() to reject if the last token on the stack is OpenList or OpenCurly when closing with ). Added comprehensive test suite with 22 test cases covering: - Mismatched bracket patterns (12 tests) - all should error - Valid nested bracket patterns (10 tests) - all should succeed ISO/IEC 13211-1:1995 Reference: - Each bracket type must close with its matching delimiter - ( with ), [ with ], { with } 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

- Add issue_3172_brackets.toml with proper CLI test args - Rewrite test.pl to use silent-success pattern (no PASS/FAIL output) - Add empty .stdout and .stderr expected output files - Remove incorrect test.expected file The test now follows the trycmd CLI test framework conventions: - Tests succeed silently with exit code 0 - Failed tests cause scryer-prolog to exit with error - Expected output is empty (no verbose reporting) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

jjtolton · 2025-11-25T12:02:58Z

Ok, please pull on the combined-testing branch and try again.

jay@Combinatus-Mk-XI:~/programs/scryer-prolog$ ./target/debug/scryer-prolog -f
?- use_module(library(charsio)), read_from_chars("{([)}.",T).
   error(syntax_error(incomplete_reduction),read_term_from_chars/3:0).
?- use_module(library(charsio)),  read_from_chars("([).",T).
   error(syntax_error(incomplete_reduction),read_term_from_chars/3:0).
?-
jay@Combinatus-Mk-XI:~/programs/scryer-prolog$ ./target/debug/scryer-prolog --version
v0.10.0-83-g5781f1aa

UWN · 2025-11-25T13:25:43Z

Now:

?- read_from_chars("{[]||!}.",T).
   T = {!}, unexpected.

jjtolton · 2025-11-25T14:24:52Z

Now:


?- read_from_chars("{[]||!}.",T).

   T = {!}, unexpected.

I'm going to have to research what the specific violation is here to generalize the fix, I suppose it's that ! is not a string?

UWN · 2025-11-25T15:23:26Z

See db: The reason is that this is only defined for double quoted lists:

term = double quoted list, bar, bar, term ;

jjtolton · 2025-11-25T18:22:17Z

We are getting into weird territory now because this issue only shows up on the combined testing branch (due to the number of parse changes being mixed together). There are now changes in the combined testing branch that are not purely additive with the PRs, so I think much further testing against it might not be reflective of Scryer as whole.

We should probably start trying to get some of these branches merged in ( @mthom :D !!! )

The || (double bar) operator should ONLY be valid after double-quoted strings per ISO/IEC 13211-1:1995 and WG17 2025. Bug: `{[]||!}` was returning `{!}` instead of syntax_error `[a,b]||X` was parsing as `[a,b|X]` instead of syntax_error Changes: - push_binary_op now validates || and returns Result - reduce_op propagates errors from push_binary_op - compute_arity_in_list rejects DoubleBar inside list syntax - reduce_list no longer converts lists to CompleteString (the root cause - single-char atom lists like [a,b,c] were being converted to strings, bypassing || validation) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

UWN · 2025-11-25T21:54:53Z

Now:

?- read_from_chars("{!!||!}.",T).
   T = {!}, unexpected.

UWN · 2025-11-25T22:00:08Z

?- read_from_chars("{\"\"!||!}.",T).
   T = {[]}, unexpected.

UWN · 2025-11-25T22:01:09Z

or, for that matter:

?- read_from_chars("{\"\"a||a}.",T).
   T = {[]}, unexpected.

UWN · 2025-11-25T22:05:58Z

?- read_from_chars("{!/[|]/}.",T).
   T = {/}, unexpected.

Migrates parser fixes from combined-testing branch to properly reject: - (|) when | is declared as operator (ISO TC2 C.2) - [|], [|X], [a,|] - missing head/tail in list notation (ISO §6.3.5) - []||X, foo||X - non-string before || operator (WG17 2025) - Mismatched brackets like ([) or ({) Changes: - push_binary_op returns Result<(), ParserError> to propagate || errors - reduce_op returns Result<(), ParserError> for error propagation - reduce_term returns Result<bool, ParserError> - reduce_brackets returns Result<bool, ParserError> - compute_arity_in_brackets rejects HeadTailSeparator at even positions - compute_arity_in_list rejects DoubleBar tokens - reduce_list uses LIST_TERM spec for empty list [] to distinguish from empty string "" in codes mode - reduce_brackets checks for HeadTailSeparator with priority > 1000 and rejects mismatched OpenList/OpenCurly Test files added: - issue_3172_operator_bar: Tests (|), [|], [|X] patterns - issue_3172_doublebar: Tests || with non-string operands 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

jjtolton · 2025-11-27T01:19:33Z

@UWN: pull latest on jjtolton:combined-testing.

Note this branch now has gestalt fixes that are not in any of the individual PRs composing this mixed branch, so there may be some regressions when we merge in everything. We will need to retest later. The good news is that there will be a comprehensive test suite so we should catch everything that is tested for sooner.

jjtolton · 2025-11-27T01:29:14Z

actua

@UWN: pull latest on jjtolton:combined-testing.

Note this branch now has gestalt fixes that are not in any of the individual PRs composing this mixed branch, so there may be some regressions when we merge in everything. We will need to retest later. The good news is that there will be a comprehensive test suite so we should catch everything that is tested for sooner.

actually, this introduced a few issues. Will finish up tomorrow.

UWN · 2025-11-27T06:24:46Z

Just an administrative remark: this item 3172 now still reads as "jjtolton wants to merge 9 commits into mthom:master" and that is, to my understanding, inaccurate.

jjtolton · 2025-11-27T11:35:05Z

I'm going to have to set up an automated testing framework of some sort to start keeping track of which tests apply to which PRs. 3172 as an enhancement to scryer master was not meant to handle all of these other cases, we are really testing against combined-testing.

Many of the cases reported involve double bars, for instance, which is not actually a feature in the master branch yet. So fixing them for 3172 doesn't make sense because that would be reimplementing the double bars feature here.

UWN · 2025-11-27T11:37:22Z

I only see "forced updates", so what shall I test?

jjtolton · 2025-11-27T11:57:35Z

Short answer is you should still test against combined-testing but we should move the conversation so that I can setup automation and we can streamline maintenance and development without causing confusion.

Well, I think the only certainty we can have from testing against the combined-testing branch is that "Scryer has the capability to achieve these features" ... whether or not they are accepted/merged is another matter. I think in order to reduce confusion and ease testing/maintenance burden we should move the conversation regarding the combined-testing branch to https://github.com/jjtolton/scryer-prolog/tree/combined-testing and make issues against it there.

I am setting up automated testing for the combined-testing branch to help protect against regressions.

I will open up individual branches/PRs against Scryer Prolog when new features or bug reports that are distinct to Scryer origin/master, and transparently merge those into combined-testing.

jjtolton · 2025-11-27T12:05:28Z

@UWN I opened up issues here specific to combined-testing and I have also made it the default branch for ease of reference.

UWN · 2025-11-27T12:20:52Z

(Do you think it makes sense to continue like this? It seems you are active on many different issues here, but none is finished. Now you are also retracting history. It would make sense to concentrate on the syntax issues only. And only continue with other issues, if this is finished.)

jjtolton · 2025-11-27T13:29:52Z

(Do you think it makes sense to continue like this? It seems you are active on many different issues here, but none is finished. Now you are also retracting history. It would make sense to concentrate on the syntax issues only. And only continue with other issues, if this is finished.)

I am not aware of retracting any history, that was an operator error if that happened. I agree -- to be clear, I think we should continue the discussion here.

UWN · 2025-11-27T16:01:13Z

Here is what I get currently. That is exactly the problem.

ulrich@gupu:/opt/gupu/jj-combined$ git pull
remote: Enumerating objects: 65, done.
remote: Counting objects: 100% (65/65), done.
remote: Compressing objects: 100% (35/35), done.
remote: Total 65 (delta 33), reused 57 (delta 25), pack-reused 0 (from 0)
Unpacking objects: 100% (65/65), 16.27 KiB | 268.00 KiB/s, done.
From https://github.com/jjtolton/scryer-prolog
 + e4591012...a7c57a5e combined-testing -> origin/combined-testing  (forced update)
 + 16ea9d3d...e993a3b1 double-bar       -> origin/double-bar  (forced update)
hint: You have divergent branches and need to specify how to reconcile them.
hint: You can do so by running one of the following commands sometime before
hint: your next pull:
hint: 
hint:   git config pull.rebase false  # merge (the default strategy)
hint:   git config pull.rebase true   # rebase
hint:   git config pull.ff only       # fast-forward only
hint: 
hint: You can replace "git config" with "git config --global" to set a default
hint: preference for all repositories. You can also pass --rebase, --no-rebase,
hint: or --ff-only on the command line to override the configured default per
hint: invocation.
fatal: Need to specify how to reconcile divergent branches.

jjtolton · 2025-11-27T16:20:20Z

Assuming you see this:

git remote -v
  # Should show: origin  https://github.com/jjtolton/scryer-prolog.git

then run

  git fetch
  git reset --hard origin/combined-testing

and then you will have the latest.

I will take care not to edit history again.

UWN · 2025-11-27T19:57:20Z

Draft only

jjtolton · 2025-11-29T16:13:24Z

Closing as superseded by #3141, which correctly implements ISO TC2 C2 semantics.

The original approach here was inverted - it rejected (|) when | IS an operator, but ISO TC2 C2 specifies the opposite:

Reject (|) when | is NOT an operator (bar is "not an atom")
Allow (|) when | IS an operator (bar "equivalent to atom")

Issue #3170 (LR artifacts) remains open and will need a separate investigation once #3141 lands.

jjtolton mentioned this pull request Nov 21, 2025

Unexpected LR artefacts #3170

Open

jjtolton changed the title ~~Partial fix for #3170: Enforce priority 1199 limit in curly braces~~ WIP: fix parsing bux Nov 21, 2025

jjtolton marked this pull request as draft November 21, 2025 23:16

jjtolton marked this pull request as ready for review November 21, 2025 23:47

jjtolton marked this pull request as draft November 22, 2025 00:01

jjtolton force-pushed the fix-issue-3170-lr-artifacts branch from 0d2c6bb to d11c700 Compare November 22, 2025 09:57

Add failing test for issue mthom#3170 (LR artifacts)

d8a45e0

Test case from issue: {!*!(|)/} incorrectly parses as {/} Expected: {!*!(|)/} Actual: {/} The parser discards !*!(|) during error recovery. - J.J.'s Robot

jjtolton changed the title ~~WIP: fix parsing bux~~ WIP: fix parsing bug Nov 22, 2025

jjtolton added 3 commits November 22, 2025 16:48

Fix mthom#3170: Reject (|) pattern inside curly braces

9a279a2

Detects invalid (|) in {} context by checking for OpenCurly on stack. Valid (|) uses outside {} still work (e.g., permission_error). All 36 tests pass. - J.J.'s Robot

jjtolton force-pushed the fix-issue-3170-lr-artifacts branch from 4aa42b1 to f00ca62 Compare November 22, 2025 23:14

jjtolton marked this pull request as ready for review November 22, 2025 23:38

Add backward compatibility tests for issue mthom#3170

e38d5e7

22 test cases verify (|) works as regular atom without op declaration. Ensures fix only affects op(1105,xfy,'|') case, not normal atom usage. ISO §6.3.1.3: '|' is valid atom name.

jjtolton changed the title ~~WIP: fix parsing bug~~ Fix #3170: Reject (|) when | declared as operator Nov 22, 2025

Annotate curly brace structure per ISO §6.3.6

0743a23

{term} == '{}'(term), commas are comma operator (priority 1000). Examples: {a} == '{}'(a), {a,b} == '{}'(','(a,b)).

jjtolton marked this pull request as draft November 27, 2025 21:18

jjtolton changed the title ~~Fix #3170: Reject (|) when | declared as operator~~ Fix (|) parsing per ISO TC2 C2: operator-dependent validity Nov 29, 2025

jjtolton closed this Nov 29, 2025

jjtolton changed the title ~~Fix (|) parsing per ISO TC2 C2: operator-dependent validity~~ Fix #3170: Reject (|) when | declared as operator [SUPERSEDED] Nov 29, 2025

Fix #3170: Reject (|) when | declared as operator [SUPERSEDED] #3172

Fix #3170: Reject (|) when | declared as operator [SUPERSEDED] #3172

Uh oh!

Conversation

jjtolton commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Status: SUPERSEDED by #3141

Uh oh!

jjtolton commented Nov 22, 2025

Uh oh!

jjtolton commented Nov 23, 2025

Uh oh!

UWN commented Nov 24, 2025

Uh oh!

UWN commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UWN commented Nov 24, 2025

Uh oh!

UWN commented Nov 24, 2025

Uh oh!

jjtolton commented Nov 24, 2025

Uh oh!

jjtolton commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UWN commented Nov 25, 2025

Uh oh!

UWN commented Nov 25, 2025

Uh oh!

UWN commented Nov 25, 2025

Uh oh!

jjtolton commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UWN commented Nov 25, 2025

Uh oh!

jjtolton commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UWN commented Nov 25, 2025

Uh oh!

jjtolton commented Nov 25, 2025

Uh oh!

UWN commented Nov 25, 2025

Uh oh!

UWN commented Nov 25, 2025

Uh oh!

UWN commented Nov 25, 2025

Uh oh!

UWN commented Nov 25, 2025

Uh oh!

jjtolton commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjtolton commented Nov 27, 2025

Uh oh!

UWN commented Nov 27, 2025

Uh oh!

jjtolton commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UWN commented Nov 27, 2025

Uh oh!

jjtolton commented Nov 27, 2025

Uh oh!

jjtolton commented Nov 27, 2025

Uh oh!

UWN commented Nov 27, 2025

Uh oh!

jjtolton commented Nov 27, 2025

Uh oh!

UWN commented Nov 27, 2025

Uh oh!

jjtolton commented Nov 27, 2025

Uh oh!

UWN commented Nov 27, 2025

Uh oh!

jjtolton commented Nov 29, 2025

Uh oh!

jjtolton commented Nov 21, 2025 •

edited

Loading

UWN commented Nov 24, 2025 •

edited

Loading

jjtolton commented Nov 25, 2025 •

edited

Loading

jjtolton commented Nov 25, 2025 •

edited

Loading

jjtolton commented Nov 25, 2025 •

edited

Loading

jjtolton commented Nov 27, 2025 •

edited

Loading

jjtolton commented Nov 27, 2025 •

edited

Loading