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

gregomni · 2018-10-16T22:54:15Z

Continuing on to try to further improve inference QoI: Let CS::solveSingle() optionally allow fixes in the solution. Use this in protocol inference to look for fixes due to missing conformances and give better candidate failure notes. (Which were previously types-didn't-match notes that didn't really say anything about the actual problem.) Also changed the diagnosis text for the existing associated type inference failure due to missing requirement so that it is phrased similarly.

Thus, new candidate notes like:

a.swift:1:8: error: type 'X' does not conform to protocol 'SetAlgebra'
struct X : OptionSet {}
       ^
Swift.OptionSet:3:37: note: candidate would match if 'X.RawValue' conformed to 'FixedWidthInteger'
    @inlinable public mutating func formUnion(_ other: Self)

a.swift:2:8: error: type 'Y' does not conform to protocol 'Sequence'
struct Y : Sequence {}
       ^
Swift.Sequence:3:20: note: unable to infer associated type 'Iterator' for protocol 'Sequence'
    associatedtype Iterator : IteratorProtocol
                   ^
Swift.Sequence:2:40: note: candidate would match and infer 'Iterator'='Y' if 'Y' conformed to 'IteratorProtocol'
    @inlinable public __consuming func makeIterator() -> Self

…s in protocol inference to give better candidate failure notes.

gregomni · 2018-10-16T22:54:27Z

@swift-ci Please smoke test.

… thing to tell yourself.

gregomni · 2018-10-17T02:07:31Z

@swift-ci Please smoke test.

slavapestov

Cute.

slavapestov · 2018-10-17T02:10:58Z

include/swift/AST/DiagnosticsSema.def

@@ -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 "


Spaces around = might be more readable

slavapestov · 2018-10-17T02:12:53Z

lib/Sema/TypeCheckProtocol.cpp

+                                protocolType);
+
+      type = type->mapTypeOutOfContext();
+      if (auto typeParamTy = type->getAs<GenericTypeParamType>())


What if it’s a dependent member type?

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

Should we generalize the DependentMemberType to hasTypeParameter()?

slavapestov · 2018-10-17T02:13:17Z

lib/Sema/TypeCheckProtocol.cpp

+                                    assocType, protocolType);
+      if (type->isEqual(conformance->getType())) {
+        if (auto bgt = type->getAs<BoundGenericType>())
+          type = bgt->getDecl()->getDeclaredType();


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

Nope, fix upcoming

slavapestov · 2018-10-17T02:15:08Z

test/stmt/foreach.swift

@@ -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'}}


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?

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

slavapestov · 2018-10-17T02:16:10Z

For completeness you could do something similar for superclass constraints as well... but maybe those are rare enough that it doesn’t matter.

gregomni · 2018-10-17T14:38:58Z

@swift-ci Please smoke test.

…rements so that we check superclass and same type requirements in the same way.

gregomni · 2018-10-19T17:04:51Z

@slavapestov Added support for other types of requirements, so it gives similar messages for missing same-type and missing superclass. I was surprised that there was even an existing test that hit the new diagnosis for the same-type one - I thought that would be vanishingly rare.

DougGregor

These diagnostics are much-improved, thank you!

DougGregor · 2018-10-19T17:18:06Z

lib/Sema/TypeCheckProtocol.cpp

+                                protocolType);
+
+      type = type->mapTypeOutOfContext();
+      if (auto typeParamTy = type->getAs<GenericTypeParamType>())


Should we generalize the DependentMemberType to hasTypeParameter()?

DougGregor · 2018-10-19T17:19:12Z

lib/Sema/TypeCheckProtocol.cpp

+    // 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) {


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."

DougGregor · 2018-10-19T17:20:47Z

lib/Sema/TypeCheckProtocolInference.cpp

