Skip to content

[Diagnostics] Correctly identify location of requirement failure #26677

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
Aug 17, 2019
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
32 changes: 30 additions & 2 deletions lib/Sema/CSDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -373,8 +373,36 @@ ValueDecl *RequirementFailure::getDeclRef() const {
if (isFromContextualType())
return getAffectedDeclFromType(cs.getContextualType());

if (auto overload = getChoiceFor(getRawAnchor()))
return overload->choice.getDecl();
if (auto overload = getChoiceFor(getRawAnchor())) {
// If there is a declaration associated with this
// failure e.g. an overload choice of the call
// expression, let's see whether failure is
// associated with it directly or rather with
// one of its parents.
if (auto *decl = overload->choice.getDeclOrNull()) {
auto *DC = decl->getDeclContext();

do {
if (auto *parent = DC->getAsDecl()) {
if (auto *GC = parent->getAsGenericContext()) {
if (GC->getGenericSignature() != Signature)
continue;

// If this is a signature if an extension
// then it means that code has referenced
// something incorrectly and diagnostic
// should point to the referenced declaration.
if (isa<ExtensionDecl>(parent))
break;

return cast<ValueDecl>(parent);
}
}
} while ((DC = DC->getParent()));

return decl;
}
}

return getAffectedDeclFromType(getOwnerType());
}
Expand Down
21 changes: 1 addition & 20 deletions lib/Sema/CSDiagnostics.h
Original file line number Diff line number Diff line change
Expand Up @@ -304,31 +304,12 @@ class RequirementFailure : public FailureDiagnostic {
if (isConditional())
return true;

auto *anchor = getAnchor();
// In the situations like this:
//
// ```swift
// enum E<T: P> { case foo(T) }
// let _: E = .foo(...)
// ```
//
// `E` is going to be opened twice. First, when
// it's used as a contextual type, and when `E.foo`
// is found and its function type is opened.
// We still want to record both fixes but should
// avoid diagnosing the same problem multiple times.
if (isa<UnresolvedMemberExpr>(anchor)) {
auto path = getLocator()->getPath();
if (path.front().getKind() != ConstraintLocator::UnresolvedMember)
return false;
}

// For static/initializer calls there is going to be
// a separate fix, attached to the argument, which is
// much easier to diagnose.
// For operator calls we can't currently produce a good
// diagnostic, so instead let's refer to expression diagnostics.
return !(Apply && (isOperator(Apply) || isa<TypeExpr>(anchor)));
return !(Apply && isOperator(Apply));
}

