Skip to content
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

[Sema][QoI] Call out missing conformances in protocol witness candidates. #19920

Merged
merged 7 commits into from
Oct 22, 2018
6 changes: 4 additions & 2 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -1782,8 +1782,8 @@ NOTE(bad_associated_type_deduction,none,
"unable to infer associated type %0 for protocol %1",
(DeclName, DeclName))
NOTE(associated_type_deduction_witness_failed,none,
"inferred type %1 (by matching requirement %0) is invalid: "
"does not %select{inherit from|conform to}3 %2",
"candidate would match and infer %0=%1 if %1 "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spaces around = might be more readable

"%select{inherited from|conformed to}3 %2",
(DeclName, Type, Type, bool))
NOTE(ambiguous_associated_type_deduction,none,
"ambiguous inference of associated type %0: %1 vs. %2",
Expand All @@ -1805,6 +1805,8 @@ NOTE(protocol_witness_kind_conflict,none,
"a subscript}0", (RequirementKind))
NOTE(protocol_witness_type_conflict,none,
"candidate has non-matching type %0%1", (Type, StringRef))
NOTE(protocol_witness_missing_conformance,none,
"candidate would match if %0 conformed to %1", (Type, Type))

NOTE(protocol_witness_optionality_conflict,none,
"candidate %select{type has|result type has|parameter type has|"
Expand Down
4 changes: 4 additions & 0 deletions lib/Sema/CSFix.h
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,10 @@ class MissingConformance final : public ConstraintFix {
static MissingConformance *create(ConstraintSystem &cs, Type type,
ProtocolDecl *protocol,
ConstraintLocator *locator);

Type getNonConformingType() { return NonConformingType; }

ProtocolDecl *getProtocol() { return Protocol; }
};

/// Skip same-type generic requirement constraint,
Expand Down
13 changes: 10 additions & 3 deletions lib/Sema/CSSolver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -524,10 +524,17 @@ ConstraintSystem::SolverScope::~SolverScope() {
/// \returns a solution if a single unambiguous one could be found, or None if
/// ambiguous or unsolvable.
Optional<Solution>
ConstraintSystem::solveSingle(FreeTypeVariableBinding allowFreeTypeVariables) {
ConstraintSystem::solveSingle(FreeTypeVariableBinding allowFreeTypeVariables,
bool allowFixes) {

SolverState state(nullptr, *this, allowFreeTypeVariables);
state.recordFixes = allowFixes;

SmallVector<Solution, 4> solutions;
if (solve(nullptr, solutions, allowFreeTypeVariables) ||
solutions.size() != 1)
solve(solutions);
filterSolutions(solutions, state.ExprWeights);

if (solutions.size() != 1)
return Optional<Solution>();

return std::move(solutions[0]);
Expand Down
5 changes: 4 additions & 1 deletion lib/Sema/ConstraintSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -3054,10 +3054,13 @@ class ConstraintSystem {
/// \param allowFreeTypeVariables How to bind free type variables in
/// the solution.
///
/// \param allowFixes Whether to allow fixes in the solution.
///
/// \returns a solution if a single unambiguous one could be found, or None if
/// ambiguous or unsolvable.
Optional<Solution> solveSingle(FreeTypeVariableBinding allowFreeTypeVariables
= FreeTypeVariableBinding::Disallow);
= FreeTypeVariableBinding::Disallow,
bool allowFixes = false);

private:
/// \brief Solve the system of constraints.
Expand Down
38 changes: 35 additions & 3 deletions lib/Sema/TypeCheckProtocol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,7 @@ swift::matchWitness(TypeChecker &tc,
auto setup = [&]() -> std::tuple<Optional<RequirementMatch>, Type, Type> {
// Construct a constraint system to use to solve the equality between
// the required type and the witness type.
cs.emplace(tc, dc, ConstraintSystemOptions());
cs.emplace(tc, dc, ConstraintSystemFlags::AllowFixes);

auto reqGenericEnv = reqEnvironment.getSyntheticEnvironment();
auto reqSubMap = reqEnvironment.getRequirementToSyntheticMap();
Expand Down Expand Up @@ -840,8 +840,35 @@ swift::matchWitness(TypeChecker &tc,
// Try to solve the system disallowing free type variables, because
// that would resolve in incorrect substitution matching when witness
// type has free type variables present as well.
auto solution = cs->solveSingle(FreeTypeVariableBinding::Disallow);
if (!solution)
auto solution = cs->solveSingle(FreeTypeVariableBinding::Disallow,
/* allowFixes */ true);

// If the types would match but for some other missing conformance, find and
// call that out.
if (solution && conformance && solution->Fixes.size() == 1 &&
solution->Fixes.front()->getKind() == FixKind::AddConformance) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is more than one fix, perhaps we should diagnose the first fix? Or walk through the fixes and diagnose the first one we recognize? It won't describe the whole problem, but it's probably a better clue than falling back to "type mismatch."

auto fix = (MissingConformance *)solution->Fixes.front();
auto type = fix->getNonConformingType();
auto protocolType = fix->getProtocol()->getDeclaredType();
if (auto memberTy = type->getAs<DependentMemberType>())
return RequirementMatch(witness, MatchKind::MissingConformance, type,
protocolType);

type = type->mapTypeOutOfContext();
if (auto typeParamTy = type->getAs<GenericTypeParamType>())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if it’s a dependent member type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked for just above (that's one of the things I broke and needed the second commit for)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we generalize the DependentMemberType to hasTypeParameter()?

if (auto env = conformance->getGenericEnvironment())
if (auto assocType = env->mapTypeIntoContext(typeParamTy))
return RequirementMatch(witness, MatchKind::MissingConformance,
assocType, protocolType);
if (type->isEqual(conformance->getType())) {
if (auto bgt = type->getAs<BoundGenericType>())
type = bgt->getDecl()->getDeclaredType();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if it’s a non-generic type nested in a generic type? Is this restriction really necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, fix upcoming

return RequirementMatch(witness, MatchKind::MissingConformance, type,
protocolType);
}
}

if (!solution || solution->Fixes.size())
return RequirementMatch(witness, MatchKind::TypeConflict,
witnessType);

Expand Down Expand Up @@ -2024,6 +2051,11 @@ diagnoseMatch(ModuleDecl *module, NormalProtocolConformance *conformance,
break;
}

case MatchKind::MissingConformance:
diags.diagnose(match.Witness, diag::protocol_witness_missing_conformance,
match.WitnessType, match.SecondType);
break;

case MatchKind::ThrowsConflict:
diags.diagnose(match.Witness, diag::protocol_witness_throws_conflict);
break;
Expand Down
26 changes: 26 additions & 0 deletions lib/Sema/TypeCheckProtocol.h
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@ enum class MatchKind : uint8_t {
/// \brief The types conflict.
TypeConflict,

/// \brief The witness would match with an additional conformance.
MissingConformance,

/// The witness throws, but the requirement does not.
ThrowsConflict,

Expand Down Expand Up @@ -352,6 +355,19 @@ struct RequirementMatch {
"Should (or should not) have witness type");
}

RequirementMatch(ValueDecl *witness, MatchKind kind,
Type witnessType, Type secondType,
Optional<RequirementEnvironment> env = None,
ArrayRef<OptionalAdjustment> optionalAdjustments = {})
: Witness(witness), Kind(kind), WitnessType(witnessType),
SecondType(secondType), ReqEnv(std::move(env)),
OptionalAdjustments(optionalAdjustments.begin(),
optionalAdjustments.end())
{
assert(hasWitnessType() && hasSecondType() &&
"Should have witness type and second type");
}

/// \brief The witness that matches the (implied) requirement.
ValueDecl *Witness;

Expand All @@ -361,6 +377,9 @@ struct RequirementMatch {
/// \brief The type of the witness when it is referenced.
Type WitnessType;

/// \brief Explanatory second type.
Type SecondType;

/// \brief The requirement environment to use for the witness thunk.
Optional<RequirementEnvironment> ReqEnv;

Expand All @@ -382,6 +401,7 @@ struct RequirementMatch {
case MatchKind::WitnessInvalid:
case MatchKind::KindConflict:
case MatchKind::TypeConflict:
case MatchKind::MissingConformance:
case MatchKind::StaticNonStaticConflict:
case MatchKind::SettableConflict:
case MatchKind::PrefixNonPrefixConflict:
Expand All @@ -404,6 +424,7 @@ struct RequirementMatch {
case MatchKind::ExactMatch:
case MatchKind::RenamedMatch:
case MatchKind::TypeConflict:
case MatchKind::MissingConformance:
case MatchKind::OptionalityConflict:
return true;

Expand All @@ -425,6 +446,11 @@ struct RequirementMatch {
llvm_unreachable("Unhandled MatchKind in switch.");
}

/// \brief Determine whether this requirement match has a second type.
bool hasSecondType() {
return Kind == MatchKind::MissingConformance;
}

swift::Witness getWitness(ASTContext &ctx) const;
};

Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/TypeCheckProtocolInference.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1749,7 +1749,7 @@ bool AssociatedTypeInference::diagnoseNoSolutions(

diags.diagnose(failed.Witness,
diag::associated_type_deduction_witness_failed,
failed.Requirement->getFullName(),
assocType->getName(),
failed.TypeWitness,
failed.Result.getRequirement(),
failed.Result.isConformanceRequirement());
Expand Down
2 changes: 1 addition & 1 deletion test/Generics/associated_types_inherit.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ struct X1 : P {
}

struct X2 : P { // expected-error{{type 'X2' does not conform to protocol 'P'}}
func getAssoc() -> E { return E() } // expected-note{{inferred type 'E' (by matching requirement 'getAssoc()') is invalid: does not inherit from 'C'}}
func getAssoc() -> E { return E() } // expected-note{{candidate would match and infer 'Assoc'='E' if 'E' inherited from 'C'}}
}

func testP<T:P>(_ t: T) {
Expand Down
5 changes: 2 additions & 3 deletions test/decl/ext/protocol_as_witness.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ protocol P1 {
protocol Q1 {}

extension P1 where Self : Q1 {
// FIXME: Poor QoI
func f() {} // expected-note{{candidate has non-matching type '<Self> () -> ()'}}
func f() {} // expected-note{{candidate would match if 'X1' conformed to 'Q1'}}
}

struct X1 : P1 {} // expected-error{{type 'X1' does not conform to protocol 'P1'}}
Expand All @@ -33,7 +32,7 @@ protocol P3 {
}

extension P3 where Self : Equatable {
func f() {} // expected-note{{candidate has non-matching type '<Self> () -> ()'}}
func f() {} // expected-note{{candidate would match if 'X3' conformed to 'Equatable'}}
}

struct X3 : P3 {} // expected-error{{type 'X3' does not conform to protocol 'P3'}}
12 changes: 6 additions & 6 deletions test/decl/protocol/req/associated_type_inference.swift
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ struct X0f : P0 { // okay: Assoc1 = Int because Float doesn't conform to PSimple
}

struct X0g : P0 { // expected-error{{type 'X0g' does not conform to protocol 'P0'}}
func f0(_: Float) { } // expected-note{{inferred type 'Float' (by matching requirement 'f0') is invalid: does not conform to 'PSimple'}}
func g0(_: Float) { } // expected-note{{inferred type 'Float' (by matching requirement 'g0') is invalid: does not conform to 'PSimple'}}
func f0(_: Float) { } // expected-note{{candidate would match and infer 'Assoc1'='Float' if 'Float' conformed to 'PSimple'}}
func g0(_: Float) { } // expected-note{{candidate would match and infer 'Assoc1'='Float' if 'Float' conformed to 'PSimple'}}
}

struct X0h<T : PSimple> : P0 {
Expand Down Expand Up @@ -93,8 +93,8 @@ protocol P2 {
}

extension P2 where Self.P2Assoc : PSimple {
func f0(_ x: P2Assoc) { } // expected-note{{inferred type 'Float' (by matching requirement 'f0') is invalid: does not conform to 'PSimple'}}
func g0(_ x: P2Assoc) { } // expected-note{{inferred type 'Float' (by matching requirement 'g0') is invalid: does not conform to 'PSimple'}}
func f0(_ x: P2Assoc) { } // expected-note{{candidate would match and infer 'Assoc1'='Float' if 'Float' conformed to 'PSimple'}}
func g0(_ x: P2Assoc) { } // expected-note{{candidate would match and infer 'Assoc1'='Float' if 'Float' conformed to 'PSimple'}}
}

struct X0k : P0, P2 {
Expand Down Expand Up @@ -123,7 +123,7 @@ struct XProp0a : PropertyP0 { // okay PropType = Int
}

struct XProp0b : PropertyP0 { // expected-error{{type 'XProp0b' does not conform to protocol 'PropertyP0'}}
var property: Float // expected-note{{inferred type 'Float' (by matching requirement 'property') is invalid: does not conform to 'PSimple'}}
var property: Float // expected-note{{candidate would match and infer 'Prop'='Float' if 'Float' conformed to 'PSimple'}}
}

// Inference from subscripts
Expand All @@ -144,7 +144,7 @@ struct XSubP0a : SubscriptP0 {

struct XSubP0b : SubscriptP0 {
// expected-error@-1{{type 'XSubP0b' does not conform to protocol 'SubscriptP0'}}
subscript (i: Int) -> Float { get { return Float(i) } } // expected-note{{inferred type 'Float' (by matching requirement 'subscript(_:)') is invalid: does not conform to 'PSimple'}}
subscript (i: Int) -> Float { get { return Float(i) } } // expected-note{{candidate would match and infer 'Element'='Float' if 'Float' conformed to 'PSimple'}}
}

struct XSubP0c : SubscriptP0 {
Expand Down
2 changes: 1 addition & 1 deletion test/decl/protocol/req/func.swift
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ protocol P12 {
struct XIndexType : P11 { }

struct X12 : P12 { // expected-error{{type 'X12' does not conform to protocol 'P12'}}
func getIndex() -> XIndexType { return XIndexType() } // expected-note{{inferred type 'XIndexType' (by matching requirement 'getIndex()') is invalid: does not conform to 'P1'}}
func getIndex() -> XIndexType { return XIndexType() } // expected-note{{candidate would match and infer 'Index'='XIndexType' if 'XIndexType' conformed to 'P1'}}
}

func ==(x: X12.Index, y: X12.Index) -> Bool { return true }
Expand Down
57 changes: 57 additions & 0 deletions test/decl/protocol/req/missing_conformance.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// RUN: %target-typecheck-verify-swift

// Test candidates for witnesses that are missing conformances
// in various ways.

protocol LikeSetAlgebra {
func onion(_ other: Self) -> Self // expected-note {{protocol requires function 'onion' with type '(X) -> X'; do you want to add a stub?}}
func indifference(_ other: Self) -> Self // expected-note {{protocol requires function 'indifference' with type '(X) -> X'; do you want to add a stub?}}

}
protocol LikeOptionSet : LikeSetAlgebra, RawRepresentable {}
extension LikeOptionSet where RawValue : FixedWidthInteger {
func onion(_ other: Self) -> Self { return self } // expected-note {{candidate would match if 'X.RawValue' conformed to 'FixedWidthInteger'}}
func indifference(_ other: Self) -> Self { return self } // expected-note {{candidate would match if 'X.RawValue' conformed to 'FixedWidthInteger'}}
}

struct X : LikeOptionSet {}
// expected-error@-1 {{type 'X' does not conform to protocol 'LikeSetAlgebra'}}
// expected-error@-2 {{type 'X' does not conform to protocol 'RawRepresentable'}}

protocol IterProtocol {}
protocol LikeSequence {
associatedtype Iter : IterProtocol // expected-note {{unable to infer associated type 'Iter' for protocol 'LikeSequence'}}
func makeIter() -> Iter
}
extension LikeSequence where Self == Self.Iter {
func makeIter() -> Self { return self } // expected-note {{candidate would match and infer 'Iter'='Y' if 'Y' conformed to 'IterProtocol'}}
}

struct Y : LikeSequence {} // expected-error {{type 'Y' does not conform to protocol 'LikeSequence'}}

protocol P1 {
associatedtype Result
func get() -> Result // expected-note {{protocol requires function 'get()' with type '() -> Result'; do you want to add a stub?}}
func got() // expected-note {{protocol requires function 'got()' with type '() -> ()'; do you want to add a stub?}}
}
protocol P2 {
static var singularThing: Self { get }
}
extension P1 where Result : P2 {
func get() -> Result { return Result.singularThing } // expected-note {{candidate would match if 'Result' conformed to 'P2'}}
}
protocol P3 {}
extension P1 where Self : P3 {
func got() {} // expected-note {{candidate would match if 'Z' conformed to 'P3'}}
}

struct Z<T1, T2, T3, Result, T4> : P1 {} // expected-error {{type 'Z<T1, T2, T3, Result, T4>' does not conform to protocol 'P1'}}

protocol P4 {
func this() // expected-note {{protocol requires function 'this()' with type '() -> ()'; do you want to add a stub?}}
}
protocol P5 {}
extension P4 where Self : P5 {
func this() {} // expected-note {{candidate would match if 'W' conformed to 'P5'}}
}
struct W : P4 {} // expected-error {{type 'W' does not conform to protocol 'P4'}}
2 changes: 1 addition & 1 deletion test/stmt/foreach.swift
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ func bad_containers_2(bc: BadContainer2) {
}

struct BadContainer3 : Sequence { // expected-error{{type 'BadContainer3' does not conform to protocol 'Sequence'}}
func makeIterator() { } // expected-note{{inferred type '()' (by matching requirement 'makeIterator()') is invalid: does not conform to 'IteratorProtocol'}}
func makeIterator() { } // expected-note{{candidate would match and infer 'Iterator'='()' if '()' conformed to 'IteratorProtocol'}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, but () is not a nominal type, so it would never conform. Can you instead just say the return type is wrong without offering a suggestion?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, more specific can't-conform-to-non-nominal and can't-inherit-from-non-class coming up.

}

func bad_containers_3(bc: BadContainer3) {
Expand Down