[ConstraintSystem] Port tuple type mismatch diagnostics #28600

hborla · 2019-12-05T22:33:38Z

Diagnose tuple element mismatches and tuple labeling failures via constraint fixes.

hborla · 2019-12-05T22:38:42Z

@swift-ci please smoke test

hborla · 2019-12-05T22:43:10Z

@swift-ci please smoke test

LucianoPAlmeida · 2019-12-06T00:03:39Z

lib/Sema/CSDiagnostics.cpp

@@ -2963,8 +2965,11 @@ ContextualFailure::getDiagnosticFor(ContextualTypePurpose context,
 bool TupleContextualFailure::diagnoseAsError() {
  auto diagnostic = isNumElementsMismatch()
                        ? diag::tuple_types_not_convertible_nelts
-                        : diag::tuple_types_not_convertible;
-  emitDiagnostic(getAnchor()->getLoc(), diagnostic, getFromType(), getToType());
+                        : getDiagnosticFor(getContextualTypePurpose(), false);


Suggested change

: getDiagnosticFor(getContextualTypePurpose(), false);

: getDiagnosticFor(getContextualTypePurpose(),

/*forProtocol=*/ false);

Sorry for random comment in your PR, but maybe this nit make sense :))

Thanks! I'll add this in

hamishknight · 2019-12-06T01:28:37Z

Sorry, #28593 caused a slight conflict by changing the return type of findSelectedOverloadFor

xedin

Overall looks good! It would be great to address a comments about the diagnostic message and static methods I've left inline before merging this.

xedin · 2019-12-05T22:55:51Z

lib/Sema/CSDiagnostics.h

+  /// application of the argument to its parameter. If the locator is not
+  /// for an argument conversion, returns \c None.
+  static Optional<FunctionArgApplyInfo>
+  getFunctionArgApplyInfo(ConstraintSystem &cs, ConstraintLocator *locator);


I see that this method is now used by a fix but I think it's just too big of a hammer for just that one use-case, please consider porting only the logic useful to retrieve types inline...

Important reason why getFunctionArgApplyInfo and resolveInterfaceType are members of FailureDiagnostic - it serves as a way to hide that it depends on a particular constraint system (it should use the same one associated with the failure and locator anyway), also in the future when CSDiag is gone we'd want to only deal with Solution and avoid passing whole constraint system (or applying solution back to it) to diagnostic so it these methods would have to be brought back under FailureDiagnostic again...

I don't think removing the dependency on ConstraintSystem and using Solution instead necessarily means that we'd need to make it an instance method again. The methods that we call on ConstraintSystem in getFunctionArgApplyInfo are either already are implemented or can be implemented on Solution. I don't see much of a difference between passing in a ConstraintSystem vs calling getConstraintSystem() on the first line - we'll have to solve the same problems either way when we make the switch over to Solution because we call methods directly on ConstraintSystem here, even before my changes. I think it would even make sense to implement this on Solution, since it's really just computing information about a particular solution and isn't related to FailureDiagnostic otherwise.

That said, the first thing I tried was implementing it inline. It needs so much of the logic from this function - everything up until the point where we find the interface type of the function because the first ~60 lines of code are just for finding the function type

If we want to keep using it that way it would have to be moved to constraint system itself then because it no longer belongs to the diagnostic.

This also points to the bigger problem with AllowTupleTypeMismatch fix - fixes supposed to be recording useful information and in cases where locator is pointing to argument/parameter comparison repairFailures would already provide both argument and parameter types...

The problem is that when we're in repairFailures due to a tuple element mismatch for an argument to a function, we don't have the full argument and parameter type. We only fail at the particular element, so we don't actually have those types when we create the fix.

Alright, let's just move this logic to be a member of constraint system until we figure out a better solution.

xedin · 2019-12-05T22:59:37Z

lib/Sema/CSSimplify.cpp

@@ -8420,6 +8436,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
    // when the tuples (X, Y, $1) and (X, $0, B) get matched, $0 is equated
    // to Y, $1 is equated to B, and $2 is defaulted to Any.
    auto lhsLarger = lhs->getNumElements() >= rhs->getNumElements();
+    auto isLabelingFailure = lhs->getNumElements() == rhs->getNumElements();


Matching number of elements indicates that the failure could either be an element type or a label, right?

Because of the way that we fail in the case of element mismatch vs arity/label mismatch, we only actually have a tuple type in the latter case

xedin · 2019-12-05T23:00:56Z

test/decl/var/variables.swift

