Skip to content

Commit 338cc32

Browse files
xedinxedin
authored
Merge pull request #39777 from xedin/restore-score-based-minimalization
[CSRanking] Allow worse (score-wise) solutions to be removed by filtering
2 parents e2cb7e7 + 5ac8bce commit 338cc32

File tree

3 files changed

+149
-41
lines changed

3 files changed

+149
-41
lines changed

‎lib/Sema/CSRanking.cpp

Lines changed: 9 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1368,8 +1368,14 @@ ConstraintSystem::findBestSolution(SmallVectorImpl<Solution> &viable,
13681368

13691369
// Find a potential best.
13701370
SmallVector<bool, 16> losers(viable.size(), false);
1371+
Score bestScore = viable.front().getFixedScore();
13711372
unsigned bestIdx = 0;
13721373
for (unsigned i = 1, n = viable.size(); i != n; ++i) {
1374+
auto currScore = viable[i].getFixedScore();
1375+
1376+
if (currScore < bestScore)
1377+
bestScore = currScore;
1378+
13731379
switch (compareSolutions(*this, viable, diff, i, bestIdx)) {
13741380
case SolutionCompareResult::Identical:
13751381
// FIXME: Might want to warn about this in debug builds, so we can
@@ -1424,52 +1430,14 @@ ConstraintSystem::findBestSolution(SmallVectorImpl<Solution> &viable,
14241430
return bestIdx;
14251431
}
14261432

1427-
// If there is not a single "better" than others
1428-
// solution, which probably means that solutions
1429-
// were incomparable, let's just keep the original
1430-
// list instead of removing everything, even if we
1431-
// are asked to "minimize" the result.
1432-
if (losers.size() == viable.size())
1433+
if (!minimize)
14331434
return None;
14341435

1435-
// The comparison was ambiguous. Identify any solutions that are worse than
1436-
// any other solution.
1437-
for (unsigned i = 0, n = viable.size(); i != n; ++i) {
1438-
// If the first solution has already lost once, don't bother looking
1439-
// further.
1440-
if (losers[i])
1441-
continue;
1442-
1443-
for (unsigned j = i + 1; j != n; ++j) {
1444-
// If the second solution has already lost once, don't bother looking
1445-
// further.
1446-
if (losers[j])
1447-
continue;
1448-
1449-
switch (compareSolutions(*this, viable, diff, i, j)) {
1450-
case SolutionCompareResult::Identical:
1451-
// FIXME: Dub one of these the loser arbitrarily?
1452-
break;
1453-
1454-
case SolutionCompareResult::Better:
1455-
losers[j] = true;
1456-
break;
1457-
1458-
case SolutionCompareResult::Worse:
1459-
losers[i] = true;
1460-
break;
1461-
1462-
case SolutionCompareResult::Incomparable:
1463-
break;
1464-
}
1465-
}
1466-
}
1467-
14681436
// Remove any solution that is worse than some other solution.
14691437
unsigned outIndex = 0;
14701438
for (unsigned i = 0, n = viable.size(); i != n; ++i) {
14711439
// Skip over the losing solutions.
1472-
if (losers[i])
1440+
if (viable[i].getFixedScore() > bestScore)
14731441
continue;
14741442

14751443
// If we have skipped any solutions, move this solution into the next
@@ -1479,6 +1447,7 @@ ConstraintSystem::findBestSolution(SmallVectorImpl<Solution> &viable,
14791447

14801448
++outIndex;
14811449
}
1450+
14821451
viable.erase(viable.begin() + outIndex, viable.end());
14831452
NumDiscardedSolutions += viable.size() - outIndex;
14841453

‎unittests/Sema/CMakeLists.txt

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@ add_swift_unittest(swiftSemaTests
44
BindingInferenceTests.cpp
55
ConstraintSimplificationTests.cpp
66
UnresolvedMemberLookupTests.cpp
7-
PlaceholderTypeInferenceTests.cpp)
7+
PlaceholderTypeInferenceTests.cpp
8+
SolutionFilteringTests.cpp)
89

910
target_link_libraries(swiftSemaTests
1011
PRIVATE

‎unittests/Sema/SolutionFilteringTests.cpp

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
//===--- SolutionFilteringTests.cpp ---------------------------------------===//
2+
//
3+
// This source file is part of the Swift.org open source project
4+
//
5+
// Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors
6+
// Licensed under Apache License v2.0 with Runtime Library Exception
7+
//
8+
// See https://swift.org/LICENSE.txt for license information
9+
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
10+
//
11+
//===----------------------------------------------------------------------===//
12+
13+
#include "SemaFixture.h"
14+
#include "swift/Sema/ConstraintSystem.h"
15+
16+
using namespace swift;
17+
using namespace swift::unittest;
18+
using namespace swift::constraints;
19+
20+
/// Make sure that minimization works as expected
21+
/// and would remove solutions that score worse
22+
/// than the best set even in ambiguity cases
23+
/// (where there is more than one best solution).
24+
TEST_F(SemaTest, TestFilteringBasedOnSolutionScore) {
25+
Score bestScore{};
26+
27+
Score worseScore1{};
28+
Score worseScore2{};
29+
30+
worseScore1.Data[SK_Unavailable] += 1;
31+
worseScore2.Data[SK_Fix] += 1;
32+
33+
ConstraintSystem CS(DC, ConstraintSystemOptions());
34+
35+
// Let's test worse solutions in different positions
36+
// in the list - beginning, middle, end.
37+
38+
// To make "best" solutions ambiguous, let's use a single
39+
// type variable resolved into multiple distinct types.
40+
auto *ambiguousVar = CS.createTypeVariable(
41+
CS.getConstraintLocator({}), /*options=*/TVO_PrefersSubtypeBinding);
42+
43+
auto formSolution = [&](Score score, Type type) -> Solution {
44+
Solution solution{CS, score};
45+
solution.typeBindings[ambiguousVar] = type;
46+
return solution;
47+
};
48+
49+
// Nothing to remove
50+
{
51+
SmallVector<Solution, 4> solutions;
52+
53+
solutions.push_back(formSolution(bestScore, getStdlibType("String")));
54+
solutions.push_back(formSolution(bestScore, getStdlibType("Int")));
55+
56+
auto best = CS.findBestSolution(solutions, /*minimize=*/true);
57+
58+
ASSERT_FALSE(best.hasValue());
59+
ASSERT_EQ(solutions.size(), 2u);
60+
ASSERT_EQ(solutions[0].getFixedScore(), bestScore);
61+
ASSERT_EQ(solutions[1].getFixedScore(), bestScore);
62+
}
63+
64+
// Worst solutions are at the front
65+
{
66+
SmallVector<Solution, 4> solutions;
67+
68+
solutions.push_back(formSolution(worseScore1, getStdlibType("Int")));
69+
solutions.push_back(formSolution(worseScore2, getStdlibType("Int")));
70+
71+
solutions.push_back(formSolution(bestScore, getStdlibType("String")));
72+
solutions.push_back(formSolution(bestScore, getStdlibType("Int")));
73+
74+
auto best = CS.findBestSolution(solutions, /*minimize=*/true);
75+
76+
ASSERT_FALSE(best.hasValue());
77+
ASSERT_EQ(solutions.size(), 2u);
78+
ASSERT_EQ(solutions[0].getFixedScore(), bestScore);
79+
ASSERT_EQ(solutions[1].getFixedScore(), bestScore);
80+
}
81+
82+
// Worst solutions are in the middle
83+
{
84+
SmallVector<Solution, 4> solutions;
85+
86+
solutions.push_back(formSolution(bestScore, getStdlibType("String")));
87+
88+
solutions.push_back(formSolution(worseScore1, getStdlibType("Int")));
89+
solutions.push_back(formSolution(worseScore2, getStdlibType("Int")));
90+
91+
solutions.push_back(formSolution(bestScore, getStdlibType("Int")));
92+
93+
auto best = CS.findBestSolution(solutions, /*minimize=*/true);
94+
95+
ASSERT_FALSE(best.hasValue());
96+
ASSERT_EQ(solutions.size(), 2u);
97+
ASSERT_EQ(solutions[0].getFixedScore(), bestScore);
98+
ASSERT_EQ(solutions[1].getFixedScore(), bestScore);
99+
}
100+
101+
// Worst solutions are at the end
102+
{
103+
SmallVector<Solution, 4> solutions;
104+
105+
solutions.push_back(formSolution(bestScore, getStdlibType("String")));
106+
solutions.push_back(formSolution(bestScore, getStdlibType("Int")));
107+
108+
solutions.push_back(formSolution(worseScore1, getStdlibType("Int")));
109+
solutions.push_back(formSolution(worseScore2, getStdlibType("Int")));
110+
111+
auto best = CS.findBestSolution(solutions, /*minimize=*/true);
112+
113+
ASSERT_FALSE(best.hasValue());
114+
ASSERT_EQ(solutions.size(), 2u);
115+
ASSERT_EQ(solutions[0].getFixedScore(), bestScore);
116+
ASSERT_EQ(solutions[1].getFixedScore(), bestScore);
117+
}
118+
119+
// Worst solutions are spread out
120+
{
121+
SmallVector<Solution, 4> solutions;
122+
123+
solutions.push_back(formSolution(worseScore1, getStdlibType("Int")));
124+
solutions.push_back(formSolution(bestScore, getStdlibType("String")));
125+
solutions.push_back(formSolution(worseScore1, getStdlibType("Int")));
126+
solutions.push_back(formSolution(worseScore2, getStdlibType("Int")));
127+
solutions.push_back(formSolution(bestScore, getStdlibType("Int")));
128+
solutions.push_back(formSolution(worseScore1, getStdlibType("Int")));
129+
solutions.push_back(formSolution(worseScore2, getStdlibType("Int")));
130+
131+
auto best = CS.findBestSolution(solutions, /*minimize=*/true);
132+
133+
ASSERT_FALSE(best.hasValue());
134+
ASSERT_EQ(solutions.size(), 2u);
135+
ASSERT_EQ(solutions[0].getFixedScore(), bestScore);
136+
ASSERT_EQ(solutions[1].getFixedScore(), bestScore);
137+
}
138+
}

0 commit comments

Comments
 (0)