Skip to content

Commit 4dda0ac

Browse files
slavapestovslavapestov
authored
Merge pull request #42176 from slavapestov/rqm-property-like-rules
RequirementMachine: Try harder to ensure completion only introduces property-like or same-type rules
2 parents cc26e15 + 85e73ed commit 4dda0ac

File tree

4 files changed

+161
-15
lines changed

4 files changed

+161
-15
lines changed

‎lib/AST/RequirementMachine/KnuthBendix.cpp‎

Lines changed: 65 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -157,8 +157,66 @@ RewriteSystem::computeCriticalPair(ArrayRef<Symbol>::const_iterator from,
157157
return false;
158158
}
159159

160-
// Add the pair (X, TYV).
161-
pairs.emplace_back(x, tyv, path);
160+
// If X == TUW for some W, then the critical pair is (TUW, TYV),
161+
// and we have
162+
// - lhs == (TUV => TUW)
163+
// - rhs == (U => Y).
164+
//
165+
// We explicitly apply the rewrite step (Y => U) to the beginning of the
166+
// rewrite path, transforming the critical pair to (TYW, TYV).
167+
//
168+
// In particular, if V == W.[P] for some protocol P, then we in fact have
169+
// a property rule and a same-type rule:
170+
//
171+
// - lhs == (TUW.[P] => TUW)
172+
// - rhs == (U => Y)
173+
//
174+
// Without this hack, the critical pair would be:
175+
//
176+
// (TUW => TYW.[P])
177+
//
178+
// With this hack, the critical pair becomes:
179+
//
180+
// (TYW.[P] => TYW)
181+
//
182+
// This ensures that the newly-added rule is itself a property rule;
183+
// otherwise, this would only be the case if addRule() reduced TUW
184+
// into TYW without immediately reducing some subterm of TUW first.
185+
//
186+
// While completion will eventually simplify all such rules down into
187+
// property rules, their existance in the first place breaks subtle
188+
// invariants in the minimal conformances algorithm, which expects
189+
// homotopy generators describing redundant protocol conformance rules
190+
// to have a certain structure.
191+
if (t.size() + rhs.getLHS().size() <= x.size() &&
192+
std::equal(rhs.getLHS().begin(),
193+
rhs.getLHS().end(),
194+
x.begin() + t.size())) {
195+
// We have a path from TUW to TYV. Invert to get a path from TYV to
196+
// TUW.
197+
path.invert();
198+
199+
// Compute the term W.
200+
MutableTerm w(x.begin() + t.size() + rhs.getLHS().size(), x.end());
201+
202+
// Now add a rewrite step T.(U => Y).W to get a path from TYV to
203+
// TYW.
204+
path.add(RewriteStep::forRewriteRule(/*startOffset=*/t.size(),
205+
/*endOffset=*/w.size(),
206+
getRuleID(rhs),
207+
/*inverse=*/false));
208+
209+
// Compute the term TYW.
210+
MutableTerm tyw(t);
211+
tyw.append(rhs.getRHS());
212+
tyw.append(w);
213+
214+
// Add the pair (TYV, TYW).
215+
pairs.emplace_back(tyv, tyw, path);
216+
} else {
217+
// Add the pair (X, TYV).
218+
pairs.emplace_back(x, tyv, path);
219+
}
162220
} else {
163221
// lhs == TU -> X, rhs == UV -> Y.
164222

@@ -217,7 +275,7 @@ RewriteSystem::computeCriticalPair(ArrayRef<Symbol>::const_iterator from,
217275
// - lhs == (TU -> X)
218276
// - rhs == (UV -> UW).
219277
//
220-
// We explicitly apply the rewrite step (TU = X) to the rewrite path,
278+
// We explicitly apply the rewrite step (TU => X) to the rewrite path,
221279
// transforming the critical pair to (XV, XW).
222280
//
223281
// In particular, if T == X, U == [P] for some protocol P, and
@@ -229,15 +287,15 @@ RewriteSystem::computeCriticalPair(ArrayRef<Symbol>::const_iterator from,
229287
//
230288
// Without this hack, the critical pair would be:
231289
//
232-
// (T.w.[p] => T.[P].w)
290+
// (T.W.[p] => T.[P].W)
233291
//
234292
// With this hack, the critical pair becomes:
235293
//
236-
// (T.w.[p] => T.w)
294+
// (T.W.[p] => T.W)
237295
//
238296
// This ensures that the newly-added rule is itself a property rule;
239-
// otherwise, this would only be the case if addRule() reduced T.[P].w
240-
// into T.w without immediately reducing some subterm of T first.
297+
// otherwise, this would only be the case if addRule() reduced T.[P].W
298+
// into T.W without immediately reducing some subterm of T first.
241299
//
242300
// While completion will eventually simplify all such rules down into
243301
// property rules, their existance in the first place breaks subtle

‎lib/AST/RequirementMachine/RequirementMachine.cpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -485,6 +485,10 @@ RequirementMachine::computeCompletion(RewriteSystem::ValidityPolicy policy) {
485485

486486
unsigned rulesAdded = (System.getRules().size() - ruleCount);
487487

488+
// If buildPropertyMap() didn't add any new rules, we are done.
489+
if (rulesAdded == 0)
490+
break;
491+
488492
if (Stats) {
489493
Stats->getFrontendCounters()
490494
.NumRequirementMachineUnifiedConcreteTerms += rulesAdded;
@@ -507,10 +511,6 @@ RequirementMachine::computeCompletion(RewriteSystem::ValidityPolicy policy) {
507511
return std::make_pair(CompletionResult::MaxRuleCount,
508512
System.getRules().size() - 1);
509513
}
510-
511-
// If buildPropertyMap() didn't add any new rules, we are done.
512-
if (rulesAdded == 0)
513-
break;
514514
}
515515
}
516516

‎lib/AST/RequirementMachine/RewriteSystem.cpp‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -591,10 +591,7 @@ void RewriteSystem::verifyRewriteRules(ValidityPolicy policy) const {
591591
ASSERT_RULE(symbol.getKind() != Symbol::Kind::GenericParam);
592592
}
593593

594-
// Completion can produce rules like [P:T].[Q:R] => [P:T].[Q]
595-
// which are immediately simplified away.
596-
if (!rule.isRHSSimplified() &&
597-
index != 0) {
594+
if (index != 0) {
598595
ASSERT_RULE(symbol.getKind() != Symbol::Kind::Protocol);
599596
}
600597
}

‎test/Generics/rdar91232987.swift‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
// RUN: %target-swift-frontend -typecheck %s -debug-generic-signatures 2>&1 | %FileCheck %s
2+
3+
// CHECK-LABEL: rdar91232987.(file).P1@
4+
// CHECK-NEXT: Requirement signature: <Self where Self.[P1]A1 : Decodable, Self.[P1]A1 : Encodable, Self.[P1]A1 : P2, Self.[P1]A1.[P2]A3.[P5]A6 : Decodable, Self.[P1]A1.[P2]A3.[P5]A6 : Encodable>
5+
protocol P1 {
6+
associatedtype A1: P2 where A1: Codable, A1.A3.A6: Codable
7+
}
8+
9+
// CHECK-LABEL: rdar91232987.(file).P2@
10+
// CHECK-NEXT: Requirement signature: <Self where Self : P3, Self.[P2]A2 : P4, Self.[P2]A3 : P5>
11+
protocol P2: P3 {
12+
associatedtype A2: P4
13+
associatedtype A3: P5
14+
}
15+
16+
// CHECK-LABEL: rdar91232987.(file).P3@
17+
// CHECK-NEXT: Requirement signature: <Self>
18+
protocol P3 {}
19+
20+
// CHECK-LABEL: rdar91232987.(file).P4@
21+
// CHECK-NEXT: Requirement signature: <Self where Self.[P4]A4 : P11, Self.[P4]A5 : P9>
22+
protocol P4 {
23+
associatedtype A4: P11
24+
associatedtype A5: P9
25+
26+
}
27+
28+
// CHECK-LABEL: rdar91232987.(file).P5@
29+
// CHECK-NEXT: Requirement signature: <Self where Self.[P5]A6 : P6>
30+
protocol P5 {
31+
associatedtype A6: P6
32+
}
33+
34+
// CHECK-LABEL: rdar91232987.(file).P6@
35+
// CHECK-NEXT: Requirement signature: <Self where Self : P10, Self : P7, Self.[P6]A7 : Decodable, Self.[P6]A7 : Encodable, Self.[P6]A7 : Hashable, Self.[P6]A7 : RawRepresentable, Self.[P6]A7.[RawRepresentable]RawValue == String>
36+
protocol P6: P7, P10 {
37+
associatedtype A7: Hashable, Codable, RawRepresentable where A7.RawValue == String
38+
}
39+
40+
// CHECK-LABEL: rdar91232987.(file).P7@
41+
// CHECK-NEXT: Requirement signature: <Self where Self : Equatable, Self : P8>
42+
protocol P7: Equatable, P8 {}
43+
44+
// CHECK-LABEL: rdar91232987.(file).P8@
45+
// CHECK-NEXT: Requirement signature: <Self>
46+
protocol P8 {}
47+
48+
// CHECK-LABEL: rdar91232987.(file).P9@
49+
// CHECK-NEXT: Requirement signature: <Self>
50+
protocol P9 {
51+
associatedtype A8
52+
}
53+
54+
// CHECK-LABEL: rdar91232987.(file).P10@
55+
// CHECK-NEXT: Requirement signature: <Self>
56+
protocol P10 {}
57+
58+
// CHECK-LABEL: rdar91232987.(file).P11@
59+
// CHECK-NEXT: Requirement signature: <Self where Self.[P11]A10 : P13, Self.[P11]A9 : P12>
60+
protocol P11 {
61+
associatedtype A9: P12
62+
associatedtype A10: P13
63+
}
64+
65+
// CHECK-LABEL: rdar91232987.(file).P12@
66+
// CHECK-NEXT: Requirement signature: <Self where Self : Decodable, Self : Encodable, Self : Equatable>
67+
protocol P12: Codable, Equatable {}
68+
69+
// CHECK-LABEL: rdar91232987.(file).P13@
70+
// CHECK-NEXT: Requirement signature: <Self>
71+
protocol P13 {}
72+
73+
// CHECK-LABEL: rdar91232987.(file).P14@
74+
// CHECK-NEXT: Requirement signature: <Self where Self : P16, Self.[P14]A1 : P2>
75+
protocol P14: P16 {
76+
associatedtype A1: P2
77+
}
78+
79+
// CHECK-LABEL: rdar91232987.(file).P15@
80+
// CHECK-NEXT: Requirement signature: <Self where Self.[P15]A6 : P6>
81+
protocol P15 {
82+
associatedtype A6: P6
83+
}
84+
85+
// CHECK-LABEL: rdar91232987.(file).P16@
86+
// CHECK-NEXT: Requirement signature: <Self>
87+
protocol P16 {}
88+
89+
// CHECK-LABEL: rdar91232987.(file).C@
90+
// CHECK-NEXT: Generic signature: <X, Y where X : P14, Y : P15, X.[P14]A1 : Decodable, X.[P14]A1 : Encodable, Y.[P15]A6 : Decodable, Y.[P15]A6 : Encodable, Y.[P15]A6 == X.[P14]A1.[P2]A3.[P5]A6>
91+
class C<X: P14, Y: P15> where X.A1: Codable, X.A1.A3.A6: Codable, X.A1.A3.A6 == Y.A6 {}

0 commit comments

Comments
 (0)