Skip to content

[CSSimplify] Detect and diagnose generic argument mismatches individually #64036

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions include/swift/Sema/CSFix.h
Original file line number Diff line number Diff line change
Expand Up @@ -1100,6 +1100,10 @@ class GenericArgumentsMismatch final
return {getTrailingObjects<unsigned>(), NumMismatches};
}

bool coalesceAndDiagnose(const Solution &solution,
ArrayRef<ConstraintFix *> secondaryFixes,
bool asNote = false) const override;

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

static GenericArgumentsMismatch *create(ConstraintSystem &cs, Type actual,
Expand All @@ -1112,6 +1116,9 @@ class GenericArgumentsMismatch final
}

private:
bool diagnose(const Solution &solution, ArrayRef<unsigned> mismatches,
bool asNote = false) const;

MutableArrayRef<unsigned> getMismatchesBuf() {
return {getTrailingObjects<unsigned>(), NumMismatches};
}
Expand Down
14 changes: 14 additions & 0 deletions include/swift/Sema/ConstraintLocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -643,6 +643,20 @@ class LocatorPathElt::TupleType : public StoredPointerElement<TypeBase> {
}
};

class LocatorPathElt::GenericType : public StoredPointerElement<TypeBase> {
public:
GenericType(Type type)
: StoredPointerElement(PathElementKind::GenericType, type.getPointer()) {
assert(type->getDesugaredType()->is<BoundGenericType>());
}

Type getType() const { return getStoredPointer(); }

static bool classof(const LocatorPathElt *elt) {
return elt->getKind() == PathElementKind::GenericType;
}
};

