Merge pull request swiftlang#21452 from brentdax/to-error-is-human-to-forgive-domain

Change error domains on @objc enums to ensure they're unique

Author: beccadax

include/swift/AST/AccessScope.h

@@ -47,6 +47,7 @@ class AccessScope {
   bool isPublic() const { return !Value.getPointer(); }
   bool isPrivate() const { return Value.getInt(); }
   bool isFileScope() const;
+  bool isInternal() const;
 
   /// Returns true if this is a child scope of the specified other access scope.
   ///

include/swift/AST/SwiftNameTranslation.h

@@ -18,6 +18,7 @@
 
 namespace swift {
   class ValueDecl;
+  class EnumDecl;
   class EnumElementDecl;
 
 namespace objc_translation {
@@ -29,6 +30,8 @@ namespace objc_translation {
   StringRef getNameForObjC(const ValueDecl *VD,
                            CustomNamesOnly_t customNamesOnly = Normal);
 
+  std::string getErrorDomainStringForObjC(const EnumDecl *ED);
+
   /// Print the ObjC name of an enum element decl to OS, also allowing the client
   /// to specify a preferred name other than the decl's original name.
   ///

lib/AST/DeclContext.cpp

@@ -839,6 +839,11 @@ bool AccessScope::isFileScope() const {
   return DC && isa<FileUnit>(DC);
 }
 
+bool AccessScope::isInternal() const {
+  auto DC = getDeclContext();
+  return DC && isa<ModuleDecl>(DC);
+}
+
 AccessLevel AccessScope::accessLevelForDiagnostics() const {
   if (isPublic())
     return AccessLevel::Public;

lib/AST/SwiftNameTranslation.cpp

@@ -16,6 +16,7 @@
 
 #include "swift/AST/SwiftNameTranslation.h"
 #include "swift/AST/ASTContext.h"
+#include "swift/AST/Module.h"
 #include "swift/AST/Decl.h"
 #include "swift/AST/LazyResolver.h"
 #include "swift/Basic/StringExtras.h"
@@ -51,6 +52,34 @@ getNameForObjC(const ValueDecl *VD, CustomNamesOnly_t customNamesOnly) {
   return VD->getBaseName().getIdentifier().str();
 }
 
+std::string swift::objc_translation::
+getErrorDomainStringForObjC(const EnumDecl *ED) {
+  // Should have already been diagnosed as diag::objc_enum_generic.
+  assert(!ED->isGenericContext() && "Trying to bridge generic enum error to Obj-C");
+
+  // Clang decls have custom domains, but we shouldn't see them here anyway.
+  assert(!ED->getClangDecl() && "clang decls shouldn't be re-exported");
+
+  SmallVector<const NominalTypeDecl *, 4> outerTypes;
+  for (const NominalTypeDecl * D = ED;
+       D != nullptr;
+       D = D->getDeclContext()->getSelfNominalTypeDecl()) {
+    // We don't currently PrintAsObjC any types whose parents are private or
+    // fileprivate.
+    assert(D->getFormalAccess() >= AccessLevel::Internal &&
+            "We don't currently append private discriminators");
+    outerTypes.push_back(D);
+  }
+
+  std::string buffer = ED->getParentModule()->getNameStr();
+  for (auto D : reversed(outerTypes)) {
+    buffer += ".";
+    buffer += D->getNameStr();
+  }
+
+  return buffer;
+}
+
 bool swift::objc_translation::
 printSwiftEnumElemNameInObjC(const EnumElementDecl *EL, llvm::raw_ostream &OS,
                              Identifier PreferredName) {

lib/PrintAsObjC/PrintAsObjC.cpp

@@ -2511,7 +2511,7 @@ class ModuleWriter {
       });
       if (!hasDomainCase) {
         os << "static NSString * _Nonnull const " << getNameForObjC(ED)
-           << "Domain = @\"" << M.getName() << "." << ED->getName() << "\";\n";
+           << "Domain = @\"" << getErrorDomainStringForObjC(ED) << "\";\n";
       }
     }
 

lib/Sema/DerivedConformanceError.cpp

@@ -22,11 +22,40 @@
 #include "swift/AST/Expr.h"
 #include "swift/AST/Module.h"
 #include "swift/AST/Types.h"
+#include "swift/AST/SwiftNameTranslation.h"
 
 using namespace swift;
+using namespace swift::objc_translation;
 
 static void deriveBodyBridgedNSError_enum_nsErrorDomain(
-              AbstractFunctionDecl *domainDecl, void *) {
+                    AbstractFunctionDecl *domainDecl, void *) {
+  // enum SomeEnum {
+  //   @derived
+  //   static var _nsErrorDomain: String {
+  //     return String(reflecting: self)
+  //   }
+  // }
+
+  auto M = domainDecl->getParentModule();
+  auto &C = M->getASTContext();
+  auto self = domainDecl->getImplicitSelfDecl();
+
+  auto selfRef = new (C) DeclRefExpr(self, DeclNameLoc(), /*implicit*/ true);
+  auto stringType = TypeExpr::createForDecl(SourceLoc(), C.getStringDecl(),
+                                            domainDecl, /*implicit*/ true);
+  auto initReflectingCall =
+    CallExpr::createImplicit(C, stringType,
+                             { selfRef }, { C.getIdentifier("reflecting") });
+  auto ret =
+    new (C) ReturnStmt(SourceLoc(), initReflectingCall, /*implicit*/ true);
+
+  auto body = BraceStmt::create(C, SourceLoc(), ASTNode(ret), SourceLoc());
+
+  domainDecl->setBody(body);
+}
+
+static void deriveBodyBridgedNSError_printAsObjCEnum_nsErrorDomain(
+                    AbstractFunctionDecl *domainDecl, void *) {
   // enum SomeEnum {
   //   @derived
   //   static var _nsErrorDomain: String {
@@ -39,10 +68,7 @@ static void deriveBodyBridgedNSError_enum_nsErrorDomain(
   auto TC = domainDecl->getInnermostTypeContext();
   auto ED = TC->getSelfEnumDecl();
 
-  std::string buffer = M->getNameStr();
-  buffer += ".";
-  buffer += ED->getNameStr();
-  StringRef value(C.AllocateCopy(buffer));
+  StringRef value(C.AllocateCopy(getErrorDomainStringForObjC(ED)));
 
   auto string = new (C) StringLiteralExpr(value, SourceRange(), /*implicit*/ true);
   auto ret = new (C) ReturnStmt(SourceLoc(), string, /*implicit*/ true);
@@ -53,17 +79,15 @@ static void deriveBodyBridgedNSError_enum_nsErrorDomain(
 }
 
 static ValueDecl *
-deriveBridgedNSError_enum_nsErrorDomain(DerivedConformance &derived) {
+deriveBridgedNSError_enum_nsErrorDomain(DerivedConformance &derived,
+                        void (*synthesizer)(AbstractFunctionDecl *, void*)) {
   // enum SomeEnum {
   //   @derived
   //   static var _nsErrorDomain: String {
-  //     return "\(self)"
+  //     ...
   //   }
   // }
 
-  // Note that for @objc enums the format is assumed to be "MyModule.SomeEnum".
-  // If this changes, please change PrintAsObjC as well.
-
   ASTContext &C = derived.TC.Context;
 
   auto stringTy = C.getStringDecl()->getDeclaredType();
@@ -78,7 +102,7 @@ deriveBridgedNSError_enum_nsErrorDomain(DerivedConformance &derived) {
   // Define the getter.
   auto getterDecl = derived.addGetterToReadOnlyDerivedProperty(
       derived.TC, propDecl, stringTy);
-  getterDecl->setBodySynthesizer(&deriveBodyBridgedNSError_enum_nsErrorDomain);
+  getterDecl->setBodySynthesizer(synthesizer);
 
   derived.addMembersToConformanceContext({getterDecl, propDecl, pbDecl});
 
@@ -89,8 +113,17 @@ ValueDecl *DerivedConformance::deriveBridgedNSError(ValueDecl *requirement) {
   if (!isa<EnumDecl>(Nominal))
     return nullptr;
 
-  if (requirement->getBaseName() == TC.Context.Id_nsErrorDomain)
-    return deriveBridgedNSError_enum_nsErrorDomain(*this);
+  if (requirement->getBaseName() == TC.Context.Id_nsErrorDomain) {
+    auto synthesizer = deriveBodyBridgedNSError_enum_nsErrorDomain;
+
+    auto scope = Nominal->getFormalAccessScope(Nominal->getModuleScopeContext());
+    if (scope.isPublic() || scope.isInternal())
+      // PrintAsObjC may print this domain, so we should make sure we use the
+      // same string it will.
+      synthesizer = deriveBodyBridgedNSError_printAsObjCEnum_nsErrorDomain;
+
+    return deriveBridgedNSError_enum_nsErrorDomain(*this, synthesizer);
+  }
 
   TC.diagnose(requirement->getLoc(), diag::broken_errortype_requirement);
   return nullptr;

test/stdlib/ErrorBridged.swift

@@ -722,4 +722,49 @@ ErrorBridgingTests.test("Error archetype identity") {
     === nsError)
 }
 
+// SR-9389
+class ParentA: NSObject {
+  @objc(ParentAError) enum Error: Int, Swift.Error {
+    case failed
+  }
+}
+class ParentB: NSObject {
+  @objc(ParentBError) enum Error: Int, Swift.Error {
+    case failed
+  }
+}
+private class NonPrintAsObjCClass: NSObject {
+  @objc enum Error: Int, Swift.Error {
+    case foo
+  }
+}
+@objc private enum NonPrintAsObjCError: Int, Error {
+  case bar
+}
+
+ErrorBridgingTests.test("@objc error domains for nested types") {
+  // Domain strings should correspond to Swift types, including parent types.
+  expectEqual(ParentA.Error.failed._domain, "main.ParentA.Error")
+  expectEqual(ParentB.Error.failed._domain, "main.ParentB.Error")
+
+  func makeNSError(like error: Error) -> NSError {
+    return NSError(domain: error._domain, code: error._code)
+  }
+
+  // NSErrors corresponding to Error types with the same name but nested in
+  // different enclosing types should not be castable to the wrong error type.
+  expectTrue(makeNSError(like: ParentA.Error.failed) is ParentA.Error)
+  expectFalse(makeNSError(like: ParentA.Error.failed) is ParentB.Error)
+  expectFalse(makeNSError(like: ParentB.Error.failed) is ParentA.Error)
+  expectTrue(makeNSError(like: ParentB.Error.failed) is ParentB.Error)
+
+  // If an @objc enum error is not eligible for PrintAsObjC, we should treat it
+  // as though it inherited the default implementation, which calls
+  // String(reflecting:).
+  expectEqual(NonPrintAsObjCClass.Error.foo._domain,
+              String(reflecting: NonPrintAsObjCClass.Error.self))
+  expectEqual(NonPrintAsObjCError.bar._domain,
+              String(reflecting: NonPrintAsObjCError.self))
+}
+
 runAllTests()