@@ -1747,9 +1747,26 @@ bool AssociatedTypeInference::diagnoseNoSolutions(
          if (failed.Result.isError())
            continue;

+          if (!failed.TypeWitness->is<NominalType>() &&


Please use getAnyNominal() to check whether we have a nominal type here; is<NominalType>() will only identify non-generic nominal types

... ugh, and I guess it's getAnyNominal() && !isExistentialType() because of protocols. The type system's modeling of protocols is not true to the language w.r.t. protocols.

Oops, missed the second part of this. Fix momentarily.

Wait, I'm confused. I think this is ok because the test is negative. An existential can't conform to a protocol and an existential is not a nominal. A protocol can conform, and a protocol is a nominal. Do I have part of this wrong?

DougGregor · 2018-10-19T17:22:36Z

lib/Sema/TypeCheckProtocolInference.cpp

+                           failed.Result.getRequirement());
+            continue;
+          }
+          if (!failed.TypeWitness->is<ClassType>() &&


Please use getClassOrBoundGenericClass() rather than is<ClassType>() here.

DougGregor · 2018-10-19T17:23:39Z

lib/Sema/TypeCheckProtocolInference.cpp

+            continue;
+          }
+          if (!failed.TypeWitness->is<ClassType>() &&
+              !failed.Result.isConformanceRequirement()) {


Hmm, could !failed.Result.isConformanceRequirement() be replaced by a positive check failed.Result.isSuperclassRequirement()? If we get a third failure kind that's "not a conformance requirement" we'd produce the wrong diagnostic here.

gregomni · 2018-10-19T18:25:58Z

@DougGregor Did those, thanks!

gregomni · 2018-10-19T18:26:06Z

@swift-ci Please smoke test.

…fix checking into a separate function and try it with multiple fixes, if they exist.

gregomni · 2018-10-20T16:44:45Z

Figured out my existential confusion, so fixed that, and implemented your suggestion of looking at all fixes, if there is more than one.

gregomni · 2018-10-20T16:44:52Z

@swift-ci Please smoke test.

Let CS::solveSingle() optionally allow fixes in the solution. Use thi…

f2a80c4

…s in protocol inference to give better candidate failure notes.

gregomni requested a review from DougGregor October 16, 2018 22:54

Of course "tiny little change, I don't have to retest" remains a dumb…

782f620

… thing to tell yourself.

slavapestov reviewed Oct 17, 2018

View reviewed changes

slavapestov self-assigned this Oct 17, 2018

Modified diagnostics a bit given feedback

002c04e

Extend candidate missing conformance checking to other types of requi…

939de4f

…rements so that we check superclass and same type requirements in the same way.

Missed removing obsolete code.

723d56c

DougGregor reviewed Oct 19, 2018

View reviewed changes

Review feedback

85ad4a1

Worked out my confusion with existentials, and pulled out constraint …

b91e455

…fix checking into a separate function and try it with multiple fixes, if they exist.

gregomni merged commit d20fdf5 into apple:master Oct 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gregomni commented Oct 16, 2018

gregomni commented Oct 16, 2018

gregomni commented Oct 17, 2018

slavapestov left a comment

slavapestov Oct 17, 2018

slavapestov Oct 17, 2018

gregomni Oct 17, 2018

DougGregor Oct 19, 2018

slavapestov Oct 17, 2018

gregomni Oct 17, 2018

slavapestov Oct 17, 2018

gregomni Oct 17, 2018

slavapestov commented Oct 17, 2018

gregomni commented Oct 17, 2018

gregomni commented Oct 19, 2018

DougGregor left a comment

DougGregor Oct 19, 2018

DougGregor Oct 19, 2018

DougGregor Oct 19, 2018

DougGregor Oct 19, 2018

gregomni Oct 19, 2018

gregomni Oct 19, 2018

DougGregor Oct 19, 2018

DougGregor Oct 19, 2018

gregomni commented Oct 19, 2018

gregomni commented Oct 19, 2018

gregomni commented Oct 20, 2018

gregomni commented Oct 20, 2018

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

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

Conversation

gregomni commented Oct 16, 2018

gregomni commented Oct 16, 2018

gregomni commented Oct 17, 2018

slavapestov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slavapestov commented Oct 17, 2018

gregomni commented Oct 17, 2018

gregomni commented Oct 19, 2018

DougGregor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregomni commented Oct 19, 2018

gregomni commented Oct 19, 2018

gregomni commented Oct 20, 2018

gregomni commented Oct 20, 2018