[Omit needless words] Ensure Swift name consistency for overriding/overridden decalrations.

DougGregor · DougGregor · commit ca736fd27ca7 · 2016-02-17T23:35:41.000-08:00
When we transform an Objective-C method or property name, we take into
account the parameter types, result type, and type of the enclosing
context. Doing so means that one can get different Swift names for a
particular method (or property) and one that overrides it, which leads
to dangerous inconsistencies in the Swift names. Address this
limitation by using the original declaration's name (i.e., one that
does not override any other).

It is possible that there is more than one "original declaration",
when the same method/property name comes from different
protocols. When this is the case, check for consistency and complain
if there are inconsistencies.

Amusingly, this changes exactly one method in Cocoa
(UINavigationController's showViewController:sender:), with no conflicts
detected. Fixes rdar://problem/24558337.
diff --git a/include/swift/AST/DiagnosticsClangImporter.def b/include/swift/AST/DiagnosticsClangImporter.def
@@ -58,6 +58,12 @@ WARNING(invalid_swift_name_method,none,
   "too %select{few|many}0 parameters in swift_name attribute (expected %1; "
   "got %2)", (bool, unsigned, unsigned))
 
+WARNING(inconsistent_swift_name,none,
+        "inconsistent Swift name for Objective-C %select{method|property}0 "
+        "'%1' in '%2' (%3 in '%4' vs. %5 in '%6')",
+        (bool, StringRef, StringRef, DeclName, StringRef, DeclName,
+         StringRef))
+
 #ifndef DIAG_NO_UNDEF
 # if defined(DIAG)
 #  undef DIAG
diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp
@@ -1903,6 +1903,95 @@ Identifier ClangImporter::Implementation::importMacroName(
   return SwiftContext.getIdentifier(name);
 }
 
+namespace {
+  typedef ClangImporter::Implementation::ImportedName ImportedName;
+}
+
+namespace llvm {
+  // An Identifier is "pointer like".
+  template<typename T> class PointerLikeTypeTraits;
+  template<>
+  class PointerLikeTypeTraits<swift::DeclName> {
+  public:
+    static inline void *getAsVoidPointer(swift::DeclName name) {
+      return name.getOpaqueValue();
+    }
+    static inline swift::DeclName getFromVoidPointer(void *ptr) {
+      return swift::DeclName::getFromOpaqueValue(ptr);
+    }
+    enum { NumLowBitsAvailable = 0 };
+  };
+}
+
+/// Retrieve the name of the given Clang declaration context for
+/// printing.
+static StringRef getClangDeclContextName(const clang::DeclContext *dc) {
+  auto type = ClangImporter::Implementation::getClangDeclContextType(dc);
+
+  // FIXME: Hack to work around getClangDeclContextType() failing for
+  // protocols.
+  if (type.isNull()) {
+    if (auto constProto = dyn_cast<clang::ObjCProtocolDecl>(dc)) {
+      auto proto = const_cast<clang::ObjCProtocolDecl *>(constProto);
+      clang::ASTContext &ctx = dc->getParentASTContext();
+      type = ctx.getObjCObjectType(ctx.ObjCBuiltinIdTy, { }, { proto }, false);
+      type = ctx.getObjCObjectPointerType(type);
+    }
+  }
+
+  if (type.isNull()) return StringRef();
+
+  return ClangImporter::Implementation::getClangTypeNameForOmission(
+           dc->getParentASTContext(),
+           type).Name;
+}
+
+namespace {
+  /// Merge the a set of imported names produced for the overridden
+  /// declarations of a given method or property.
+  template<typename DeclType>
+  void mergeOverriddenNames(ASTContext &ctx,
+                            const DeclType *decl,
+                            SmallVectorImpl<std::pair<const DeclType *,
+                                                      ImportedName>>
+                              &overriddenNames) {
+    typedef std::pair<const DeclType *, ImportedName> OverriddenName;
+    llvm::SmallPtrSet<DeclName, 4> known;
+    (void)known.insert(DeclName());
+    overriddenNames.erase(std::remove_if(overriddenNames.begin(),
+                                         overriddenNames.end(),
+                                         [&](OverriddenName overridden) {
+                                           return !known.insert(
+                                                     overridden.second.Imported)
+                                             .second;
+                                         }),
+                          overriddenNames.end());
+
+    if (overriddenNames.size() < 2)
+      return;
+
+    // Complain about inconsistencies.
+    std::string nameStr;
+    auto method = dyn_cast<clang::ObjCMethodDecl>(decl);
+    if (method)
+      nameStr = method->getSelector().getAsString();
+    else
+      nameStr = cast<clang::ObjCPropertyDecl>(decl)->getName().str();
+    for (unsigned i = 1, n = overriddenNames.size(); i != n; ++i) {
+      ctx.Diags.diagnose(SourceLoc(), diag::inconsistent_swift_name,
+                         method == nullptr,
+                         nameStr,
+                         getClangDeclContextName(decl->getDeclContext()),
+                         overriddenNames[0].second,
+                         getClangDeclContextName(
+                           overriddenNames[0].first->getDeclContext()),
+                         overriddenNames[i].second,
+                         getClangDeclContextName(
+                           overriddenNames[i].first->getDeclContext()));
+    }
+  }
+}
+
 auto ClangImporter::Implementation::importFullName(
        const clang::NamedDecl *D,
        ImportNameOptions options,
@@ -1951,6 +2040,64 @@ auto ClangImporter::Implementation::importFullName(
     }
   }
 
+  // When omitting needless words, find the original method/property
+  // declaration.
+  if (OmitNeedlessWords) {
+    if (auto method = dyn_cast<clang::ObjCMethodDecl>(D)) {
+      // Inherit the name from the "originating" declarations, if
+      // there are any.
+      SmallVector<std::pair<const clang::ObjCMethodDecl *, ImportedName>, 4>
+        overriddenNames;
+      SmallVector<const clang::ObjCMethodDecl *, 4> overriddenMethods;
+      method->getOverriddenMethods(overriddenMethods);
+      for (auto overridden : overriddenMethods) {
+        const auto overriddenName = importFullName(overridden, options,
+                                                   nullptr,clangSemaOverride);
+        if (overriddenName.Imported)
+          overriddenNames.push_back({overridden, overriddenName});
+      }
+
+      // If we found any names of overridden methods, return those names.
+      if (!overriddenNames.empty()) {
+        if (overriddenNames.size() > 1)
+          mergeOverriddenNames(SwiftContext, method, overriddenNames);
+        return overriddenNames[0].second;
+      }
+    } else if (auto property = dyn_cast<clang::ObjCPropertyDecl>(D)) {
+      // Inherit the name from the "originating" declarations, if
+      // there are any.
+      if (auto getter = property->getGetterMethodDecl()) {
+        SmallVector<std::pair<const clang::ObjCPropertyDecl *, ImportedName>, 4>
+          overriddenNames;
+        SmallVector<const clang::ObjCMethodDecl *, 4> overriddenMethods;
+        SmallPtrSet<const clang::ObjCPropertyDecl *, 4> knownProperties;
+        (void)knownProperties.insert(property);
+
+        getter->getOverriddenMethods(overriddenMethods);
+        for (auto overridden : overriddenMethods) {
+          if (!overridden->isPropertyAccessor()) continue;
+          auto overriddenProperty = overridden->findPropertyDecl(true);
+          if (!overriddenProperty) continue;
+          if (!knownProperties.insert(overriddenProperty).second) continue;
+
+          const auto overriddenName = importFullName(overriddenProperty,
+                                                     options,
+                                                     nullptr,
+                                                     clangSemaOverride);
+          if (overriddenName.Imported)
+            overriddenNames.push_back({overriddenProperty, overriddenName});
+        }
+
+        // If we found any names of overridden methods, return those names.
+        if (!overriddenNames.empty()) {
+          if (overriddenNames.size() > 1)
+            mergeOverriddenNames(SwiftContext, property, overriddenNames);
+          return overriddenNames[0].second;
+        }
+      }
+    }
+  }
+
   // If we have a swift_name attribute, use that.
   if (auto *nameAttr = D->getAttr<clang::SwiftNameAttr>()) {
     bool skipCustomName = false;
diff --git a/lib/ClangImporter/ImportType.cpp b/lib/ClangImporter/ImportType.cpp
@@ -1857,7 +1857,7 @@ bool ClangImporter::Implementation::omitNeedlessWordsInFunctionName(
 /// Retrieve the instance type of the given Clang declaration context.
 clang::QualType ClangImporter::Implementation::getClangDeclContextType(
                   const clang::DeclContext *dc) {
-  auto &ctx = getClangASTContext();
+  auto &ctx = dc->getParentASTContext();
   if (auto objcClass = dyn_cast<clang::ObjCInterfaceDecl>(dc))
     return ctx.getObjCObjectPointerType(ctx.getObjCInterfaceType(objcClass));
 
diff --git a/lib/ClangImporter/ImporterImpl.h b/lib/ClangImporter/ImporterImpl.h
@@ -796,7 +796,7 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
 
   /// Retrieve the type of an instance of the given Clang declaration context,
   /// or a null type if the DeclContext does not have a corresponding type.
-  clang::QualType getClangDeclContextType(const clang::DeclContext *dc);
+  static clang::QualType getClangDeclContextType(const clang::DeclContext *dc);
 
   /// Determine whether this typedef is a CF type.
   static bool isCFTypeDecl(const clang::TypedefNameDecl *Decl);
diff --git a/test/IDE/Inputs/custom-modules/OmitNeedlessWords.h b/test/IDE/Inputs/custom-modules/OmitNeedlessWords.h
@@ -1,5 +1,9 @@
 @import Foundation;
 
+#ifndef NS_SWIFT_NAME(Name)
+#  define NS_SWIFT_NAME(Name) __attribute__((swift_name(#Name)))
+#endif
+
 @interface SEGreebieArray : NSObject
 @end
 
@@ -27,3 +31,26 @@
 -(void)drawFilledPolygonWithPoints:(NSPointArray)points count:(NSInteger)count;
 -(void)drawGreebies:(nonnull SEGreebieArray*)greebies;
 @end
+
+@protocol OMWWiggle
+-(void)joinSub;
+-(void)conflicting1 NS_SWIFT_NAME(wiggle1());
+@property (readonly) NSInteger conflictingProp1 NS_SWIFT_NAME(wiggleProp1);
+@end
+
+@protocol OMWWaggle
+-(void)conflicting1 NS_SWIFT_NAME(waggle1());
+@property (readonly) NSInteger conflictingProp1 NS_SWIFT_NAME(waggleProp1);
+@end
+
+@interface OMWSuper : NSObject <OMWWiggle>
+-(void)jumpSuper;
+@property (readonly) NSInteger conflictingProp1;
+@end
+
+@interface OMWSub : OMWSuper <OMWWaggle>
+-(void)jumpSuper;
+-(void)joinSub;
+-(void)conflicting1;
+@property (readonly) NSInteger conflictingProp1;
+@end
diff --git a/test/IDE/print_omit_needless_words.swift b/test/IDE/print_omit_needless_words.swift
@@ -21,8 +21,10 @@
 // RUN: %target-swift-ide-test(mock-sdk: -sdk %S/../Inputs/clang-importer-sdk -I %t -I %S/../ClangModules/Inputs/custom-modules) -print-module -source-filename %s -module-to-print=CoreCooling -function-definitions=false -prefer-type-repr=true -enable-omit-needless-words  -enable-strip-ns-prefix -skip-parameter-names -enable-infer-default-arguments > %t.CoreCooling.txt
 // RUN: FileCheck %s -check-prefix=CHECK-CORECOOLING -strict-whitespace < %t.CoreCooling.txt
 
-// RUN: %target-swift-ide-test(mock-sdk: -sdk %S/../Inputs/clang-importer-sdk -I %t -I %S/Inputs/custom-modules) -print-module -source-filename %s -module-to-print=OmitNeedlessWords -function-definitions=false -prefer-type-repr=true -enable-omit-needless-words  -enable-strip-ns-prefix -skip-parameter-names -enable-infer-default-arguments > %t.OmitNeedlessWords.txt
+// RUN: %target-swift-ide-test(mock-sdk: -sdk %S/../Inputs/clang-importer-sdk -I %t -I %S/Inputs/custom-modules) -print-module -source-filename %s -module-to-print=OmitNeedlessWords -function-definitions=false -prefer-type-repr=true -enable-omit-needless-words  -enable-strip-ns-prefix -skip-parameter-names -enable-infer-default-arguments > %t.OmitNeedlessWords.txt 2> %t.OmitNeedlessWords.diagnostics.txt
 // RUN: FileCheck %s -check-prefix=CHECK-OMIT-NEEDLESS-WORDS -strict-whitespace < %t.OmitNeedlessWords.txt
+// RUN: FileCheck %s -check-prefix=CHECK-OMIT-NEEDLESS-WORDS-DIAGS -strict-whitespace < %t.OmitNeedlessWords.diagnostics.txt
+
 
 // RUN: %target-swift-ide-test(mock-sdk: -sdk %S/../Inputs/clang-importer-sdk -I %t) -print-module -source-filename %s -module-to-print=errors -function-definitions=false -prefer-type-repr=true -enable-omit-needless-words  -enable-strip-ns-prefix -skip-parameter-names -enable-infer-default-arguments > %t.errors.txt
 // RUN: FileCheck %s -check-prefix=CHECK-ERRORS -strict-whitespace < %t.errors.txt
@@ -307,5 +309,26 @@
 // Non-parameterized Objective-C class ending in "Array".
 // CHECK-OMIT-NEEDLESS-WORDS: func draw(_: SEGreebieArray)
 
+// Verify that we get the Swift name from the original declaration.
+// CHECK-OMIT-NEEDLESS-WORDS: protocol OMWWiggle
+// CHECK-OMIT-NEEDLESS-WORDS-NEXT: func joinSub()
+// CHECK-OMIT-NEEDLESS-WORDS-NEXT: func wiggle1()
+// CHECK-OMIT-NEEDLESS-WORDS-NEXT: var wiggleProp1: Int { get }
+
+// CHECK-OMIT-NEEDLESS-WORDS: protocol OMWWaggle
+// CHECK-OMIT-NEEDLESS-WORDS-NEXT: func waggle1()
+// CHECK-OMIT-NEEDLESS-WORDS-NEXT: var waggleProp1: Int { get }
+
+// CHECK-OMIT-NEEDLESS-WORDS: class OMWSuper
+// CHECK-OMIT-NEEDLESS-WORDS-NEXT: func jump()
+// CHECK-OMIT-NEEDLESS-WORDS-NEXT: var wiggleProp1: Int { get }
+
+// CHECK-OMIT-NEEDLESS-WORDS: class OMWSub
+// CHECK-OMIT-NEEDLESS-WORDS-NEXT: func jump()
+// CHECK-OMIT-NEEDLESS-WORDS-NEXT: func joinSub()
+
+// CHECK-OMIT-NEEDLESS-WORDS-DIAGS: inconsistent Swift name for Objective-C method 'conflicting1' in 'OMWSub' ('waggle1()' in 'OMWWaggle' vs. 'wiggle1()' in 'OMWWiggle')
+// CHECK-OMIT-NEEDLESS-WORDS-DIAGS: inconsistent Swift name for Objective-C property 'conflictingProp1' in 'OMWSub' ('waggleProp1' in 'OMWWaggle' vs. 'wiggleProp1' in 'OMWSuper')
+
 // Don't drop the 'error'.
 // CHECK-ERRORS: func tryAndReturnError(_: ()) throws
