[Diagnostics] Add a detailed diagnostic for generic argument mismatches #25060

sl · 2019-05-25T06:57:05Z

Adds a detailed diagnostic for generic argument mismatches. The new diagnostic covers both existing cases that would have been diagnosed as cannot convert X<Y> to X<Z>, providing more information, and takes the place of some diagnostics generated by CSDiag which provided little help.

The new diagnostic has a few forms depending on the context it's used in, but here's an example of it with assignment:

struct A<X, Y, Z> {}

let a: A<Int, Int, Int> = A<Int, Int, Int>()
let b: A<Bool, Int, Float> = a

Which produces the following diagnostics:

example.swift:4:30: error: cannot convert value of type 'A<Int, Int, Int>' to 'A<Bool, Int, Float>' in assignment, arguments to generic parameter 'X' ('Int' and 'Bool') are expected to be equal
let b: A<Bool, Int, Float> = a
                             ^
example.swift:1:10: note: generic parameter 'X' declared here
struct A<X, Y, Z> {}
         ^
example.swift:4:30: error: cannot convert value of type 'A<Int, Int, Int>' to 'A<Bool, Int, Float>' in assignment, arguments to generic parameter 'Z' ('Int' and 'Float') are expected to be equal
let b: A<Bool, Int, Float> = a
                             ^
example.swift:1:16: note: generic parameter 'Z' declared here
struct A<X, Y, Z> {}

Resolves: SR-10790

sl · 2019-05-25T07:00:40Z

@swift-ci please smoke test

sl · 2019-05-25T07:04:56Z

Some tests will likely fail as there may be a few tests I missed adding the new diagnostic to. I will patch those up tomorrow morning and re-run tests.

theblixguy · 2019-05-25T18:02:17Z

include/swift/AST/DiagnosticsSema.def