static bool isOperator(const ApplyExpr *apply) {
Expand Down
23 changes: 19 additions & 4 deletions lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2484,8 +2484,14 @@ bool ConstraintSystem::repairFailures(
if (lhs->hasDependentMember() || rhs->hasDependentMember())
break;

if (auto *fix = fixRequirementFailure(*this, lhs, rhs, anchor, path))
auto reqKind = static_cast<RequirementKind>(elt.getValue2());
if (hasFixedRequirement(lhs, reqKind, rhs))
return true;

if (auto *fix = fixRequirementFailure(*this, lhs, rhs, anchor, path)) {
recordFixedRequirement(lhs, reqKind, rhs);
conversionsOrFixes.push_back(fix);
}
break;
}

Expand Down Expand Up @@ -3756,6 +3762,11 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyConformsToConstraint(
if (type->isVoid() || type->isUninhabited())
return SolutionKind::Error;

auto protocolTy = protocol->getDeclaredType();
// If this conformance has been fixed already, let's just consider this done.
if (hasFixedRequirement(type, RequirementKind::Conformance, protocolTy))
return SolutionKind::Solved;

// If this is a generic requirement let's try to record that
// conformance is missing and consider this a success, which
// makes it much easier to diagnose problems like that.
Expand All @@ -3768,10 +3779,14 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyConformsToConstraint(

if (path.back().isTypeParameterRequirement() ||
path.back().isConditionalRequirement()) {
if (auto *fix = fixRequirementFailure(
*this, type, protocol->getDeclaredType(), anchor, path)) {
if (!recordFix(fix))
if (auto *fix =
fixRequirementFailure(*this, type, protocolTy, anchor, path)) {
if (!recordFix(fix)) {
// Record this conformance requirement as "fixed".
recordFixedRequirement(type, RequirementKind::Conformance,
protocolTy);
return SolutionKind::Solved;
}
}
}

Expand Down
5 changes: 5 additions & 0 deletions lib/Sema/CSSolver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,7 @@ ConstraintSystem::SolverScope::SolverScope(ConstraintSystem &cs)
numSavedBindings = cs.solverState->savedBindings.size();
numConstraintRestrictions = cs.ConstraintRestrictions.size();
numFixes = cs.Fixes.size();
numFixedRequirements = cs.FixedRequirements.size();
numDisjunctionChoices = cs.DisjunctionChoices.size();
numOpenedTypes = cs.OpenedTypes.size();
numOpenedExistentialTypes = cs.OpenedExistentialTypes.size();
Expand Down Expand Up @@ -507,6 +508,10 @@ ConstraintSystem::SolverScope::~SolverScope() {
// Remove any opened types.
truncate(cs.OpenedTypes, numOpenedTypes);

// Remove any conformances solver had to fix along
// the current path.
truncate(cs.FixedRequirements, numFixedRequirements);

// Remove any opened existential types.
truncate(cs.OpenedExistentialTypes, numOpenedExistentialTypes);

Expand Down
20 changes: 20 additions & 0 deletions lib/Sema/ConstraintSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -1119,6 +1119,23 @@ class ConstraintSystem {
SmallVector<std::pair<ConstraintLocator *, ArrayRef<OpenedType>>, 4>
OpenedTypes;

/// The list of all generic requirements fixed along the current
/// solver path.
using FixedRequirement = std::tuple<TypeBase *, RequirementKind, TypeBase *>;
SmallVector<FixedRequirement, 4> FixedRequirements;

bool hasFixedRequirement(Type lhs, RequirementKind kind, Type rhs) {
auto reqInfo = std::make_tuple(lhs.getPointer(), kind, rhs.getPointer());
return llvm::any_of(
FixedRequirements,
[&reqInfo](const FixedRequirement &entry) { return entry == reqInfo; });
}

void recordFixedRequirement(Type lhs, RequirementKind kind, Type rhs) {
FixedRequirements.push_back(
std::make_tuple(lhs.getPointer(), kind, rhs.getPointer()));
}

/// A mapping from constraint locators to the opened existential archetype
/// used for the 'self' of an existential type.
SmallVector<std::pair<ConstraintLocator *, OpenedArchetypeType *>, 4>
Expand Down Expand Up @@ -1614,6 +1631,9 @@ class ConstraintSystem {
/// The length of \c Fixes.
unsigned numFixes;

/// The length of \c FixedRequirements.
unsigned numFixedRequirements;

/// The length of \c DisjunctionChoices.
unsigned numDisjunctionChoices;

Expand Down
35 changes: 26 additions & 9 deletions test/Constraints/generics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -335,14 +335,14 @@ func testFixIts() {
_ = AnyClassAndProtoBound() // expected-error {{generic parameter 'Foo' could not be inferred}} expected-note {{explicitly specify the generic arguments to fix this issue}} {{28-28=<<#Foo: SubProto & AnyObject#>>}}
_ = AnyClassAndProtoBound2() // expected-error {{generic parameter 'Foo' could not be inferred}} expected-note {{explicitly specify the generic arguments to fix this issue}} {{29-29=<<#Foo: SubProto & AnyObject#>>}}

_ = ClassAndProtoBound() // expected-error {{referencing initializer 'init()' on 'ClassAndProtoBound' requires that 'X' conform to 'SubProto'}}
_ = ClassAndProtoBound() // expected-error {{generic struct 'ClassAndProtoBound' requires that 'X' conform to 'SubProto'}}

_ = ClassAndProtosBound()
// expected-error@-1 {{referencing initializer 'init()' on 'ClassAndProtosBound' requires that 'X' conform to 'NSCopyish'}}
// expected-error@-2 {{referencing initializer 'init()' on 'ClassAndProtosBound' requires that 'X' conform to 'SubProto'}}
_ = ClassAndProtosBound()
// expected-error@-1 {{generic struct 'ClassAndProtosBound' requires that 'X' conform to 'SubProto'}}
// expected-error@-2 {{generic struct 'ClassAndProtosBound' requires that 'X' conform to 'NSCopyish'}}
_ = ClassAndProtosBound2()
// expected-error@-1 {{referencing initializer 'init()' on 'ClassAndProtosBound2' requires that 'X' conform to 'NSCopyish'}}
// expected-error@-2 {{referencing initializer 'init()' on 'ClassAndProtosBound2' requires that 'X' conform to 'SubProto'}}
// expected-error@-1 {{generic struct 'ClassAndProtosBound2' requires that 'X' conform to 'SubProto'}}
// expected-error@-2 {{generic struct 'ClassAndProtosBound2' requires that 'X' conform to 'NSCopyish'}}

_ = Pair()
// expected-error@-1 {{generic parameter 'T' could not be inferred}}
Expand Down Expand Up @@ -631,16 +631,16 @@ func rdar40537858() {
}

var arr: [S] = []
_ = List(arr, id: \.id) // expected-error {{referencing initializer 'init(_:id:)' on 'List' requires that 'S.Id' conform to 'Hashable'}}
_ = List(arr, id: \.id) // expected-error {{generic struct 'List' requires that 'S.Id' conform to 'Hashable'}}

enum E<T: P> { // expected-note 2 {{where 'T' = 'S'}}
case foo(T)
case bar([T])
}

var s = S(id: S.Id())
let _: E = .foo(s) // expected-error {{enum case 'foo' requires that 'S' conform to 'P'}}
let _: E = .bar([s]) // expected-error {{enum case 'bar' requires that 'S' conform to 'P'}}
let _: E = .foo(s) // expected-error {{generic enum 'E' requires that 'S' conform to 'P'}}
let _: E = .bar([s]) // expected-error {{generic enum 'E' requires that 'S' conform to 'P'}}
}

// https://bugs.swift.org/browse/SR-8934
Expand Down Expand Up @@ -798,3 +798,20 @@ struct R_51413254 {
self.str = try anyDict ==> Key("a") // Ok
}
}

func test_correct_identification_of_requirement_source() {
struct X<T: P> { // expected-note {{where 'T' = 'Int'}}
init<U: P>(_: T, _: U) {} // expected-note {{where 'U' = 'Int'}}
}

struct A : P {
static func foo(_ arg: A) -> A {
return A()
}
}

_ = X(17, A())
// expected-error@-1 {{generic struct 'X' requires that 'Int' conform to 'P'}}
_ = X(A(), 17)
// expected-error@-1 {{initializer 'init(_:_:)' requires that 'Int' conform to 'P'}}
}
5 changes: 2 additions & 3 deletions test/Constraints/rdar44569159.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,12 @@ struct S<V> where V: P { // expected-note {{where 'V' = 'Double'}}
}

struct A {
subscript<T>(_ v: S<T>) -> A { // expected-note {{where 'T' = 'Double'}}
subscript<T>(_ v: S<T>) -> A {
fatalError()
}
}

func foo(_ v: Double) {
_ = A()[S(value: v)]
// expected-error@-1 {{subscript 'subscript(_:)' requires that 'Double' conform to 'P'}}
// expected-error@-2 {{referencing initializer 'init(value:)' on 'S' requires that 'Double' conform to 'P'}}
// expected-error@-1 {{generic struct 'S' requires that 'Double' conform to 'P'}}
}
2 changes: 2 additions & 0 deletions test/Sema/diag_ambiguous_overloads.swift
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ class sr7440_Genre {
return sr7440_Genre.fetch(genreID: iTunesGenre.genreID, name: iTunesGenre.name)
// expected-error@-1 {{value of type 'sr7440_ITunesGenre' has no member 'genreID'; did you mean 'genre'?}}
// expected-error@-2 {{cannot convert return expression of type '()' to return type 'sr7440_Genre'}}
// expected-error@-3 {{protocol type 'Any' cannot conform to 'BinaryInteger' because only concrete types can conform to protocols}}
// TODO(diagnostics): Last diagnostic should not be recorded but to be able to correctly handle it we need a notion of a "hole" in constraint system.
}
}

Expand Down
2 changes: 1 addition & 1 deletion test/decl/nested/type_in_type.swift
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ func test() {
// expected-error@-1 {{generic parameter 'A' could not be inferred}}
_ = Claws.Fangs<NotADog>()
// expected-error@-1 {{generic parameter 'A' could not be inferred}}
// expected-error@-2 {{referencing initializer 'init()' on 'Claws.Fangs' requires that 'NotADog' conform to 'ExpressibleByDogLiteral'}}
// expected-error@-2 {{generic struct 'Fangs' requires that 'NotADog' conform to 'ExpressibleByDogLiteral'}}
// expected-note@-3 {{explicitly specify the generic arguments to fix this issue}} {{12-12=<<#A: ExpressibleByCatLiteral#>>}}
}

Expand Down
7 changes: 4 additions & 3 deletions test/expr/unary/keypath/salvage-with-other-type-errors.swift
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@
// to diagnose other errors in adjacent exprs.

struct P<T: K> { }
// expected-note@-1 {{arguments to generic parameter 'T' ('String' and '_') are expected to be equal}}

struct S {
init<B>(_ a: P<B>) { // expected-note {{where 'B' = 'String'}}
init<B>(_ a: P<B>) {
fatalError()
}
}
Expand All @@ -17,7 +18,7 @@ func + <Object>(lhs: KeyPath<A, Object>, rhs: String) -> P<Object> {
fatalError()
}

// expected-error@+1{{}}
// expected-error@+1{{type 'String' does not conform to protocol 'K'}}
func + (lhs: KeyPath<A, String>, rhs: String) -> P<String> {
fatalError()
}
Expand All @@ -27,7 +28,7 @@ struct A {
}

extension A: K {
static let j = S(\A.id + "id") // expected-error {{initializer 'init(_:)' requires that 'String' conform to 'K'}}
static let j = S(\A.id + "id") // expected-error {{cannot convert value of type 'P<String>' to expected argument type 'P<_>'}}
}

// SR-5034
Expand Down