Skip to content

Commit b7f4344

Browse files
committed
make MustBeCopyable::diagnoseForAmbiguity robust
Previous implementation blindly took the first solution and tried to diagnose, but that's not a safe assumption. It's possible that among the solution-fix pairs, one of `noncopyableTy`'s within the fix differs from the others. Things probably can go wrong because the solution doesn't correspond to that type. Also, in that case we'd be emitting a diagnostic that may not make any sense. In such cases, now we decline to emit a diagnostic. Thanks Pavel for catching this.
1 parent 83166e4 commit b7f4344

File tree

2 files changed

+21
-4
lines changed

2 files changed

+21
-4
lines changed

include/swift/Sema/CSFix.h

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2040,15 +2040,13 @@ class MustBeCopyable final : public ConstraintFix {
20402040

20412041
bool diagnose(const Solution &solution, bool asNote = false) const override;
20422042

2043-
bool diagnoseForAmbiguity(CommonFixesArray commonFixes) const override {
2044-
return diagnose(*commonFixes.front().first);
2045-
}
2043+
bool diagnoseForAmbiguity(CommonFixesArray commonFixes) const override;
20462044

20472045
static MustBeCopyable *create(ConstraintSystem &cs,
20482046
Type noncopyableTy,
20492047
ConstraintLocator *locator);
20502048

2051-
static bool classof(ConstraintFix *fix) {
2049+
static bool classof(ConstraintFix const* fix) {
20522050
return fix->getKind() == FixKind::MustBeCopyable;
20532051
}
20542052
};

lib/Sema/CSFix.cpp

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1343,6 +1343,25 @@ MustBeCopyable* MustBeCopyable::create(ConstraintSystem &cs,
13431343
return new (cs.getAllocator()) MustBeCopyable(cs, noncopyableTy, locator);
13441344
}
13451345

1346+
bool MustBeCopyable::diagnoseForAmbiguity(CommonFixesArray commonFixes) const {
1347+
// Only diagnose if all solutions agreed on the same errant non-copyable type.
1348+
Type firstNonCopyable;
1349+
for (const auto &solutionAndFix : commonFixes) {
1350+
const auto *solution = solutionAndFix.first;
1351+
const auto *fix = solutionAndFix.second->getAs<MustBeCopyable>();
1352+
1353+
auto otherNonCopyable = solution->simplifyType(fix->noncopyableTy);
1354+
if (!firstNonCopyable)
1355+
firstNonCopyable = otherNonCopyable;
1356+
1357+
if (firstNonCopyable->getCanonicalType() != otherNonCopyable->getCanonicalType()) {
1358+
return false; // fixes differed, so decline to emit a tailored diagnostic.
1359+
}
1360+
}
1361+
1362+
return diagnose(*commonFixes.front().first);
1363+
}
1364+
13461365
bool CollectionElementContextualMismatch::diagnose(const Solution &solution,
13471366
bool asNote) const {
13481367
CollectionElementContextualFailure failure(

0 commit comments

Comments
 (0)