improve diagnosis of bad SuppressedEntries

Author: kavon

include/swift/AST/DiagnosticsSema.def

@@ -6872,6 +6872,14 @@ ERROR(suppress_cannot_suppress,none,
 ERROR(suppress_on_wrong_decl,none,
       "cannot suppress conformance on %0", (DescriptiveDeclKind))
 
+ERROR(suppress_duplicate,none,
+      "implicit conformance to %0 already suppressed", (Type))
+NOTE(suppress_previously_here,none,
+      "%0 previously suppressed here", (Type))
+
+ERROR(suppress_not_protocol,none,
+      "suppression of non-protocol type %0", (Type))
+
 //------------------------------------------------------------------------------
 // MARK: Type inference from default expressions
 //------------------------------------------------------------------------------

lib/Sema/TypeCheckDeclPrimary.cpp

@@ -65,6 +65,7 @@
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/DJB.h"
+#include <unordered_map>
 
 using namespace swift;
 
@@ -96,6 +97,13 @@ static void checkSuppressedEntries(
   ASTContext &ctx = typeDecl->getASTContext();
   auto &diags = ctx.Diags;
 
+  using EntryTracker = std::unordered_map<KnownProtocolKind,
+                                          SuppressedEntry const *>;
+  EntryTracker seenEntries;
+
+  bool haveMoveOnlyClasses =
+      typeDecl->getASTContext().LangOpts.hasFeature(Feature::MoveOnlyClasses);
+
   ArrayRef<SuppressedEntry> suppressedEntries = typeDecl->getSuppressed();
   for (unsigned i = 0, n = suppressedEntries.size(); i != n; ++i) {
     auto &suppressed = suppressedEntries[i];
@@ -108,21 +116,63 @@ static void checkSuppressedEntries(
     if (!suppressedTy || suppressedTy->hasError())
       continue;
 
-    // Skip over suppressible protocols.
+    // only structs, enums, and some classes can have suppressed entries
+    if (!(isa<StructDecl>(typeDecl) || isa<EnumDecl>(typeDecl)
+        || (isa<ClassDecl>(typeDecl) && haveMoveOnlyClasses))) {
+      diags.diagnose(suppressed.getLoc(),
+                     diag::suppress_on_wrong_decl,
+                     typeDecl->getDescriptiveKind());
+      // FIXME: a great fix-it would be to remove the entire entry,
+      //  including any trailing comma. See checkInheritedEntry
+      //  for an example of the logic to do that. You'll need to update it
+      //  using MergedArrayRef so it understands source locations for both
+      //  suppressed and inherited entries.
+      continue;
+    }
+
+    // Ensure it is a suppressible protocol.
     if (auto protocolTy = dyn_cast<ProtocolType>(suppressedTy))
       if (auto protocolDecl = protocolTy->getDecl())
         if (auto knownProtocol = protocolDecl->getKnownProtocolKind())
-          if (isSuppressibleProtocol(knownProtocol.getValue()))
-            continue;
+          if (isSuppressibleProtocol(*knownProtocol)) {
+            // remember that we've seen this suppression for the type
+            EntryTracker::iterator iter;
+            bool uniqueEntry;
+            std::tie(iter, uniqueEntry) =
+                seenEntries.insert({*knownProtocol, &suppressed});
+
+            // check if already encountered an entry suppressing this protocol
+            if (!uniqueEntry) {
+              assert(suppressedTy->getCanonicalType()
+                         == iter->second->getType()->getCanonicalType());
+
+              // Emit a diagnostic about the duplicate suppression.
+              diags.diagnose(suppressed.getLoc(),
+                             diag::suppress_duplicate,
+                             suppressedTy);
+              // FIXME: a great fix-it would be to remove the entire entry.
+
+              diags.diagnose(iter->second->getLoc(),
+                             diag::suppress_previously_here,
+                             suppressedTy);
+            }
 
-    diags.diagnose(suppressed.getLoc(), diag::suppress_cannot_suppress, suppressedTy);
-    // TODO: fixit to remove both suppressed TypeLoc up to the next item in
-    // the inheritance clause list, including a trailing comma, etc.
+            continue;
+          }
 
-    // TODO: find duplicate suppressions.
+    // At this point, the suppressed type is wrong. Tailor the error messages.
 
-    // TODO: update checkInheritanceClause using MergedArrayRef so it can
-    // properly locate and understand all of the clauses in its fix-its.
+    if (suppressedTy->isConstraintType()) {
+      diags.diagnose(suppressed.getLoc(),
+                     diag::suppress_cannot_suppress,
+                     suppressedTy);
+      // FIXME: a great fix-it would be to remove the entire entry.
+    } else {
+      diags.diagnose(suppressed.getLoc(),
+                     diag::suppress_not_protocol,
+                     suppressedTy);
+      // FIXME: a great fix-it would be to remove the entire entry.
+    }
   }
 }
 

test/Sema/without_implicit_conformance.swift

@@ -0,0 +1,44 @@
+// RUN: %target-typecheck-verify-swift
+
+protocol UserDefinedProtocol {}
+
+class Who: ~SomebodyNew {} // expected-error {{cannot find type 'SomebodyNew' in scope}}
+
+class Blah: ~UserDefinedProtocol {} // expected-error {{cannot suppress conformance on class}}
+
+protocol Brotocol<T>: ~Copyable { // expected-error {{cannot suppress conformances here}}
+  associatedtype T: ~Copyable // expected-error {{cannot suppress conformances here}}
+}
+
+func inspectGPTCoin<T: ~Copyable>(_ t: T) {}
+// expected-error@-1 {{expected a class type or protocol-constrained type restricting 'T'}}
+
+// FIXME: not a pleasant set of errors!
+func piano<T>(_ t: T) where T: ~Copyable {}
+// expected-error@-1 {{expected '{' in body of function declaration}}
+// expected-error@-2 {{consecutive statements on a line must be separated by ';'}}
+// expected-error@-3 {{expected type}}
+// expected-error@-4 {{'any Copyable' cannot be constructed because it has no accessible initializers}}
+
+struct DoubleNotCopyable: ~Copyable, // expected-note {{'Copyable' previously suppressed here}}
+                          Sendable,
+                          ~Copyable { } // expected-error {{implicit conformance to 'Copyable' already suppressed}}
+
+extension DoubleNotCopyable: ~Copyable {} // expected-error {{cannot suppress conformances here}}
+
+enum Colors: ~Equatable, ~Hashable {
+// expected-error@-1 {{cannot suppress implicit conformance to 'Equatable'}}
+// expected-error@-2 {{cannot suppress implicit conformance to 'Hashable'}}
+  case red
+  case white
+  case blue
+}
+
+// FIXME: this is confusing! We need to require parens!!
+struct Composition: ~Copyable & Equatable {} // expected-error {{cannot suppress implicit conformance to 'Copyable & Equatable'}}
+
+enum Whatever<T> : ~Copyable
+                  where T: ~Copyable {} // expected-error {{expected type}}
+                                        // expected-error@-1 {{expected '{' in enum}}
+
+struct SuppressANonProtocolType: ~Colors {} // expected-error {{suppression of non-protocol type 'Colors'}}