+      "arguments to generic parameter %2 (%3 and %4) are expected to be equal",
+      (Type, Type, Identifier, Type, Type))
+ERROR(cannot_convert_generic_type_dict_key,none,
+      "cannot convert value of type %0 to expected dictionary key type %1, "


Some of these diagnostics could be combined using the select syntax, but I suppose the way it’s being used in the code might make it tricky to do so.

Yeah, that's why I decided against utilizing it here. It would make the types of the diagnostic not line up so we wouldn't be able to unify the logic for actually producing the error message. As such, I think that's a trade off that's not worth making. I do wish I could make use of the select syntax here though but I think the benefits of having a single logical flow outweigh the benefits of having less diagnostics relating to this case.

sl · 2019-05-28T20:10:54Z

@swift-ci please smoke test

sl · 2019-05-28T22:16:34Z

@swift-ci please smoke test

sl · 2019-05-28T23:18:05Z

@swift-ci please smoke test

sl · 2019-05-29T03:01:48Z

@swift-ci please smoke test

xedin

Overall this is great! I have left some comments inline we need to address before merging.

xedin · 2019-05-29T17:55:55Z

lib/Sema/CSFix.h

@@ -88,6 +88,9 @@ enum class FixKind : uint8_t {
  /// like the types are aligned.
  ContextualMismatch,

+  /// Fix up the generic arguments of two types so they match eachother.


Nit: "each other".

xedin · 2019-05-29T18:02:16Z

lib/Sema/CSSimplify.cpp

-                       ConstraintLocatorBuilder locator) {
+static ConstraintSystem::TypeMatchResult matchDeepTypeArguments(
+    ConstraintSystem &cs, ConstraintSystem::TypeMatchOptions subflags,
+    BoundGenericType *base1, BoundGenericType *base2, ArrayRef<Type> args1,


I think we need to make this more flexible to e.g. accommodate opaque result types, so maybe it would be easier to add a callback e.g. llvm::function_ref<void(unsigned)> (which does nothing by default) here to capture each mismatched argument position and then decide what to do in the call site?

xedin · 2019-05-29T18:09:58Z

lib/Sema/CSSimplify.cpp

@@ -2163,6 +2202,18 @@ bool ConstraintSystem::repairFailures(
  }

  case ConstraintLocator::ClosureResult: {
+    // If we could record a generic arguments mismatch instead of this fix,
+    // don't record a ContextualMismatch here.
+    auto hasDeepEqualityRestriction = llvm::any_of(


Let's get this generalized for any ConversionRestrictionKind and extracted into a separate closure it could be used in both places.

xedin · 2019-05-29T18:10:14Z

lib/Sema/CSSimplify.cpp

@@ -6837,7 +6902,7 @@ void ConstraintSystem::addConstraint(Requirement req,
    kind = ConstraintKind::Subtype;
    break;
  case RequirementKind::SameType:
-    kind = ConstraintKind::Bind;
+    kind = ConstraintKind::Equal;


I think this is unrelated.

xedin · 2019-05-29T18:10:44Z

lib/Sema/CSSimplify.cpp

@@ -4745,6 +4808,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(

    // FIXME(diagnostics): If there were no viable results, but there are
    // unviable ones, we'd have to introduce fix for each specific problem.
+    // TODO(diagnostics): Remove this, see what happens (should be noop)


Looks like something committed unintentionally.

xedin · 2019-05-29T18:59:31Z

test/decl/protocol/protocol_with_superclass_where_clause.swift

@@ -13,6 +13,12 @@ class Generic<T> : Concrete {

  func genericMethod(_: GenericAlias) {}
 }
+// expected-note@-5 {{generic parameter 'T' declared here}}


Here could be simplified too.

xedin · 2019-05-29T18:59:41Z

test/decl/typealias/generic.swift

@@ -3,6 +3,11 @@
 struct MyType<TyA, TyB> { // expected-note {{generic type 'MyType' declared here}}
  var a : TyA, b : TyB
 }
+// expected-note@-3 {{generic parameter 'TyA' declared here}}


xedin · 2019-05-29T19:00:54Z

include/swift/AST/DiagnosticsSema.def

@@ -394,6 +394,51 @@ ERROR(cannot_convert_closure_result_protocol,none,
 ERROR(cannot_convert_closure_result_nil,none,
      "'nil' is not compatible with closure result type %0", (Type))

+ERROR(cannot_convert_generic_type_argument,none,
+      "cannot convert %0 to expected argument type %1, "
+      "arguments to generic parameter %2 (%3 and %4) are expected to be equal",


Since this note is the same for all diagnostics, let's maybe make it a note and attach it to the same location as error?

Overall after looking at all of the diagnostic changes it seems like it might be test to have original messages with added notes about mismatched generic parameters, WDYT?

I think that probably is the best approach. My one question is would there be too many notes for this diagnostic to be useful? We'd be emitting one note pointing to the mismatch at the location itself, and one note pointing to the declaration of the generic parameter per mismatch which i feel could get confusing.

I think we can remove declared here note in favor of just pointing new note to the parameter declaration then? So in example from description it would look like:

example.swift:4:30: error: cannot assign value of type 'A<Int, Int, Int>' to type 'A<Bool, Int, Float>' let b: A<Bool, Int, Float> = a ^ example.swift:1:10: note: arguments to generic parameter 'X' ('Int' and 'Bool') are expected to be equal struct A<X, Y, Z> {} ^

or to the type repr location e.g.

example.swift:4:30: error: cannot assign value of type 'A<Int, Int, Int>' to type 'A<Bool, Int, Float>' let b: A<Bool, Int, Float> = a ^ example.swift:1:10: note: arguments to generic parameter 'X' ('Int' and 'Bool') are expected to be equal let b: A<Bool, Int, Float> = a ^

So is the idea that we attempt to put the diagnostic at the first one where possible, and fall back to the second one if that source location wasn't valid? If so, I think that seems good!

Sure, that sounds good to me.

xedin · 2019-05-29T20:23:45Z

test/expr/cast/set_coerce.swift

@@ -18,4 +18,4 @@ var setD = Set<D>()

 // Test set upcasts
 setC = setD
-setD = setC // expected-error{{cannot assign value of type 'Set<C>' to type 'Set<D>'}}
+setD = setC // expected-error{{cannot convert value of type 'Set<C>' to 'Set<D>' in assignment, arguments to generic parameter 'Element' ('C' and 'D') are expected to be equal}}


Actually it seems like for assignment it make make sense to go with original text "cannot assign value of type ... to type ...`

Yeah, on second thought, that does look much nicer. I'll make that change in my next round of revisions 👍

xedin · 2019-05-29T20:24:16Z

test/stdlib/UnsafePointerDiagnostics.swift

  _ = UnsafePointer<String>(upi) // expected-error {{cannot convert value of type 'UnsafePointer<Int>' to expected argument type 'RawPointer'}}
-  _ = UnsafeMutablePointer<String>(umpi) // expected-error {{cannot convert value of type 'UnsafeMutablePointer<Int>' to expected argument type 'UnsafeMutablePointer<_>'}}
+  _ = UnsafeMutablePointer<String>(umpi) // expected-error {{cannot convert 'UnsafeMutablePointer<Int>' to expected argument type 'UnsafeMutablePointer<_>', arguments to generic parameter 'Pointee' ('Int' and '_') are expected to be equal}}


Lets preserve value of type in the message.

sl · 2019-06-11T01:49:21Z

@swift-ci please smoke test

sl · 2019-06-11T12:57:53Z

@swift-ci please smoke test

sl · 2019-06-11T14:02:15Z

@swift-ci please smoke test

include/swift/AST/DiagnosticsSema.def

lib/Sema/CSSimplify.cpp

sl · 2019-06-13T04:11:06Z

@swift-ci please smoke test

sl · 2019-06-13T04:21:52Z

@swift-ci please smoke test

sl · 2019-06-13T04:38:00Z

test/Constraints/keypath.swift

@@ -31,6 +31,9 @@ class Demo {
 }

 // FIXME: This error is better than it was, but the diagnosis should break it down more specifically to 'here's type.
+// TODO(diagnostics): Fix this regression, we want the following error message ideally


Not sure what the best way to address this regression is. Figured I'd post a version with the other review comments addressed and leave this in for the moment. @xedin Thoughts?

It's not incorrect to produce that diagnostic really :) Let me pull your changes locally and see what could be done, I think it should be possible to default type variables found in the left-hand side to Any which is going to take affect only if the only way they get bindings is from context.

sl · 2019-06-13T06:24:46Z

@swift-ci please smoke test

sl · 2019-06-13T16:16:18Z

@swift-ci please smoke test

xedin

Looks great, I have left some refactoring feedback but it looks like the logic is correct now!

xedin · 2019-06-13T18:41:33Z

lib/Sema/CSSimplify.cpp

+        *this, bound1, bound2, failedArguments, getConstraintLocator(locator));
+    // Increase the solution's score for each mismtach this fixes.
+    for (unsigned i = 0; i < failedArguments.size(); ++i) {
+      increaseScore(SK_Fix);


This could be simplified to increaseScore(SK_Fix, failedArguments.size());

Also I suggest to increase score only if the fix has been recorded successfully e.g.

if (!recordFix(fix)) { increaseScore(SK_Fix, failedArguments.size()); return getTypeMatchSuccess(); }

xedin · 2019-06-13T18:53:14Z

lib/Sema/CSSimplify.cpp

+    ConstraintSystem &cs, ConstraintSystem::TypeMatchOptions subflags,
+    ArrayRef<Type> args1, ArrayRef<Type> args2,
+    ConstraintLocatorBuilder locator,
+    llvm::function_ref<bool(unsigned)> recordMismatch = [](unsigned) { return false; }) {


See my later comment, this callback doesn't really have to return anything.

xedin · 2019-06-13T18:53:49Z

lib/Sema/CSSimplify.cpp

+  auto result =
+      matchDeepTypeArguments(*this, subflags, args1, args2, locator,
+                             [&](unsigned position) {
+                                if (forRequirement || isOptional || isArray)


I think this check belongs at top level. This block could be simplified to:

if (shouldAttemptFixes()) { // Optionals, arrays, requirements etc. if (<check for additional conditions where fix can't be applied>) return matchDeepTypeArguments(*this, subflags, args1, args2, locator); SmallVector<unsigned, 4> mismatches; auto result = matchDeepTypeArguments(*this, subflags, args1, args2, locator, [&mismatches](unsigned position) { mismatches.push_back(mismatchPosition); }); if (mismatches.empty()) return result; // record the fix and increase the score return <result>; } return matchDeepTypeArguments(*this, subflags, args1, args2, locator);

xedin · 2019-06-13T18:55:32Z

test/decl/protocol/protocol_with_superclass.swift

@@ -43,8 +43,8 @@ extension ProtoRefinesClass {
    let _: BaseProto & Generic<Int> = self
    let _: BaseProto & Concrete = self

-    let _: Generic<String> = self
-    // expected-error@-1 {{cannot convert value of type 'Self' to specified type 'Generic<String>'}}
+    let _: Generic<String> = self;


Extraneous semi-colon here.

sl · 2019-06-14T00:58:13Z

@swift-ci please smoke test

sl · 2019-06-14T02:07:13Z

@swift-ci please smoke test

sl · 2019-06-14T02:17:08Z

@swift-ci please smoke test

sl · 2019-06-14T03:17:55Z

@swift-ci please smoke test

sl · 2019-06-14T16:34:29Z

@swift-ci please smoke test

…atchFailure Additionally, fixed a crash caused by the change relating to opaque types.

…tDiagnosticFor

…agnostic

… recorded anything

sl · 2019-06-14T16:38:03Z

@swift-ci please smoke test

sl · 2019-06-14T16:41:49Z

@swift-ci please smoke test

xedin · 2019-06-14T18:07:55Z

sl requested a review from xedin May 25, 2019 06:57

sl force-pushed the generic-arguments-mismatch-diagnostic branch from 545be8c to 5347bb7 Compare May 25, 2019 06:59

theblixguy reviewed May 25, 2019

View reviewed changes

sl force-pushed the generic-arguments-mismatch-diagnostic branch from da49966 to 7b92eb9 Compare May 28, 2019 22:16

sl force-pushed the generic-arguments-mismatch-diagnostic branch from 7b92eb9 to 71fdd37 Compare May 28, 2019 23:15

xedin requested changes May 29, 2019

View reviewed changes

sl force-pushed the generic-arguments-mismatch-diagnostic branch from f711de5 to e593791 Compare June 11, 2019 01:48

sl force-pushed the generic-arguments-mismatch-diagnostic branch from e593791 to 4772c1f Compare June 11, 2019 12:57

xedin reviewed Jun 12, 2019

View reviewed changes

include/swift/AST/DiagnosticsSema.def Outdated Show resolved Hide resolved

lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved

lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved

lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved

sl commented Jun 13, 2019

View reviewed changes

sl force-pushed the generic-arguments-mismatch-diagnostic branch from 30594da to e32654e Compare June 13, 2019 05:09

xedin approved these changes Jun 13, 2019

View reviewed changes

sl force-pushed the generic-arguments-mismatch-diagnostic branch from 714ad3f to 4d56dff Compare June 14, 2019 02:06

sl force-pushed the generic-arguments-mismatch-diagnostic branch from 4d56dff to 798b81b Compare June 14, 2019 03:17

sl force-pushed the generic-arguments-mismatch-diagnostic branch from 798b81b to 9111ea2 Compare June 14, 2019 16:34

sl added 12 commits June 14, 2019 12:35

Sema: Add a more descriptive diagnostic for generic argument mismatches

7719b0f

Test: Add tests for the new generic arguments mismatch diagnostic

dc5ccd1

Sema / Test: Fix tests broken by introduction of GenericArgumentsMism…

81dc546

…atchFailure Additionally, fixed a crash caused by the change relating to opaque types.

Sema: Changed GenericArgumentsMismatch to use trailing objects

1701ea1

Test: Update tests to reflect change to generic mismatch note locations

de7851b

Sema: Handle CTP_SubscripAssignSource in GenericArgumentsMismatch::ge…

0a7cdb8

…tDiagnosticFor

Sema: Fix how matchDeepEqualityType failures are handled

cc8c2b1

Sema: Change generic argument mismatch diags to reuse existing ones

122a68c

Test: Add a todo about a regression introduced by generic mismatch di…

cd0cc3c

…agnostic

Sema: Change closure passed to matchDeepTypeArguments to return if it…

64b1186

… recorded anything

Test: Update additional tests with improved generic mismatch diagnostic

2cefbe0

Sema: Refactor logic for excluding arrays / optionals out of closure

ff950a4

sl force-pushed the generic-arguments-mismatch-diagnostic branch from 9111ea2 to ff950a4 Compare June 14, 2019 16:37

sl merged commit 2397dea into swiftlang:master Jun 14, 2019

swift-ci mentioned this pull request May 29, 2019

[SR-10790] Add a detailed diagnostic for generic argument mismatches #53180

Closed

xedin mentioned this pull request Jan 11, 2020

[SR-11137] Type inference compiler error #53533

Closed

[Diagnostics] Add a detailed diagnostic for generic argument mismatches #25060

[Diagnostics] Add a detailed diagnostic for generic argument mismatches #25060

Uh oh!

Conversation

sl commented May 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sl commented May 25, 2019

Uh oh!

sl commented May 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theblixguy May 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sl May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sl commented May 28, 2019

Uh oh!

sl commented May 28, 2019

Uh oh!

sl commented May 28, 2019

Uh oh!

sl commented May 29, 2019

Uh oh!

xedin 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!

sl May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sl commented Jun 11, 2019

Uh oh!

sl commented Jun 11, 2019

Uh oh!

sl commented Jun 11, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sl commented Jun 13, 2019

Uh oh!

sl commented Jun 13, 2019

Uh oh!

Choose a reason for hiding this comment

sl commented May 25, 2019 •

edited

Loading

sl commented May 25, 2019 •

edited

Loading

theblixguy May 25, 2019 •

edited

Loading

sl May 28, 2019 •

edited

Loading

sl May 30, 2019 •

edited

Loading