[ConstraintSystem] Diagnose missing conformance requirements via "fixes" #18484

xedin · 2018-08-03T05:01:03Z

If fixes are allowed let solver record missing protocol conformance
requirements and assume that conformsTo constraint is successfully
solved, this helps to diagnose such errors without involving
heavy-weight expression based diagnostics.

Resolves: rdar://problem/40537858

If fixes are allowed let solver record missing protocol conformance requirements and assume that `conformsTo` constraint is successfully solved, this helps to diagnose such errors without involving heavy-weight expression based diagnostics. Resolves: rdar://problem/40537858

Any solution which doesn't require source changes should be considered better than any other solution which does. Otherwise solutions with fixes are always considered better than solutions with unavailable overloads, which isn't correct especially considering that different types of errors are now diagnosed via forming solutions with fixes.

…pr is available While trying to diagnose missing conformances don't attempt to lookup parameter position if the parent apply expression is not available, which could happen when fixes are produced as part of the sub-expression diagnostics.

… tracking

…conformances

slavapestov

Most of the diagnostic changes look like clear improvements, nice work! I have a few small suggestions and some ideas for future work.

slavapestov · 2018-08-03T07:09:26Z

test/Constraints/generics.swift

+  }
+
+  var arr: [S] = []
+  _ = List(arr, id: \.id) // expected-error {{'List<[S], S.Id>' requires that 'S.Id' conform to 'Hashable'}}


Eventually, we should say something like

List<T, E> requires that 'E' conform to 'Hashable' (with 'E' = 'S.Id')

I can definitely add requirement information to the diagnostic message, since owner is known I can get generic signature and extract the original type from requirement.

Yeah. Including the name of the decl and it’s generic signature would make the diagnostic clearer, unless it becomes too long; but in that case we can split off some notes to spell out the substitutions, for example

slavapestov · 2018-08-03T07:10:02Z

test/Constraints/conditionally_defined_types.swift

@@ -75,18 +75,18 @@ let _ = Conforms<X>.Decl3.self
 let _ = Conforms<X>.Decl4<X>.self
 let _ = Conforms<X>.Decl5<X>.self

-let _ = Conforms<Y>.TypeAlias1.self // expected-error {{type 'Y' does not conform to protocol 'P'}}
-let _ = Conforms<Y>.TypeAlias2.self // expected-error {{type 'Y' does not conform to protocol 'P'}}
+let _ = Conforms<Y>.TypeAlias1.self // expected-error {{'Conforms<Y>.TypeAlias1.Type' (aka 'Y.Type') requires that 'Y' conform to 'P'}}


The '.Type' is just noise. It might be better to refer to the declaration here -- something like

typealias 'TypeAlias1' requires that 'Y' conform to 'P'

slavapestov · 2018-08-03T07:11:38Z

