Skip to content

Commit 83a79a5

Browse files
xedinxedin
authored
Merge pull request #39747 from xedin/multi-stmt-closure-diag-fixes
[CSStep] Conjunction: Stop after first ambiguous or fixed element
2 parents cbb69ce + 42b1a39 commit 83a79a5

File tree

4 files changed

+226
-25
lines changed

4 files changed

+226
-25
lines changed

‎include/swift/Sema/ConstraintSystem.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5886,6 +5886,10 @@ class ConjunctionElementProducer : public BindingProducer<ConjunctionElement> {
58865886
bool needsToComputeNext() const override { return false; }
58875887

58885888
bool isExhausted() const override { return Index >= Elements.size(); }
5889+
5890+
void markExhausted() {
5891+
Index = Elements.size();
5892+
}
58895893
};
58905894

58915895
/// Determine whether given type is a known one

‎lib/Sema/CSStep.cpp‎

Lines changed: 74 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -849,11 +849,29 @@ StepResult ConjunctionStep::resume(bool prevFailed) {
849849

850850
// There could be a local ambiguity related to
851851
// the current element, let's try to resolve it.
852-
if (Solutions.size() > 1) {
852+
if (Solutions.size() > 1)
853853
filterSolutions(Solutions, /*minimize=*/true);
854854

855-
if (Solutions.size() != 1)
856-
return failConjunction();
855+
// In diagnostic mode we need to stop a conjunction
856+
// but consider it successful if there are:
857+
//
858+
// - More than one solution for this element. Ambiguity
859+
// needs to get propagated back to the outer context
860+
// to be diagnosed.
861+
// - A single solution that requires one or more fixes,
862+
// continuing would result in more errors associated
863+
// with the failed element.
864+
if (CS.shouldAttemptFixes()) {
865+
if (Solutions.size() > 1)
866+
Producer.markExhausted();
867+
868+
if (Solutions.size() == 1) {
869+
auto score = Solutions.front().getFixedScore();
870+
if (score.Data[SK_Fix] > 0)
871+
Producer.markExhausted();
872+
}
873+
} else if (Solutions.size() != 1) {
874+
return failConjunction();
857875
}
858876

859877
// Since there is only one solution, let's
@@ -885,34 +903,44 @@ StepResult ConjunctionStep::resume(bool prevFailed) {
885903
log << "(applying conjunction result to outer context\n";
886904
}
887905

888-
// Restore constraint system state before conjunction.
889-
//
890-
// Note that this doesn't include conjunction constraint
891-
// itself because we don't want to re-solve it at this
892-
// point.
893906
assert(
894907
Snapshot &&
895908
"Isolated conjunction requires a snapshot of the constraint system");
896-
Snapshot->setupOuterContext(Solutions.pop_back_val());
897909

898-
// Restore best score, since upcoming step is going to
899-
// work with outer scope in relation to the conjunction.
900-
CS.solverState->BestScore = BestScore;
901-
902-
// Active all of the previously out-of-scope constraints
903-
// because conjunction can propagate type information up
904-
// by allowing its elements to reference type variables
905-
// from outer scope (e.g. variable declarations and or captures).
906-
{
907-
CS.ActiveConstraints.splice(CS.ActiveConstraints.end(),
908-
CS.InactiveConstraints);
909-
for (auto &constraint : CS.ActiveConstraints)
910-
constraint.setActive(true);
910+
// In diagnostic mode it's valid for an element to have
911+
// multiple solutions. Ambiguity just needs to be merged
912+
// into the outer context to be property diagnosed.
913+
if (Solutions.size() > 1) {
914+
assert(CS.shouldAttemptFixes());
915+
916+
// Restore all outer type variables, constraints
917+
// and scoring information.
918+
Snapshot.reset();
919+
restoreOuterState();
920+
921+
// Apply all of the information deduced from the
922+
// conjunction (up to the point of ambiguity)
923+
// back to the outer context and form a joined solution.
924+
for (auto &solution : Solutions) {
925+
ConstraintSystem::SolverScope scope(CS);
926+
927+
CS.applySolution(solution);
928+
// Note that `worseThanBestSolution` isn't checked
929+
// here because `Solutions` were pre-filtered, and
930+
// outer score is the same for all of them.
931+
OuterSolutions.push_back(CS.finalize());
932+
}
933+
934+
return done(/*isSuccess=*/true);
911935
}
912936

913-
// Restore score to the one before conjunction. This has
914-
// be done after solution, reached for the body, is applied.
915-
CS.CurrentScore = CurrentScore;
937+
// Restore outer type variables and prepare to solve
938+
// constraints associated with outer context together
939+
// with information deduced from the conjunction.
940+
Snapshot->setupOuterContext(Solutions.pop_back_val());
941+
942+
// Pretend that conjunction never happend.
943+
restoreOuterState();
916944

917945
// Now that all of the information from the conjunction has
918946
// been applied, let's attempt to solve the outer scope.
@@ -923,3 +951,24 @@ StepResult ConjunctionStep::resume(bool prevFailed) {
923951
// Attempt next conjunction choice.
924952
return take(prevFailed);
925953
}
954+
955+
void ConjunctionStep::restoreOuterState() const {
956+
// Restore best score, since upcoming step is going to
957+
// work with outer scope in relation to the conjunction.
958+
CS.solverState->BestScore = BestScore;
959+
960+
// Active all of the previously out-of-scope constraints
961+
// because conjunction can propagate type information up
962+
// by allowing its elements to reference type variables
963+
// from outer scope (e.g. variable declarations and or captures).
964+
{
965+
CS.ActiveConstraints.splice(CS.ActiveConstraints.end(),
966+
CS.InactiveConstraints);
967+
for (auto &constraint : CS.ActiveConstraints)
968+
constraint.setActive(true);
969+
}
970+
971+
// Restore score to the one before conjunction. This has
972+
// be done after solution, reached for the body, is applied.
973+
CS.CurrentScore = CurrentScore;
974+
}

‎lib/Sema/CSStep.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -949,6 +949,13 @@ class ConjunctionStep : public BindingStep<ConjunctionElementProducer> {
949949
// successfully by use of fixes, and ignore the rest.
950950
AnySolved = false;
951951
}
952+
953+
private:
954+
// Restore constraint system state before conjunction.
955+
//
956+
// Note that this doesn't include conjunction constraint
957+
// itself because we don't want to re-solve it.
958+
void restoreOuterState() const;
952959
};
953960

954961
} // end namespace constraints

‎test/expr/closure/multi_statement.swift‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
// RUN: %target-typecheck-verify-swift -swift-version 5 -experimental-multi-statement-closures -enable-experimental-static-assert
2+
3+
func isInt<T>(_ value: T) -> Bool {
4+
return value is Int
5+
}
6+
7+
func maybeGetValue<T>(_ value: T) -> T? {
8+
return value
9+
}
10+
11+
enum MyError: Error {
12+
case featureIsTooCool
13+
14+
func doIt() { }
15+
}
16+
17+
enum State {
18+
case suspended
19+
case partial(Int, Int)
20+
case finished
21+
}
22+
23+
func random(_: Int) -> Bool { return false }
24+
25+
func mightThrow() throws -> Bool { throw MyError.featureIsTooCool }
26+
27+
func mapWithMoreStatements(ints: [Int], state: State) throws {
28+
let _ = try ints.map { i in
29+
guard var actualValue = maybeGetValue(i) else {
30+
return String(0)
31+
}
32+
33+
let value = actualValue + 1
34+
do {
35+
if isInt(i) {
36+
print(value)
37+
} else if value == 17 {
38+
print("seventeen!")
39+
}
40+
}
41+
42+
while actualValue < 100 {
43+
actualValue += 1
44+
}
45+
46+
my_repeat:
47+
repeat {
48+
print("still here")
49+
50+
if i % 7 == 0 {
51+
break my_repeat
52+
}
53+
} while random(i)
54+
55+
defer {
56+
print("I am so done here")
57+
}
58+
59+
for j in 0..<i where j % 2 == 0 {
60+
if j % 7 == 0 {
61+
continue
62+
}
63+
64+
switch (state, j) {
65+
case (.suspended, 0):
66+
print("something")
67+
fallthrough
68+
case (.finished, 0):
69+
print("something else")
70+
71+
case (.partial(let current, let end), let j):
72+
print("\(current) of \(end): \(j)")
73+
74+
default:
75+
print("so, here we are")
76+
}
77+
print("even")
78+
throw MyError.featureIsTooCool
79+
}
80+
81+
#assert(true)
82+
83+
// expected-warning@+1{{danger zone}}
84+
#warning("danger zone")
85+
86+
#if false
87+
struct NothingHere { }
88+
#else
89+
struct NestedStruct {
90+
var x: Int
91+
}
92+
#endif
93+
94+
do {
95+
print(try mightThrow())
96+
} catch let e as MyError {
97+
e.doIt()
98+
} catch {
99+
print(error)
100+
}
101+
return String(value)
102+
}
103+
}
104+
105+
func acceptsWhateverClosure<T, R>(_ value: T, _ fn: (T) -> R) { }
106+
107+
func testReturnWithoutExpr(i: Int) {
108+
acceptsWhateverClosure(i) { i in
109+
print(i)
110+
return
111+
}
112+
}
113+
114+
// `withContiguousStorageIfAvailable` is overloaded, so let's make sure that
115+
// filtering works correctly.
116+
func test_overloaded_call(arr: [Int], body: (UnsafeBufferPointer<Int>) -> Void) -> Void {
117+
arr.withContiguousStorageIfAvailable { buffer in
118+
let _ = type(of: buffer)
119+
body(buffer) // ok
120+
}
121+
}
122+
123+
// Used to wrap closure in `FunctionConversionExpr` in this case,
124+
// but now solver would just inject return expression into optional where necessary.
125+
func test_result_optional_injection() {
126+
func fn<T>(_: () -> T?) -> [T] {
127+
[]
128+
}
129+
130+
_ = fn {
131+
if true {
132+
return // Ok
133+
}
134+
}
135+
}
136+
137+
let _ = {
138+
for i: Int8 in 0 ..< 20 { // Ok (pattern can inform a type of the sequence)
139+
print(i)
140+
}
141+
}

0 commit comments

Comments
 (0)