Serialize extended nominal separately when serializing an extension.

varungandhi-apple · varungandhi-apple · commit 0dcb93653170 · 2019-09-05T15:33:02.000-07:00
Instead of computing it from the extended type after deserialization --
which is tricky to do, due to potential presence of protocol
compositions -- we obtain the extended nominal directly.

Fixes SR-11227 and linked rdar://problem/53712389.
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -1601,6 +1601,11 @@ ERROR(inferred_opaque_type,none,
 // Extensions
 ERROR(non_nominal_extension,none,
       "non-nominal type %0 cannot be extended", (Type))
+WARNING(composition_in_extended_type,none,
+        "extending a protocol composition is not supported; extending %0 "
+        "instead", (Type))
+NOTE(composition_in_extended_type_alternative,none,
+     "did you mean to extend the most specific type %0 instead?", (Type))
 ERROR(extension_access_with_conformances,none,
       "%0 modifier cannot be used with extensions that declare "
       "protocol conformances", (DeclAttribute))
diff --git a/lib/AST/NameLookup.cpp b/lib/AST/NameLookup.cpp
@@ -2142,7 +2142,10 @@ ExtendedNominalRequest::evaluate(Evaluator &evaluator,
   auto nominalTypes
     = resolveTypeDeclsToNominal(evaluator, ctx, referenced, modulesFound,
                                 anyObject);
-  return nominalTypes.empty() ? nullptr : nominalTypes.front();
+
+  // If there is more than 1 element, we will emit a warning or an error
+  // elsewhere, so don't handle that case here.
+  return nominalTypes.empty() ? nullptr : nominalTypes[0];
 }
 
 llvm::Expected<NominalTypeDecl *>
diff --git a/lib/Sema/TypeCheckDecl.cpp b/lib/Sema/TypeCheckDecl.cpp
@@ -3083,6 +3083,41 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
 
     TC.validateExtension(ED);
 
+    extType = ED->getExtendedType();
+    if (extType && !extType->hasError()) {
+      // The first condition catches syntactic forms like
+      //     protocol A & B { ... } // may be protocols or typealiases
+      // The second condition also looks through typealiases and catches
+      //    typealias T = P1 & P2 // P2 is a refined protocol of P1
+      //    extension T { ... }
+      // However, it is trickier to catch cases like
+      //    typealias T = P2 & P1 // P2 is a refined protocol of P1
+      //    extension T { ... }
+      // so we don't do that here.
+      auto extTypeRepr = ED->getExtendedTypeRepr();
+      auto *extTypeNominal = extType->getAnyNominal();
+      bool firstNominalIsNotMostSpecific =
+        extTypeNominal && extTypeNominal != nominal;
+      if (isa<CompositionTypeRepr>(extTypeRepr)
+          || firstNominalIsNotMostSpecific) {
+        auto firstNominalType = nominal->getDeclaredType();
+        auto diag = ED->diagnose(diag::composition_in_extended_type,
+                                 firstNominalType);
+        diag.highlight(extTypeRepr->getSourceRange());
+        if (firstNominalIsNotMostSpecific) {
+          diag.flush();
+          Type mostSpecificProtocol = extTypeNominal->getDeclaredType();
+          ED->diagnose(diag::composition_in_extended_type_alternative,
+                       mostSpecificProtocol)
+            .fixItReplace(extTypeRepr->getSourceRange(),
+                          mostSpecificProtocol->getString());
+        } else {
+          diag.fixItReplace(extTypeRepr->getSourceRange(),
+                            firstNominalType->getString());
+        }
+      }
+    }
+
     checkInheritanceClause(ED);
 
     // Check the raw values of an enum, since we might synthesize
diff --git a/lib/Serialization/Deserialization.cpp b/lib/Serialization/Deserialization.cpp
@@ -20,6 +20,7 @@
 #include "swift/AST/ForeignErrorConvention.h"
 #include "swift/AST/GenericEnvironment.h"
 #include "swift/AST/Initializer.h"
+#include "swift/AST/NameLookupRequests.h"
 #include "swift/AST/Pattern.h"
 #include "swift/AST/ParameterList.h"
 #include "swift/AST/PrettyStackTrace.h"
@@ -3689,13 +3690,15 @@ class swift::DeclDeserializer {
   Expected<Decl *> deserializeExtension(ArrayRef<uint64_t> scratch,
                                         StringRef blobData) {
     TypeID extendedTypeID;
+    DeclID extendedNominalID;
     DeclContextID contextID;
     bool isImplicit;
     GenericSignatureID genericEnvID;
     unsigned numConformances, numInherited;
     ArrayRef<uint64_t> inheritedAndDependencyIDs;
 
-    decls_block::ExtensionLayout::readRecord(scratch, extendedTypeID, contextID,
+    decls_block::ExtensionLayout::readRecord(scratch, extendedTypeID,
+                                             extendedNominalID, contextID,
                                              isImplicit, genericEnvID,
                                              numConformances, numInherited,
                                              inheritedAndDependencyIDs);
@@ -3735,7 +3738,9 @@ class swift::DeclDeserializer {
     auto extendedType = MF.getType(extendedTypeID);
     ctx.evaluator.cacheOutput(ExtendedTypeRequest{extension},
                               std::move(extendedType));
-    auto nominal = extension->getExtendedNominal();
+    auto nominal = dyn_cast<NominalTypeDecl>(MF.getDecl(extendedNominalID));
+    ctx.evaluator.cacheOutput(ExtendedNominalRequest{extension},
+                              std::move(nominal));
 
     if (isImplicit)
       extension->setImplicit();
diff --git a/lib/Serialization/ModuleFormat.h b/lib/Serialization/ModuleFormat.h
@@ -52,7 +52,7 @@ const uint16_t SWIFTMODULE_VERSION_MAJOR = 0;
 /// describe what change you made. The content of this comment isn't important;
 /// it just ensures a conflict if two people change the module format.
 /// Don't worry about adhering to the 80-column limit for this line.
-const uint16_t SWIFTMODULE_VERSION_MINOR = 517; // better string hash seed
+const uint16_t SWIFTMODULE_VERSION_MINOR = 518; // save extended nominal separately when serializing extensions
 
 /// A standard hash seed used for all string hashes in a serialized module.
 ///
@@ -1284,7 +1284,8 @@ namespace decls_block {
 
   using ExtensionLayout = BCRecordLayout<
     EXTENSION_DECL,
-    TypeIDField, // base type
+    TypeIDField, // extended type
+    DeclIDField, // extended nominal
     DeclContextIDField, // context decl
     BCFixed<1>,  // implicit flag
     GenericSignatureIDField,  // generic environment
diff --git a/lib/Serialization/Serialization.cpp b/lib/Serialization/Serialization.cpp
@@ -2654,11 +2654,6 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
     // simpler user model to just always desugar extension types.
     extendedType = extendedType->getCanonicalType();
 
-    // Make sure the base type has registered itself as a provider of generic
-    // parameters.
-    auto firstNominalInExtendedType = extendedType->getAnyNominal();
-    (void)S.addDeclRef(firstNominalInExtendedType);
-
     auto conformances = extension->getLocalConformances(
                           ConformanceLookupKind::All, nullptr);
 
@@ -2680,8 +2675,10 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
       inheritedAndDependencyTypes.push_back(S.addTypeRef(dependencyTy));
 
     unsigned abbrCode = S.DeclTypeAbbrCodes[ExtensionLayout::Code];
+    auto extendedNominal = extension->getExtendedNominal();
     ExtensionLayout::emitRecord(S.Out, S.ScratchRecord, abbrCode,
                                 S.addTypeRef(extendedType),
+                                S.addDeclRef(extendedNominal),
                                 contextID.getOpaqueValue(),
                                 extension->isImplicit(),
                                 S.addGenericEnvironmentRef(
@@ -2691,9 +2688,9 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
                                 inheritedAndDependencyTypes);
 
     bool isClassExtension = false;
-    if (firstNominalInExtendedType) {
-      isClassExtension = isa<ClassDecl>(firstNominalInExtendedType) ||
-                         isa<ProtocolDecl>(firstNominalInExtendedType);
+    if (extendedNominal) {
+      isClassExtension = isa<ClassDecl>(extendedNominal) ||
+                         isa<ProtocolDecl>(extendedNominal);
     }
 
     // Extensions of nested generic types have multiple generic parameter
diff --git a/test/Sema/Inputs/composition_extension_usage.swift b/test/Sema/Inputs/composition_extension_usage.swift
diff --git a/test/Sema/composition_extension.swift b/test/Sema/composition_extension.swift
@@ -0,0 +1,46 @@
+// RUN: %target-typecheck-verify-swift
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -primary-file %s %S/Inputs/composition_extension_usage.swift -emit-module-path %t/P-partial.swiftmodule -module-name SR11227 -enable-testing
+// RUN: %target-swift-frontend -primary-file %S/Inputs/composition_extension_usage.swift %s -emit-module-path %t/S-partial.swiftmodule -module-name SR11227 -enable-testing
+// RUN: %target-swift-frontend -sil-merge-partial-modules %t/P-partial.swiftmodule %t/S-partial.swiftmodule -emit-module -o %t/SR11227.swiftmodule
+
+protocol P1 {}
+
+protocol P1_1 : P1 {
+  func p1_1()
+}
+
+protocol P2 {}
+
+extension P1 & P1_1 where Self : P2 {
+  // expected-warning@-1 {{extending a protocol composition is not supported; extending 'P1' instead}}
+  // expected-note@-2 {{did you mean to extend the most specific type 'P1_1' instead?}} {{11-20=P1_1}}
+  func p1_1() {}
+}
+
+extension P1_1 & P1 where Self : P2 {
+  // expected-warning@-1 {{extending a protocol composition is not supported; extending 'P1_1' instead}} {{11-20=P1_1}}
+  func p1_1_alt() {}
+}
+
+typealias T1 = P1 & P1_1
+
+extension T1 {
+  // expected-warning@-1 {{extending a protocol composition is not supported; extending 'P1' instead}}
+  // expected-note@-2 {{did you mean to extend the most specific type 'P1_1' instead?}} {{11-13=P1_1}}
+  func t1() {}
+}
+
+typealias T2 = T1
+
+extension T2 {
+  // expected-warning@-1 {{extending a protocol composition is not supported; extending 'P1' instead}}
+  // expected-note@-2 {{did you mean to extend the most specific type 'P1_1' instead?}} {{11-13=P1_1}}
+  func t2() {}
+}
+
+typealias T3 = P1_1 & P1
+
+extension T3 { // Ideally, we should emit a warning here but the current implementation doesn't do that.
+    func t3() {}
+}
diff --git a/test/Sema/composition_extensions.swift b/test/Sema/composition_extensions.swift
diff --git a/test/decl/ext/protocol.swift b/test/decl/ext/protocol.swift
@@ -991,9 +991,7 @@ class BadClass5 : BadProto5 {} // expected-error{{type 'BadClass5' does not conf
 typealias A = BadProto1
 typealias B = BadProto1
 
-extension A & B { // okay
-
-}
+extension A & B {} // expected-warning {{extending a protocol composition is not supported; extending 'BadProto1' instead}}
 
 // Suppress near-miss warning for unlabeled initializers.
 protocol P9 {
