Skip to content

Conversation

@jjtolton
Copy link
Contributor

@jjtolton jjtolton commented Nov 22, 2025

Summary

Fixes #3151 - setof/3 was creating duplicate groups for variant witnesses

Problem

When using setof/3 with witness variables, variants with the same structure were being grouped separately instead of together.

Test case:

p([_A,_A,_B]).
p([_A,_B,_A]).
p([_A,_A,_B]).

?- countall(p(_),M), countall(setof(t,p(L),_),N), write((M,N)), nl.

Expected: 3,2 (3 clauses, 2 unique variant groups)
Actual (before fix): 3,3 (incorrectly counted 3 groups)

Clauses 1 and 3 have the same variant structure [X,X,Y] and should be grouped together, but were being counted as separate groups.

Root Cause

The group_by_variant/4 predicate in src/lib/builtins.pl only grouped consecutive variants after sorting with $keysort_with_constant_var_ordering/2.

With VarComparison::Indistinct, all variable terms compare as equal and the sort is stable (maintains original order). This means variants with the same structure like [X,X,Y] from clauses 1 and 3 can be separated by different structures like [X,Y,X] from clause 2 in the sorted list.

The original implementation only checked the next element in the list, so it would miss variants that appeared later.

Solution

Modified group_by_variant/4 (src/lib/builtins.pl:993-1001) to recursively scan the ENTIRE list for ALL variants, not just consecutive elements.

Tail-recursive implementation:

group_by_variant([V2-S2 | Pairs], V1-S1, Solutions, Pairs0) :-
    (  variant(V1, V2) ->
       V1 = V2,
       Solutions = [S2 | MoreSolutions],
       group_by_variant(Pairs, V1-S1, MoreSolutions, Pairs0)
    ;  Pairs0 = [V2-S2 | RestPairs],
       group_by_variant(Pairs, V1-S1, Solutions, RestPairs)
    ).
group_by_variant([], _, [], []).

The key insight: in the else branch, we unify Pairs0 with [V2-S2 | RestPairs] before the recursive call, making it tail-recursive.

Test Plan

  • Added failing test following TDD methodology (tests/scryer/cli/issues/issue_3151.*)
  • Implemented fix (TDD Green phase)
  • Verified test now passes with correct output 3,2
  • Made predicate tail-recursive for better performance
  • All existing tests still passing

Commits

  1. 7de9d0d4 Add failing test for setof/3 duplicate grouping bug
  2. 5a204fe7 Fix setof/3 duplicate grouping bug (issue setof/3 problem #3151)
  3. 86ba395c Make group_by_variant/4 tail-recursive
  • J.J.'s Robot

Issue mthom#3151: setof/3 incorrectly creates duplicate groups for variant
witnesses. Test demonstrates that 3 clauses with 2 unique variant
patterns incorrectly produce 3 groups instead of 2.

Expected: M=3, N=2 (3 clauses, 2 unique variant groups)
Actual: M=3, N=3 (incorrectly counts 3 groups)

- J.J.'s Robot
Modified group_by_variant/4 to scan entire list for variants instead of
only grouping consecutive elements. With VarComparison::Indistinct, all
variable terms compare equal but maintain original order, so variants
with same structure can be separated by different structures.

Fix: recursively scan full list, collecting ALL variants regardless of
position. Non-variants are preserved in Pairs0 for next group.

src/lib/builtins.pl:993-1001

- J.J.'s Robot
Moved Pairs0 construction before recursive call in else branch.
This ensures the predicate is tail-recursive by avoiding work
after the recursive call returns.

src/lib/builtins.pl:998-999

- J.J.'s Robot
@jjtolton
Copy link
Contributor Author

Test Results

No CI - ran locally:

  • ✅ Issue setof/3 problem #3151 test passes (correct output: 3,2)

  • ✅ All 36 existing tests pass

  • ✅ No regressions

  • J.J.'s Robot

@UWN UWN mentioned this pull request Nov 26, 2025
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.

setof/3 problem

1 participant