test/Constraints/diagnostics.swift


 func platypus<T>(a: [T]) {
-    _ = elephant(a) // expected-error {{generic parameter 'U' could not be inferred}}
+    _ = elephant(a) // expected-error {{'([T]) -> ()' requires that 'T' conform to 'Hashable'}}


Another case where we diagnose the type but not the decl -- shouldn't this say

function 'elephant' requires that 'T' conform to 'Hashable'

Yes, that definitely would be great! I left that for last, but I think it shouldn't be hard to do because anchor point to either DeclRefExpr or UnresolvedDot which should have a kind/name attach to it.

Just trying to get protocol compositions diagnose correctly first...

slavapestov · 2018-08-03T07:13:28Z

test/Constraints/generics.swift

@@ -188,7 +188,7 @@ func r22459135() {

 // <rdar://problem/19710848> QoI: Friendlier error message for "[] as Set"
 // <rdar://problem/22326930> QoI: "argument for generic parameter 'Element' could not be inferred" lacks context
-_ = [] as Set  // expected-error {{generic parameter 'Element' could not be inferred in cast to 'Set'}} expected-note {{explicitly specify the generic arguments to fix this issue}} {{14-14=<<#Element: Hashable#>>}}
+_ = [] as Set  // expected-error {{using 'Any' as a concrete type conforming to protocol 'Hashable' is not supported}}


This is more accurate now, awesome!

I know this is an existing diagnostic, but in a subsequent patch, do you mind changing it to not say "not supported". Instead, I would just say "protocol type 'Any' cannot conform to 'Hashable' because only concrete types can conform to protocols" or something like that.

Sure! There are currently 2 existing diagnostics - one for objc and one for swift types, should I modify both or just the one related to swift types?

I think the objc one already diagnoses the correct cause - static or initializer requirements. The swift one just says “not supported” which suggests it might be supported, but we don’t know if we’re going to implement self conforming existentials yet. Instead of promising a behavior, the diagnostic should explain the language rule - only concrete types can satisfy protocol requirements.

slavapestov · 2018-08-03T07:14:08Z

test/Generics/deduction.swift

@@ -214,7 +214,7 @@ func rangeOfIsBefore<R : IteratorProtocol>(_ range: R) where R.Element : IsBefor

 func callRangeOfIsBefore(_ ia: [Int], da: [Double]) {
  rangeOfIsBefore(ia.makeIterator())
-  rangeOfIsBefore(da.makeIterator()) // expected-error{{type 'Double' does not conform to protocol 'IsBefore'}}
+  rangeOfIsBefore(da.makeIterator()) // expected-error{{'(IndexingIterator<[Double]>) -> ()' requires that 'Double' conform to 'IsBefore'}}


Another case where the decl and not the type would be most useful

slavapestov · 2018-08-03T07:14:16Z

test/Generics/existential_restrictions.swift

@@ -17,7 +17,7 @@ func fAOE(_ t: AnyObject) { }
 func fT<T>(_ t: T) { }

 func testPassExistential(_ p: P, op: OP, opp: OP & P, cp: CP, sp: SP, any: Any, ao: AnyObject) {
-  fP(p) // expected-error{{cannot invoke 'fP' with an argument list of type '(P)'}} // expected-note{{expected an argument list of type '(T)'}}
+  fP(p) // expected-error{{using 'P' as a concrete type conforming to protocol 'P' is not supported}}


slavapestov · 2018-08-03T07:14:33Z

test/Generics/unbound.swift

-  return foor20792596(x) // expected-error {{generic parameter 'T' could not be inferred}}
+  return foor20792596(x)
+  // expected-error@-1 {{missing argument label 'x:' in call}}
+  // expected-error@-2 {{argument type 'T' does not conform to expected type 'r20792596P'}}


slavapestov · 2018-08-03T07:15:01Z

test/Serialization/builtin.swift

@@ -11,5 +11,4 @@ var a : TheBuiltinInt64

 // Check that it really is Builtin.Int64.
 var wrapped = Int64(a) // okay
-var badWrapped = Int32(a) // expected-error{{cannot invoke initializer for type 'Int32' with an argument list of type '(TheBuiltinInt64)'}}
-// expected-note @-1 {{overloads for 'Int32' exist with}}
+var badWrapped = Int32(a) // expected-error{{'Int32' requires that 'TheBuiltinInt64' conform to 'BinaryInteger'}}


Would be clearer if it said "initializer for 'Int32'"

slavapestov · 2018-08-03T07:32:15Z

Note that I didn’t review the code in detail, just the diagnostic changes. @rudkx should do that. But I think this and the other change to diagnose argument label mismatches as fixes are the right approach overall. Do you think soon you’ll be able to start ripping out code from CSDiag?

xedin · 2018-08-03T07:40:11Z

Yes, I hope to start removing logic from CSDiag soon. For example, I want to get out-of-order and missing arguments diagnosed via fixes, which would give it parity with what we have in CSDiag and, in theory, would allow us to remove ArgumentMatcher and related logic. Once we get all of the requirement kinds diagnosed via fixes, we can remove some of the logic related to failing constraint diagnostics fallback. I also have some ideas how to diagnose contextual errors with arguments/result types but that would require more changes to constraint system.

DougGregor

These are great improvements to the diagnostics! I added a few comments/questions, but none that should block merging.

DougGregor · 2018-08-03T17:06:15Z

lib/Sema/CSApply.cpp

@@ -8148,6 +8149,95 @@ bool ConstraintSystem::applySolutionFix(
                                      fix.first.getArgumentLabels(*this),
                                      isa<SubscriptExpr>(call->getFn()));
  }
+
+  case FixKind::AddConformace: {


Can we extract this case into a different function? It's a ton of code

I was thinking about doing that in the separate PR because I think we need to refactor applySolutionFixes into Solution itself, add diagnoseFix instead of applySolutionFix, and extract this logic into fixConformance, WDYT?

DougGregor · 2018-08-03T17:19:17Z

lib/Sema/CSApply.cpp

+    auto protocolType = conformance.second->getDeclaredType();
+
+    //  Find `ApplyExpr` based on a function expression attached to it.
+    auto findApplyExpr = [](Expr *parent, Expr *fnExpr) -> ApplyExpr * {


I'm a little surprised that we have to go searching for the ApplyExpr and matching up the parameter. Doesn't the (pre-simplified) locator contain the ApplyExpr, as well as information about which parameter we're talking about? Or is that still hard to get to because of the matchTypes/function call mess?

Yes, unfortunately that exactly the reason, currently generic requirements are opened from function expression, which means that it would have DeclRefExpr or UnresolvedDotExpr or OverloadedDeclRefExpr as its anchor and opened generic -> type requirement # as its path...

DougGregor · 2018-08-03T17:19:46Z

lib/Sema/CSApply.cpp

+      // If this is a static, initializer or operator call,
+      // let's not try to diagnose it here, but refer to expression
+      // diagnostics.
+      if (isa<BinaryExpr>(applyExpr) || isa<TypeExpr>(anchor))


I would have expected that these diagnostics would be better for operators (at least), but okay.

There are some problems with them currently, where current way of listing overload choices is better IMO, otherwise it would just say something like Int does not conform to RangeReplaceableCollection :(

DougGregor · 2018-08-03T17:24:33Z

lib/Sema/CSApply.cpp

+      auto *argExpr = getArgumentAt(applyExpr, *atParameterPos);
+      TC.diagnose(argExpr->getLoc(),
+                  diag::cannot_convert_argument_value_protocol, type,
+                  protocolType);


Oh! Oh! In many cases, we can probably resolveapplyExpr->getFn() down to a declaration, in which case we can produce a note pointing to the declaration we are trying to call. Bonus points for finding the type variable bindings when that declaration is generic. It would be super amazing if we could do something like this:

note: in call to function 'foo(blah:)' [with T=Int, U=String]

Indeed! I've mentioned that in reply to Slava's comment - I'm going to change the diagnostic to <decl-kind> <name> requires that 'X' conform to 'Y' [with T = X]

DougGregor · 2018-08-03T17:25:50Z

lib/Sema/CSSimplify.cpp

+      auto typeRequirement = path.back();
+      std::pair<Expr *, unsigned> reqLoc = {anchor, typeRequirement.getValue()};
+      MissingConformances[reqLoc] = {type.getPointer(), protocol};
+      // Let's strip all of the uncessary information from locator,


Typo "uncessary"

DougGregor · 2018-08-03T17:27:34Z

lib/Sema/CSSimplify.cpp

+      // Let's strip all of the uncessary information from locator,
+      // diagnostics only care about anchor - to lookup type,
+      // and what was the requirement# which is not satisfied.
+      ConstraintLocatorBuilder requirement(getConstraintLocator(anchor));


Ah, so this is what helps us be sure we can find the missing conformance information later. Makes sense.

rudkx · 2018-08-03T20:27:15Z

lib/Sema/ConstraintSystem.h

@@ -1009,6 +1009,12 @@ class ConstraintSystem {
  /// Argument labels fixed by the constraint solver.
  SmallVector<std::vector<Identifier>, 4> FixedArgLabels;

+  /// Conformances which solver "fixed" to exist to help with


typo? "to exist to help", perhaps just "to help"?

rudkx

LGTM - very nice improvement.

…gnostic

…formance failures Having `.Type` in `requires conformance` diagnostic is unnecessary.

xedin · 2018-08-03T23:55:18Z

I'll split the rest of the changes into multiple separate PR refactoring and extraction of diagnostics could be done separately.

xedin · 2018-08-03T23:57:44Z

@swift-ci please test

DougGregor · 2018-08-06T15:27:12Z

Excellent, thank you!

xedin added 5 commits August 2, 2018 16:40

[TypeChecker] NFC: Fix all of the diagnostics improved by conformance…

c2bf3d5

… tracking

[Diagnostics] NFC: Add another test-case improve my tracking missing …

c60e517

…conformances

xedin requested review from rudkx and DougGregor August 3, 2018 05:02

slavapestov approved these changes Aug 3, 2018

View reviewed changes

DougGregor approved these changes Aug 3, 2018

View reviewed changes

rudkx reviewed Aug 3, 2018

View reviewed changes

rudkx approved these changes Aug 3, 2018

View reviewed changes

xedin added 3 commits August 3, 2018 14:27

[Sema] NFC: reword "only concrete types can conform to protocols" dia…

f15e17a

…gnostic

[ConstraintSystem] NFC: Fix typo in FixKind::AddConformance name

9d50122

[ConstraintSystem] Look through metatype of base while diagnosing con…

e4da7a1

…formance failures Having `.Type` in `requires conformance` diagnostic is unnecessary.

swiftlang deleted a comment from swift-ci Aug 3, 2018

xedin merged commit 913f371 into swiftlang:master Aug 4, 2018

BasThomas mentioned this pull request Aug 6, 2018

[115] Issue #115 - August 9, 2018 SwiftWeekly/swiftweekly.github.io#404

Closed

[ConstraintSystem] Diagnose missing conformance requirements via "fixes" #18484

[ConstraintSystem] Diagnose missing conformance requirements via "fixes" #18484

Uh oh!

Conversation

xedin commented Aug 3, 2018

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Aug 3, 2018

Uh oh!

xedin commented Aug 3, 2018

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rudkx left a comment

Choose a reason for hiding this comment

Uh oh!

xedin commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Aug 3, 2018

Uh oh!

DougGregor commented Aug 6, 2018

Uh oh!

Uh oh!

xedin commented Aug 3, 2018 •

edited

Loading