@@ -96,7 +96,7 @@ func tuplePatternDestructuring(_ x : Int, y : Int) {
  _ = i+j

  // <rdar://problem/20395243> QoI: type variable reconstruction failing for tuple types
-  let (x: g1, a: h1) = (b: x, a: y)  // expected-error {{tuple type '(b: Int, a: Int)' is not convertible to tuple '(x: Int, a: Int)'}}
+  let (x: g1, a: h1) = (b: x, a: y)  // expected-error {{cannot convert value of type '(b: Int, a: Int)' to specified type '(x: Int, a: Int)'}}


It feels like in the particular case it would be better to produce original diagnostic because there is no "specified type" in this case, it's inferred for each element...

Yeah, this one is strange because the "specified" part is actually the labels. This is the diagnostic for the CTP_Initialization purpose. I think I can distinguish this case though, because there is no contextual type here even though there is a contextual type purpose

xedin · 2019-12-05T23:12:38Z

lib/Sema/CSFix.cpp

-  auto failure = TupleContextualFailure(
-      getConstraintSystem(), getFromType(), getToType(), getLocator());
+bool AllowTupleTypeMismatch::coalesceAndDiagnose(
+    ArrayRef<ConstraintFix *> fixes, bool asNote) const {


This looks very strange to me especially because fixes are unused... As far as understand the reason why we need that at the moment is related to the fact that the last element of the locator is stripped so all of the fixes, instead of pointing each to its TupleElement, points to the tuple itself, is that correct? Would it make sense to produce a special kind of fix for each of these (sub-)element problems and only implement diagnoseAsNote for associated diagnostic so the main tuple diagnostic could use them?

We'll actually need those fixes when we want to keep track of which tuple elements didn't match. I'm going to add that back now so it's ready for when we add the note for which elements didn't match.

The reason this is necessary is because when we create the AllowTupleTypeMismatch fix, we only know the type of the specific element that didn't match - we don't know the full tuple type to put it into context. We're computing that in this method, and we want to coalesce the fixes together because we don't want to report the same error for each part of the tuple that didn't match. The reason why the fixes aren't being used here is because the fixes only contain the isolated element type that didn't match, but I'm changing it to also include the element index (the locator has this info before we strip it off). So, if we're matching tuple types (Int, Float) and (Int, Int), when we create the fix we only know that Int and Float didn't match. There is no overarching tuple diagnostic in this case.

It does seem strange to me that we're never using these the types that we record in the case of an element mismatch, though. I considered creating separate fixes for element mismatch, label mismatch, and artity mismatch, but tuples can have all three errors and with the way we're reporting it, it makes sense to have one error message for the contextual mismatch that contains the full tuple types and notes saying which parts structurally are wrong (i.e. tuples have a different type for element #1, tuples have a different number of elements, etc)

This way of dealing with structural failures seems a bit too complicated to me...

Maybe we should start with simpler approach and consider element to be label + type that way either one or both are going to be the same failure. I think it might make sense to consider aggregating element mismatches in matchTupleTypes where we have access to both top level tuples and also do matching of their elements. It seems like we don't really need to record particulars about how types mismatched, just the fact that they did (so if https://github.com/apple/swift/blob/master/lib/Sema/CSSimplify.cpp#L1218 fails we record the position just like we do in fix for generic arguments) which means that we can potentially keep this all contained in one place...

The only problem with that approach might be when one side is generic parameter but we can also record anything unsolved as potential failure and skip it in repairFailures for TupleElement if there is a top level fix for that already. Do you know if any examples like that?

Yes, this is a problem when we have any kind of type variable that we're matching in the tuples:

func foo(_ x: (Int, Double)) {} foo((1.0, 2.0))

matchTupleTypes "succeeds" because we're comparing ($T1, $T2) and (Int, Double). In this case, matchTupleTypes is only called during CSGen.

Hm, yeah this is really unfortunately and we can't delay destructuring because some of these types could be contextual... Maybe we should expend the locator with tuple element e.g. tuple -> tuple element #N?

It seems like we need to introduce a generalized ApplyArgToParam element for structural comparisons like that to make it work.

Yeah, that's exactly the information we would need. I think we're going to hit the same problem with matchFunctionTypes as we have here

to-do list.

into `ConstraintSystem`.

types do not match in AllowTupleTypeMismatch/TupleContextualFailure.

when there is no contextual type.

hborla · 2019-12-09T19:29:01Z

@swift-ci please smoke test

hborla requested a review from xedin December 5, 2019 22:33

hborla force-pushed the tuple-mismatch-diagnostics branch from 5e00f08 to 6f7a438 Compare December 5, 2019 22:42

LucianoPAlmeida reviewed Dec 6, 2019

View reviewed changes

xedin reviewed Dec 6, 2019

View reviewed changes

hborla added 3 commits December 6, 2019 13:12

[ConstraintSystem] Port tuple type mismatches to the new framework

f9a1ab2

[Docs] Remove visitTupleExpr from the New Diagnostic Architecture

ff53718

to-do list.

[ConstraintSystem] Move FailureDiagnostic::getFunctionArgApplyInfo

1e013a0

into `ConstraintSystem`.

hborla force-pushed the tuple-mismatch-diagnostics branch from 6f7a438 to 1e013a0 Compare December 6, 2019 23:44

hborla added 2 commits December 9, 2019 11:04

[Diagnostics] Keep track of the indices of the tuple elements whose

50abedd

types do not match in AllowTupleTypeMismatch/TupleContextualFailure.

[Diagnostics] Use a generic "tuple type not convertible" error message

94e9fd1

when there is no contextual type.

hborla merged commit edb4e70 into swiftlang:master Dec 9, 2019

hborla deleted the tuple-mismatch-diagnostics branch July 13, 2020 16:00

	: getDiagnosticFor(getContextualTypePurpose(), false);
	: getDiagnosticFor(getContextualTypePurpose(),
	/forProtocol=/ false);

[ConstraintSystem] Port tuple type mismatch diagnostics #28600

[ConstraintSystem] Port tuple type mismatch diagnostics #28600

Uh oh!

Conversation

hborla commented Dec 5, 2019

Uh oh!

hborla commented Dec 5, 2019

Uh oh!

hborla commented Dec 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hamishknight commented Dec 6, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hborla Dec 6, 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!

hborla commented Dec 9, 2019

Uh oh!

Uh oh!

hborla Dec 6, 2019 •

edited

Loading