/// Abstract superclass for any kind of tuple element.
class LocatorPathElt::AnyTupleElement : public StoredIntegerElement<1> {
protected:
Expand Down
4 changes: 4 additions & 0 deletions include/swift/Sema/ConstraintLocatorPathElts.def
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,10 @@ CUSTOM_LOCATOR_PATH_ELT(SynthesizedArgument)
/// path components.
CUSTOM_LOCATOR_PATH_ELT(TupleType)

/// A generic type, which provides context for subsequent generic
/// argument path components.
CUSTOM_LOCATOR_PATH_ELT(GenericType)

/// Tuple elements.
ABSTRACT_LOCATOR_PATH_ELT(AnyTupleElement)
/// A tuple element referenced by position.
Expand Down
5 changes: 5 additions & 0 deletions include/swift/Sema/ConstraintSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -4655,6 +4655,10 @@ class ConstraintSystem {
/// Indicates that we are attempting a possible type for
/// a type variable.
TMF_BindingTypeVariable = 0x04,

/// Indicates that the solver is matching one of the
/// generic argument pairs as part of matching two generic types.
TMF_MatchingGenericArguments = 0x08,
};

/// Options that govern how type matching should proceed.
Expand Down Expand Up @@ -5257,6 +5261,7 @@ class ConstraintSystem {
/// \return true if at least some of the failures has been repaired
/// successfully, which allows type matcher to continue.
bool repairFailures(Type lhs, Type rhs, ConstraintKind matchKind,
TypeMatchOptions flags,
SmallVectorImpl<RestrictionOrFix> &conversionsOrFixes,
ConstraintLocatorBuilder locator);

Expand Down
23 changes: 22 additions & 1 deletion lib/Sema/CSFix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -732,10 +732,31 @@ AllowFunctionTypeMismatch::create(ConstraintSystem &cs, Type lhs, Type rhs,
AllowFunctionTypeMismatch(cs, lhs, rhs, locator, index);
}

bool GenericArgumentsMismatch::coalesceAndDiagnose(
const Solution &solution, ArrayRef<ConstraintFix *> secondaryFixes,
bool asNote) const {
std::set<unsigned> scratch(getMismatches().begin(), getMismatches().end());

for (auto *fix : secondaryFixes) {
auto *genericArgsFix = fix->castTo<GenericArgumentsMismatch>();
for (auto mismatchIdx : genericArgsFix->getMismatches())
scratch.insert(mismatchIdx);
}

SmallVector<unsigned> mismatches(scratch.begin(), scratch.end());
return diagnose(solution, mismatches, asNote);
}

bool GenericArgumentsMismatch::diagnose(const Solution &solution,
bool asNote) const {
return diagnose(solution, getMismatches(), asNote);
}

bool GenericArgumentsMismatch::diagnose(const Solution &solution,
ArrayRef<unsigned> mismatches,
bool asNote) const {
GenericArgumentsMismatchFailure failure(solution, getFromType(), getToType(),
getMismatches(), getLocator());
mismatches, getLocator());
return failure.diagnose(asNote);
}

Expand Down
82 changes: 71 additions & 11 deletions lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3748,15 +3748,23 @@ ConstraintSystem::matchDeepEqualityTypes(Type type1, Type type2,
// Match up the generic arguments, exactly.

if (shouldAttemptFixes()) {
auto *baseLoc =
getConstraintLocator(locator, {LocatorPathElt::GenericType(bound1),
LocatorPathElt::GenericType(bound2)});

auto argMatchingFlags =
subflags | TMF_ApplyingFix | TMF_MatchingGenericArguments;

// Optionals have a lot of special diagnostics and only one
// generic argument so if we' re dealing with one, don't produce generic
// arguments mismatch fixes.
if (bound1->getDecl()->isOptionalDecl())
return matchDeepTypeArguments(*this, subflags, args1, args2, locator);
return matchDeepTypeArguments(*this, argMatchingFlags, args1, args2,
baseLoc);

SmallVector<unsigned, 4> mismatches;
auto result = matchDeepTypeArguments(
*this, subflags | TMF_ApplyingFix, args1, args2, locator,
*this, argMatchingFlags, args1, args2, baseLoc,
[&mismatches](unsigned position) { mismatches.push_back(position); });

if (mismatches.empty())
Expand Down Expand Up @@ -4843,7 +4851,7 @@ static bool repairOutOfOrderArgumentsInBinaryFunction(
/// \return true if at least some of the failures has been repaired
/// successfully, which allows type matcher to continue.
bool ConstraintSystem::repairFailures(
Type lhs, Type rhs, ConstraintKind matchKind,
Type lhs, Type rhs, ConstraintKind matchKind, TypeMatchOptions flags,
SmallVectorImpl<RestrictionOrFix> &conversionsOrFixes,
ConstraintLocatorBuilder locator) {
SmallVector<LocatorPathElt, 4> path;
Expand Down Expand Up @@ -5344,7 +5352,7 @@ bool ConstraintSystem::repairFailures(
// let's re-attempt to repair without l-value conversion in the
// locator to fix underlying type mismatch.
if (path.back().is<LocatorPathElt::FunctionResult>()) {
return repairFailures(lhs, rhs, matchKind, conversionsOrFixes,
return repairFailures(lhs, rhs, matchKind, flags, conversionsOrFixes,
getConstraintLocator(anchor, path));
}

Expand Down Expand Up @@ -6133,7 +6141,7 @@ bool ConstraintSystem::repairFailures(
if (!path.empty() && path.back().is<LocatorPathElt::PackType>())
path.pop_back();

return repairFailures(lhs, rhs, matchKind, conversionsOrFixes,
return repairFailures(lhs, rhs, matchKind, flags, conversionsOrFixes,
getConstraintLocator(anchor, path));
}

Expand Down Expand Up @@ -6369,19 +6377,70 @@ bool ConstraintSystem::repairFailures(
// failure e.g. `String bind T.Element`, so let's drop the generic argument
// path element and recurse in repairFailures to check and potentially
// record the requirement failure fix.
path.pop_back();
auto genericArgElt =
path.pop_back_val().castTo<LocatorPathElt::GenericArgument>();

// If we have something like ... -> type req # -> pack element #, we're
// solving a requirement of the form T : P where T is a type parameter pack
if (!path.empty() && path.back().is<LocatorPathElt::PackElement>())
path.pop_back();

if (!path.empty() && path.back().is<LocatorPathElt::AnyRequirement>()) {
return repairFailures(lhs, rhs, matchKind, conversionsOrFixes,
return repairFailures(lhs, rhs, matchKind, flags, conversionsOrFixes,
getConstraintLocator(anchor, path));
}

break;
// When the solver sets `TMF_MatchingGenericArguments` it means
// that it's matching generic argument pairs to identify any mismatches
// as part of larger matching of two generic types. Letting this
// fail results in a single fix that aggregates all mismatch locations.
//
// Types are not always resolved enough to enable that which means
// that the comparison should be delayed, which brings us here - a
// standalone constraint that represents such a match, in such cases
// we create a fix per mismatch location and coalesce them during
// diagnostics.
if (flags.contains(TMF_MatchingGenericArguments))
break;

Type fromType;
Type toType;

if (path.size() >= 2) {
if (path[path.size() - 2].is<LocatorPathElt::GenericType>()) {
fromType = path[path.size() - 2]
.castTo<LocatorPathElt::GenericType>()
.getType();
}

if (path[path.size() - 1].is<LocatorPathElt::GenericType>()) {
toType = path[path.size() - 1]
.castTo<LocatorPathElt::GenericType>()
.getType();
}
}

if (!fromType || !toType)
break;

// Drop both `GenericType` elements.
path.pop_back();
path.pop_back();

ConstraintFix *fix = nullptr;
if (!path.empty() && path.back().is<LocatorPathElt::AnyRequirement>()) {
fix = fixRequirementFailure(*this, fromType, toType, anchor, path);
} else {
fix = GenericArgumentsMismatch::create(
*this, fromType, toType, {genericArgElt.getIndex()},
getConstraintLocator(anchor, path));
}

if (!fix)
break;

conversionsOrFixes.push_back(fix);
return true;
}

case ConstraintLocator::ResultBuilderBodyResult: {
Expand Down Expand Up @@ -7479,7 +7538,8 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
// Attempt fixes iff it's allowed, both types are concrete and
// we are not in the middle of attempting one already.
if (shouldAttemptFixes() && !flags.contains(TMF_ApplyingFix)) {
if (repairFailures(type1, type2, kind, conversionsOrFixes, locator)) {
if (repairFailures(type1, type2, kind, flags, conversionsOrFixes,
locator)) {
if (conversionsOrFixes.empty())
return getTypeMatchSuccess();
}
Expand Down Expand Up @@ -14022,7 +14082,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
case FixKind::RenameConflictingPatternVariables:
case FixKind::MustBeCopyable:
case FixKind::MacroMissingPound:
case FixKind::AllowGlobalActorMismatch: {
case FixKind::AllowGlobalActorMismatch:
case FixKind::GenericArgumentsMismatch: {
return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
}
case FixKind::IgnoreInvalidASTNode: {
Expand Down Expand Up @@ -14242,7 +14303,6 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
case FixKind::TreatKeyPathSubscriptIndexAsHashable:
case FixKind::AllowInvalidRefInKeyPath:
case FixKind::DefaultGenericArgument:
case FixKind::GenericArgumentsMismatch:
case FixKind::AllowMutatingMemberOnRValueBase:
case FixKind::AllowTupleSplatForSingleParameter:
case FixKind::AllowNonClassTypeToConvertToAnyObject:
Expand Down
7 changes: 7 additions & 0 deletions lib/Sema/ConstraintLocator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ unsigned LocatorPathElt::getNewSummaryFlags() const {
case ConstraintLocator::GenericParameter:
case ConstraintLocator::GenericArgument:
case ConstraintLocator::TupleType:
case ConstraintLocator::GenericType:
case ConstraintLocator::NamedTupleElement:
case ConstraintLocator::TupleElement:
case ConstraintLocator::ProtocolRequirement:
Expand Down Expand Up @@ -242,6 +243,12 @@ void LocatorPathElt::dump(raw_ostream &out) const {
break;
}

case ConstraintLocator::GenericType: {
auto genericTyElt = elt.castTo<LocatorPathElt::GenericType>();
out << "generic type '" << genericTyElt.getType()->getString(PO) << "'";
break;
}

case ConstraintLocator::NamedTupleElement: {
auto tupleElt = elt.castTo<LocatorPathElt::NamedTupleElement>();
out << "named tuple element #" << llvm::utostr(tupleElt.getIndex());
Expand Down
1 change: 1 addition & 0 deletions lib/Sema/ConstraintSystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5372,6 +5372,7 @@ void constraints::simplifyLocator(ASTNode &anchor,
continue;

case ConstraintLocator::TupleType:
case ConstraintLocator::GenericType:
path = path.slice(1);
continue;

Expand Down
24 changes: 24 additions & 0 deletions test/Constraints/generics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1031,3 +1031,27 @@ func test_requirement_failures_in_ambiguous_context() {

f3(A()) // expected-error {{no exact matches in call to local function 'f3'}}
}

// rdar://106054263 - failed to produce a diagnostic upon generic argument mismatch
func test_mismatches_with_dependent_member_generic_arguments() {
struct Binding<T, U> {}
// expected-note@-1 {{arguments to generic parameter 'T' ('Double?' and 'Data.SomeAssociated') are expected to be equal}}
// expected-note@-2 {{arguments to generic parameter 'U' ('Int' and 'Data.SomeAssociated') are expected to be equal}}

struct Data : SomeProtocol {
typealias SomeAssociated = String
}

func test1<T: SomeProtocol>(_: Binding<T.SomeAssociated, T.SomeAssociated>, _: T) where T.SomeAssociated == String {
}

func test2<T: SomeProtocol>(_: Optional<T.SomeAssociated>, _: T) where T.SomeAssociated == String {
}

test1(Binding<Double?, Int>(), Data())
// expected-error@-1 {{cannot convert value of type 'Binding<Double?, Int>' to expected argument type 'Binding<Data.SomeAssociated, Data.SomeAssociated>'}}

test2(Optional<Int>(nil), Data())
// expected-error@-1 {{cannot convert value of type 'Optional<Int>' to expected argument type 'Optional<Data.SomeAssociated>'}}
// expected-note@-2 {{arguments to generic parameter 'Wrapped' ('Int' and 'Data.SomeAssociated') are expected to be equal}}
}
16 changes: 8 additions & 8 deletions test/Generics/issue-55666.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,19 @@ struct W<T> {}

struct S<C1: Collection> {
init(){}
// expected-note@+2 {{where 'C1.Element' = 'String', 'W<C2.Element>' = 'Int'}}
// expected-note@+1 {{where 'C1.Element' = 'C1', 'W<C2.Element>' = 'C2.Element'}}
// expected-note@+2 {{where 'C1.Element' = 'W<C1>', 'main.W<C2.Element>' = 'main.W<C2.Element>'}}
// expected-note@+1 {{where 'C1.Element' = 'W<String>', 'W<C2.Element>' = 'W<Array<Int>.Element>'}}
init<C2>(_ c2: W<C2>) where C2: Collection, C1.Element == W<C2.Element> {}
// expected-note@+1 {{where 'C1.Element' = 'String', 'W<C2.Element>' = 'Int'}}
// expected-note@+1 {{where 'C1.Element' = 'W<String>', 'W<C2.Element>' = 'W<Array<Int>.Element>'}}
static func f<C2>(_ c2: W<C2>) where C2: Collection, C1.Element == W<C2.Element> {}
// expected-note@+1 {{where 'C1.Element' = 'String', 'W<C2.Element>' = 'Int'}}
// expected-note@+1 {{where 'C1.Element' = 'W<String>', 'W<C2.Element>' = 'W<Array<Int>.Element>'}}
func instancef<C2>(_ c2: W<C2>) where C2: Collection, C1.Element == W<C2.Element> {}
}
let _ = S<[W<String>]>(W<[Int]>()) // expected-error{{initializer 'init(_:)' requires the types 'String' and 'Int' be equivalent}}
let _ = S<[W<String>]>.f(W<[Int]>()) // expected-error{{static method 'f' requires the types 'String' and 'Int' be equivalent}}
let _ = S<[W<String>]>().instancef(W<[Int]>()) // expected-error{{instance method 'instancef' requires the types 'String' and 'Int' be equivalent}}
let _ = S<[W<String>]>(W<[Int]>()) // expected-error{{initializer 'init(_:)' requires the types 'W<String>' and 'W<Array<Int>.Element>' be equivalent}}
let _ = S<[W<String>]>.f(W<[Int]>()) // expected-error{{static method 'f' requires the types 'W<String>' and 'W<Array<Int>.Element>' be equivalent}}
let _ = S<[W<String>]>().instancef(W<[Int]>()) // expected-error{{instance method 'instancef' requires the types 'W<String>' and 'W<Array<Int>.Element>' be equivalent}}

// Archetypes requirement failure
func genericFunc<C1: Collection, C2: Collection>(_ c2: W<C2>, c1: C1.Type) where C1.Element == W<C2.Element> {
let _ = S<[W<C1>]>(W<C2>()) // expected-error{{initializer 'init(_:)' requires the types 'C1' and 'C2.Element' be equivalent}}
let _ = S<[W<C1>]>(W<C2>()) // expected-error{{initializer 'init(_:)' requires the types 'W<C1>' and 'W<C2.Element>' be equivalent}}
}