Skip to content

satore: robust confluence — one-literal critical pairs and max-steps=1 - #741

Open
PratikDhanave wants to merge 1 commit into
google-deepmind:masterfrom
PratikDhanaveFork:fix/satore-confluence-robustness
Open

satore: robust confluence — one-literal critical pairs and max-steps=1#741
PratikDhanave wants to merge 1 commit into
google-deepmind:masterfrom
PratikDhanaveFork:fix/satore-confluence-robustness

Conversation

@PratikDhanave

Copy link
Copy Markdown

Summary

Two fixes in the rewrite-tree confluence code (satore/rewrite-tree.rkt).

1. find-critical-pairs crashes on a one-literal critical pair

The bounded-size filter hard-codes two literals:

(> (max (literal-size (first cl)) (literal-size (second cl))) max-size)

But cl is (clause-normalize (substitute (list to-lit1 to-lit2) s)), and safe-factoring (inside clause-normalize) can collapse the two to literals into a single literal — e.g. rules p(X) -> q(X) and ~p(Y) -> q(Y) produce the resolvent [q(Y) q(Y)], which normalizes to [q(Y)]. (second cl) then raises an index error.

Fix — take the max literal-size over the whole clause (identical to the old behavior for two-literal clauses, and matching the docstring "a literal heavier than a from literal"):

(> (apply max 0 (map literal-size cl)) max-size)

2. rewrite-tree-confluence! drops the final step's change from its result

The terminating branch returns any-change?, which never OR-s in the current iteration's changed?. When the loop stops because of max-steps (e.g. max-steps = 1) after a step that did add a rule, it wrongly returns #false, contradicting the docstring "Returns whether any change has been made." Fixed with (or any-change? changed?).

Testing

Verified by static tracing (clause-normalize/safe-factoring can reduce a 2-literal resolvent to 1 literal; the max-steps = 1 boundary returns before OR-ing the change). No Racket toolchain available locally; reviewers can run raco test satore/tests/.

…teps=1

Two fixes in the rewrite-tree confluence code:

1. find-critical-pairs: the bounded-size filter used
   (max (literal-size (first cl)) (literal-size (second cl))), hard-coding two
   literals. But `cl` is (clause-normalize (substitute (list to-lit1 to-lit2) s)),
   and safe-factoring can collapse the two `to` literals into a single literal
   (e.g. rules p(X)->q(X) and ~p(Y)->q(Y) resolve to [q(Y) q(Y)] -> [q(Y)]).
   `(second cl)` then raises an index error. Take the max literal-size over the
   whole clause instead; identical to the old behavior for two-literal clauses,
   and matches the docstring ("a literal heavier than a `from` literal").

2. rewrite-tree-confluence!: the terminating branch returned `any-change?`,
   which omits the current iteration's `changed?`. When the loop stops because of
   max-steps (e.g. max-steps = 1) after making a change, it wrongly reported "no
   change". Return (or any-change? changed?). Contradicts the docstring "Returns
   whether any change has been made."
@polarbe

polarbe commented Jul 25, 2026 via email

Copy link
Copy Markdown

@PratikDhanave

Copy link
Copy Markdown
Author

satore: makes confluence robust (one-literal critical pairs, max-steps=1). CI green — ready for review.

@polarbe

polarbe commented Aug 21, 2026 via email

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants