Skip to content

Commit fee97ea

Browse files
committed
RequirementMachine: Fix the most embarassing bug of all time
The implementation of Knuth-Bendix completion has had a subtle bookkeeping bug since I first wrote the code in 2021. It is possible for two rules to overlap in more than one position, but the ResolvedOverlaps set was a set of pairs (i, j), where i and j are the index of the two rules. So overlaps other than the first were not considered. Fix this by changing ResolvedOverlaps to a set of triples (i, j, k), where k is the position in the left-hand side of the first rule. The end result is that we would incorrectly accept the protocol M3 shown in the test case. I'm pretty sure the monoid that M3 encodes does not have a complete presentation over any alphabet, so of course it should not be accepted here.
1 parent bf3f4a6 commit fee97ea

File tree

3 files changed

+6
-4
lines changed

3 files changed

+6
-4
lines changed

lib/AST/RequirementMachine/KnuthBendix.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -399,7 +399,8 @@ RewriteSystem::performKnuthBendix(unsigned maxRuleCount,
399399

400400
// We don't have to consider the same pair of rules more than once,
401401
// since those critical pairs were already resolved.
402-
if (!CheckedOverlaps.insert(std::make_pair(i, j)).second)
402+
unsigned k = from - lhs.getLHS().begin();
403+
if (!CheckedOverlaps.insert(std::make_tuple(i, j, k)).second)
403404
return;
404405

405406
// Try to repair the confluence violation by adding a new rule.

lib/AST/RequirementMachine/RewriteSystem.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ class RewriteSystem final {
219219
//////////////////////////////////////////////////////////////////////////////
220220

221221
/// Pairs of rules which have already been checked for overlap.
222-
llvm::DenseSet<std::pair<unsigned, unsigned>> CheckedOverlaps;
222+
llvm::DenseSet<std::tuple<unsigned, unsigned, unsigned>> CheckedOverlaps;
223223

224224
std::pair<CompletionResult, unsigned>
225225
performKnuthBendix(unsigned maxRuleCount, unsigned maxRuleLength);

test/Generics/non_confluent.swift

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,11 +99,12 @@ protocol M2 {
9999
where B.A == A.B, C.A == A, A.C == A // expected-error *{{is not a member type}}
100100
}
101101

102-
// FIXME: This should be rejected
103102
protocol M3 {
103+
// expected-error@-1 {{cannot build rewrite system for protocol; rule length limit exceeded}}
104+
// expected-note@-2 {{failed rewrite rule is }}
104105
associatedtype A: M3
105106
associatedtype B: M3
106-
where A.A.A == A, A.B.B.A == B.B
107+
where A.A.A == A, A.B.B.A == B.B // expected-error *{{is not a member type}}
107108
}
108109

109110
protocol M4 {

0 commit comments

Comments
 (0)