[Diagnostics] Improve diagnostics for implicit (un)tupling. (#28076)

Fixes rdar://problem/56436226.

Author: varungandhi-apple

include/swift/AST/DiagnosticsSema.def

@@ -3694,18 +3694,14 @@ ERROR(pattern_type_mismatch_context,none,
 
 ERROR(tuple_pattern_in_non_tuple_context,none,
       "tuple pattern cannot match values of the non-tuple type %0", (Type))
-WARNING(matching_pattern_with_many_assoc_values, none,
-        "cannot match several associated values at once, "
-        "implicitly tupling the associated values and trying to match that "
-        "instead", ())
-WARNING(matching_tuple_pattern_with_many_assoc_values,none,
-        "a tuple pattern cannot match several associated values at once, "
-        "implicitly tupling the associated values and trying to match "
-        "that instead", ())
-WARNING(matching_many_patterns_with_tupled_assoc_value,none,
-        "the enum case has a single tuple as an associated value, but "
-        "there are several patterns here, implicitly tupling the patterns "
-        "and trying to match that instead", ())
+WARNING(found_one_pattern_for_several_associated_values,none,
+        "enum case '%0' has %1 associated values; matching them as a tuple "
+        "is deprecated", (StringRef, unsigned))
+WARNING(converting_tuple_into_several_associated_values,none,
+        "enum case '%0' has %1 associated values", (StringRef, unsigned))
+WARNING(converting_several_associated_values_into_tuple,none,
+        "enum case '%0' has one associated value that is a tuple of %1 "
+        "elements",(StringRef, unsigned))
 ERROR(closure_argument_list_tuple,none,
       "contextual closure type %0 expects %1 argument%s1, "
       "but %2 %select{were|was}3 used in closure body", (Type, unsigned, unsigned, bool))

lib/Sema/TypeCheckPattern.cpp

@@ -827,50 +827,89 @@ llvm::Expected<Type> PatternTypeRequest::evaluate(
   llvm_unreachable("bad pattern kind!");
 }
 
-namespace {
-
+/// Potentially tuple/untuple a pattern before passing it to the pattern engine.
+///
 /// We need to allow particular matches for backwards compatibility, so we
-/// "repair" the pattern if needed, so that the exhaustiveness checker receives
-/// well-formed input. Also emit diagnostics warning the user to fix their code.
+/// "repair" the pattern if needed. This ensures that the pattern engine
+/// receives well-formed input, avoiding the need to implement an additional
+/// compatibility hack there, as doing that is lot more tricky due to the
+/// different cases that need to handled.
+///
+/// We also emit diagnostics and potentially a fix-it to help the user.
 ///
 /// See SR-11160 and SR-11212 for more discussion.
 //
 // type ~ (T1, ..., Tn) (n >= 2)
-//   1a. pat ~ ((P1, ..., Pm)) (m >= 2)
-//   1b. pat
+//   1a. pat ~ ((P1, ..., Pm)) (m >= 2) -> untuple the pattern
+//   1b. pat (a single pattern, not a tuple) -> handled by pattern engine
 // type ~ ((T1, ..., Tn)) (n >= 2)
-//   2. pat ~ (P1, ..., Pm) (m >= 2)
-void implicitlyUntuplePatternIfApplicable(ASTContext &Ctx,
-                                          Pattern *&enumElementInnerPat,
-                                          Type enumPayloadType) {
+//   2. pat ~ (P1, ..., Pm) (m >= 2) -> tuple the pattern
+static
+void repairTupleOrAssociatedValuePatternIfApplicable(
+    ASTContext &Ctx,
+    Pattern *&enumElementInnerPat,
+    Type enumPayloadType,
+    const EnumElementDecl *enumCase) {
   auto &DE = Ctx.Diags;
+  bool addDeclNote = false;
   if (auto *tupleType = dyn_cast<TupleType>(enumPayloadType.getPointer())) {
     if (tupleType->getNumElements() >= 2
         && enumElementInnerPat->getKind() == PatternKind::Paren) {
       auto *semantic = enumElementInnerPat->getSemanticsProvidingPattern();
       if (auto *tuplePattern = dyn_cast<TuplePattern>(semantic)) {
         if (tuplePattern->getNumElements() >= 2) {
-          DE.diagnose(tuplePattern->getLoc(),
-                      diag::matching_tuple_pattern_with_many_assoc_values);
+          auto diag = DE.diagnose(tuplePattern->getLoc(),
+              diag::converting_tuple_into_several_associated_values,
+              enumCase->getNameStr(), tupleType->getNumElements());
+          auto subPattern =
+            dyn_cast<ParenPattern>(enumElementInnerPat)->getSubPattern();
+
+          // We might also have code like
+          //
+          // enum Upair { case upair(Int, Int) }
+          // func f(u: Upair) { switch u { case .upair(let (x, y)): () } }
+          //
+          // This needs a more complex rearrangement to fix the code. So only
+          // apply the fix-it if we have a tuple immediately inside.
+          if (subPattern->getKind() == PatternKind::Tuple) {
+            auto leadingParen = SourceRange(enumElementInnerPat->getStartLoc());
+            auto trailingParen = SourceRange(enumElementInnerPat->getEndLoc());
+            diag.fixItRemove(leadingParen)
+                .fixItRemove(trailingParen);
+          }
+
+          addDeclNote = true;
           enumElementInnerPat = semantic;
         }
       } else {
         DE.diagnose(enumElementInnerPat->getLoc(),
-                    diag::matching_pattern_with_many_assoc_values);
+          diag::found_one_pattern_for_several_associated_values,
+          enumCase->getNameStr(),
+          tupleType->getNumElements());
+        addDeclNote = true;
       }
     }
   } else if (auto *tupleType = enumPayloadType->getAs<TupleType>()) {
-    if (tupleType->getNumElements() >= 2
-        && enumElementInnerPat->getKind() == PatternKind::Tuple) {
-      DE.diagnose(enumElementInnerPat->getLoc(),
-                  diag::matching_many_patterns_with_tupled_assoc_value);
-      enumElementInnerPat =
-        new (Ctx) ParenPattern(enumElementInnerPat->getStartLoc(),
-                               enumElementInnerPat,
-                               enumElementInnerPat->getEndLoc());
+    if (tupleType->getNumElements() >= 2) {
+      if (auto *tuplePattern = dyn_cast<TuplePattern>(enumElementInnerPat)) {
+        DE.diagnose(enumElementInnerPat->getLoc(),
+                    diag::converting_several_associated_values_into_tuple,
+                    enumCase->getNameStr(),
+                    tupleType->getNumElements())
+          .fixItInsert(enumElementInnerPat->getStartLoc(), "(")
+          .fixItInsertAfter(enumElementInnerPat->getEndLoc(), ")");
+        addDeclNote = true;
+        enumElementInnerPat =
+          new (Ctx) ParenPattern(enumElementInnerPat->getStartLoc(),
+                                 enumElementInnerPat,
+                                 enumElementInnerPat->getEndLoc());
+      }
     }
   }
-}
+
+  if (addDeclNote)
+    DE.diagnose(enumCase->getStartLoc(), diag::decl_declared_here,
+                enumCase->getFullName());
 }
 
 /// Perform top-down type coercion on the given pattern.
@@ -1450,7 +1489,8 @@ Pattern *TypeChecker::coercePatternToType(ContextualPattern pattern,
       newSubOptions.setContext(TypeResolverContext::EnumPatternPayload);
       newSubOptions |= TypeResolutionFlags::FromNonInferredPattern;
 
-      ::implicitlyUntuplePatternIfApplicable(Context, sub, elementType);
+      ::repairTupleOrAssociatedValuePatternIfApplicable(
+        Context, sub, elementType, elt);
 
       sub = coercePatternToType(
           pattern.forSubPattern(sub, /*retainTopLevel=*/false), elementType,

test/Parse/matching_patterns.swift

@@ -100,7 +100,8 @@ enum Voluntary<T> : Equatable {
       ()
 
     case .Twain(), // expected-error{{tuple pattern has the wrong length for tuple type '(T, T)'}}
-         .Twain(_), // expected-warning {{cannot match several associated values at once, implicitly tupling the associated values and trying to match that instead}}
+         .Twain(_), // expected-warning {{enum case 'Twain' has 2 associated values; matching them as a tuple is deprecated}}
+                    // expected-note@-25 {{'Twain' declared here}}
          .Twain(_, _),
          .Twain(_, _, _): // expected-error{{tuple pattern has the wrong length for tuple type '(T, T)'}}
       ()
@@ -143,7 +144,8 @@ case Voluntary<Int>.Mere,
      .Mere(_):
   ()
 case .Twain,
-     .Twain(_), // expected-warning {{cannot match several associated values at once, implicitly tupling the associated values and trying to match that instead}}
+     .Twain(_), // expected-warning {{enum case 'Twain' has 2 associated values; matching them as a tuple is deprecated}}
+                // expected-note@-69 {{'Twain' declared here}}
      .Twain(_, _),
      .Twain(_, _, _): // expected-error{{tuple pattern has the wrong length for tuple type '(Int, Int)'}}
   ()

test/Sema/exhaustive_switch.swift

@@ -1279,21 +1279,24 @@ enum SR11212Tests {
   func sr11212_content_untupled_pattern_tupled1(u: Untupled) -> (Int, Int) {
     switch u {
     case .upair((let x, let y)): return (x, y)
-    // expected-warning@-1 {{a tuple pattern cannot match several associated values at once, implicitly tupling the associated values and trying to match that instead}}
+    // expected-warning@-1 {{enum case 'upair' has 2 associated values}}{{16-17=}}{{31-32=}}
+    // expected-note@-7 {{'upair' declared here}}
     }
   }
 
   func sr11212_content_untupled_pattern_tupled2(u: Untupled) -> (Int, Int) {
     switch u {
     case .upair(let (x, y)): return (x, y)
-    // expected-warning@-1 {{a tuple pattern cannot match several associated values at once, implicitly tupling the associated values and trying to match that instead}}
+    // expected-warning@-1 {{enum case 'upair' has 2 associated values}} // No fix-it as that would require us to peek inside the 'let' :-/
+    // expected-note@-15 {{'upair' declared here}}
     }
   }
 
   func sr11212_content_untupled_pattern_tupled3(u: Untupled) -> (Int, Int) {
     switch u {
     case let .upair((x, y)): return (x, y)
-    // expected-warning@-1 {{a tuple pattern cannot match several associated values at once, implicitly tupling the associated values and trying to match that instead}}
+    // expected-warning@-1 {{enum case 'upair' has 2 associated values}}{{20-21=}}{{27-28=}}
+    // expected-note@-23 {{'upair' declared here}}
     }
   }
 
@@ -1312,14 +1315,16 @@ enum SR11212Tests {
   func sr11212_content_untupled_pattern_ambiguous1(u: Untupled) -> (Int, Int) {
     switch u {
     case .upair(let u_): return u_
-    // expected-warning@-1 {{cannot match several associated values at once, implicitly tupling the associated values and trying to match that instead}}
+    // expected-warning@-1 {{enum case 'upair' has 2 associated values; matching them as a tuple is deprecated}}
+    // expected-note@-43 {{'upair' declared here}}
     }
   }
 
   func sr11212_content_untupled_pattern_ambiguous2(u: Untupled) -> (Int, Int) {
     switch u {
     case let .upair(u_): return u_
-    // expected-warning@-1 {{cannot match several associated values at once, implicitly tupling the associated values and trying to match that instead}}
+    // expected-warning@-1 {{enum case 'upair' has 2 associated values; matching them as a tuple is deprecated}}
+    // expected-note@-51 {{'upair' declared here}}
     }
   }
 
@@ -1348,14 +1353,16 @@ enum SR11212Tests {
   func sr11212_content_tupled_pattern_untupled1(t: Tupled) -> (Int, Int) {
     switch t {
     case .tpair(let x, let y): return (x, y)
-    // expected-warning@-1 {{the enum case has a single tuple as an associated value, but there are several patterns here, implicitly tupling the patterns and trying to match that instead}}
+    // expected-warning@-1 {{enum case 'tpair' has one associated value that is a tuple of 2 elements}}{{16-16=(}}{{30-30=)}}
+    // expected-note@-25 {{'tpair' declared here}}
     }
   }
 
   func sr11212_content_tupled_pattern_untupled2(t: Tupled) -> (Int, Int) {
     switch t {
     case let .tpair(x, y): return (x, y)
-    // expected-warning@-1 {{the enum case has a single tuple as an associated value, but there are several patterns here, implicitly tupling the patterns and trying to match that instead}}
+    // expected-warning@-1 {{enum case 'tpair' has one associated value that is a tuple of 2 elements}}{{20-20=(}}{{26-26=)}}
+    // expected-note@-33 {{'tpair' declared here}}
     }
   }
 
@@ -1396,22 +1403,25 @@ enum SR11212Tests {
   func sr11212_content_generic_pattern_untupled1(b: Box<(Int, Int)>) -> (Int, Int) {
     switch b {
     case .box(let x, let y): return (x, y)
-    // expected-warning@-1 {{the enum case has a single tuple as an associated value, but there are several patterns here, implicitly tupling the patterns and trying to match that instead}}
+    // expected-warning@-1 {{enum case 'box' has one associated value that is a tuple of 2 elements}}{{14-14=(}}{{28-28=)}}
+    // expected-note@-25 {{'box' declared here}}
     }
   }
 
   func sr11212_content_generic_pattern_untupled2(b: Box<(Int, Int)>) -> (Int, Int) {
     switch b {
     case let .box(x, y): return (x, y)
-    // expected-warning@-1 {{the enum case has a single tuple as an associated value, but there are several patterns here, implicitly tupling the patterns and trying to match that instead}}
+    // expected-warning@-1 {{enum case 'box' has one associated value that is a tuple of 2 elements}}{{18-18=(}}{{24-24=)}}
+    // expected-note@-33 {{'box' declared here}}
     }
   }
 
   // rdar://problem/58578342
   func sr11212_content_generic_pattern_untupled3(b: Box<((Int, Int), Int)>) -> (Int, Int, Int) {
     switch b {
     case let .box((x, y), z): return (x, y, z)
-    // expected-warning@-1 {{the enum case has a single tuple as an associated value, but there are several patterns here, implicitly tupling the patterns and trying to match that instead}}
+    // expected-warning@-1 {{enum case 'box' has one associated value that is a tuple of 2 elements}}{{18-18=(}}{{29-29=)}}
+    // expected-note@-42 {{'box' declared here}}
     }
   }
 

test/decl/enum/enumtest.swift

@@ -277,7 +277,8 @@ func testDirection() {
     i = x
     break
 
-  case .NorthEast(let x): // expected-warning {{cannot match several associated values at once, implicitly tupling the associated values and trying to match that instead}}
+  case .NorthEast(let x): // expected-warning {{enum case 'NorthEast' has 2 associated values; matching them as a tuple is deprecated}}
+                          // expected-note@-14 {{'NorthEast(distanceNorth:distanceEast:)' declared here}}
     i = x.distanceEast
     break
   }