[ClangImporter] Find Swift 3 / 4 names via dynamic lookup too.

Finishes rdar://problem/29170671

Author: jrose-apple

lib/ClangImporter/ClangImporter.cpp

@@ -3041,37 +3041,30 @@ void ClangImporter::Implementation::lookupObjCMembers(
     // If the entry is not visible, skip it.
     if (!isVisibleClangEntry(clangCtx, clangDecl)) continue;
 
-    // Import the declaration.
-    auto decl =
-        cast_or_null<ValueDecl>(importDeclReal(clangDecl, CurrentVersion));
-    if (!decl)
-      continue;
-
-    // If the name we found matches, report the declaration.
-    bool matchedAny = false;
-    if (decl->getFullName().matchesRef(name)) {
-      consumer.foundDecl(decl, DeclVisibilityKind::DynamicLookup);
-      matchedAny = true;
-    }
+    forEachDistinctName(clangDecl,
+                        [&](ImportedName importedName,
+                            ImportNameVersion nameVersion) {
+      // Import the declaration.
+      auto decl =
+          cast_or_null<ValueDecl>(importDeclReal(clangDecl, nameVersion));
+      if (!decl)
+        return;
 
-    // Check for an alternate declaration; if it's name matches,
-    // report it.
-    for (auto alternate : getAlternateDecls(decl)) {
-      if (alternate->getFullName().matchesRef(name)) {
-        consumer.foundDecl(alternate, DeclVisibilityKind::DynamicLookup);
-        matchedAny = true;
+      // If the name we found matches, report the declaration.
+      // FIXME: If we didn't need to check alternate decls here, we could avoid
+      // importing the member at all by checking importedName ahead of time.
+      if (decl->getFullName().matchesRef(name)) {
+        consumer.foundDecl(decl, DeclVisibilityKind::DynamicLookup);
       }
-    }
 
-    // If we didn't find anything, try under the Swift 2 name.
-    if (!matchedAny) {
-      if (auto swift2Decl = cast_or_null<ValueDecl>(
-                              importDeclReal(clangDecl, Version::Swift2))) {
-        if (swift2Decl->getFullName().matchesRef(name)) {
-          consumer.foundDecl(swift2Decl, DeclVisibilityKind::DynamicLookup);
+      // Check for an alternate declaration; if its name matches,
+      // report it.
+      for (auto alternate : getAlternateDecls(decl)) {
+        if (alternate->getFullName().matchesRef(name)) {
+          consumer.foundDecl(alternate, DeclVisibilityKind::DynamicLookup);
         }
       }
-    }
+    });
   }
 }
 

lib/ClangImporter/ImportDecl.cpp

@@ -345,27 +345,15 @@ static bool isNSDictionaryMethod(const clang::ObjCMethodDecl *MD,
   return true;
 }
 
-/// Attempts to import the name of \p decl with each possible ImportNameVersion.
-/// \p action will be called with each unique name.
-///
-/// In this case, "unique" means either the full name is distinct or the
-/// effective context is distinct. This method does not attempt to handle
-/// "unresolved" contexts in any special way---if one name references a
-/// particular Clang declaration and the other has an unresolved context that
-/// will eventually reference that declaration, the contexts will still be
-/// considered distinct.
-///
-/// The names are generated in the same order as
-/// forEachImportNameVersionFromCurrent. The current name is always first.
-static void forEachDistinctName(
-    ClangImporter::Implementation &impl, const clang::NamedDecl *decl,
+void ClangImporter::Implementation::forEachDistinctName(
+    const clang::NamedDecl *decl,
     llvm::function_ref<void(ImportedName, ImportNameVersion)> action) {
   using ImportNameKey = std::pair<DeclName, EffectiveClangContext>;
   SmallVector<ImportNameKey, 8> seenNames;
-  forEachImportNameVersionFromCurrent(impl.CurrentVersion,
+  forEachImportNameVersionFromCurrent(CurrentVersion,
                                       [&](ImportNameVersion nameVersion) {
     // Check to see if the name is different.
-    ImportedName newName = impl.importFullName(decl, nameVersion);
+    ImportedName newName = importFullName(decl, nameVersion);
     ImportNameKey key(newName, newName.getEffectiveContext());
     bool seen = llvm::any_of(seenNames,
                              [&key](const ImportNameKey &existing) -> bool {
@@ -2669,9 +2657,9 @@ namespace {
         switch (enumKind) {
         case EnumKind::Constants:
         case EnumKind::Unknown:
-          forEachDistinctName(Impl, constant,
-                              [&](ImportedName newName,
-                                  ImportNameVersion nameVersion) {
+          Impl.forEachDistinctName(constant,
+                                   [&](ImportedName newName,
+                                       ImportNameVersion nameVersion) {
             Decl *imported = Impl.importDecl(constant, nameVersion);
             if (!imported)
               return;
@@ -2682,9 +2670,9 @@ namespace {
           });
           break;
         case EnumKind::Options:
-          forEachDistinctName(Impl, constant,
-                              [&](ImportedName newName,
-                                  ImportNameVersion nameVersion) {
+          Impl.forEachDistinctName(constant,
+                                   [&](ImportedName newName,
+                                       ImportNameVersion nameVersion) {
             if (!contextIsEnum(newName))
               return;
             SwiftDeclConverter converter(Impl, nameVersion);
@@ -2741,9 +2729,9 @@ namespace {
             }
           }
 
-          forEachDistinctName(Impl, constant,
-                              [&](ImportedName newName,
-                                  ImportNameVersion nameVersion) {
+          Impl.forEachDistinctName(constant,
+                                   [&](ImportedName newName,
+                                       ImportNameVersion nameVersion) {
             if (nameVersion == getActiveSwiftVersion())
               return;
             if (!contextIsEnum(newName))
@@ -7737,8 +7725,8 @@ ClangImporter::Implementation::loadAllMembers(Decl *D, uint64_t extra) {
       // Only continue members in the same submodule as this extension.
       if (decl->getImportedOwningModule() != submodule) continue;
 
-      forEachDistinctName(*this, decl, [&](ImportedName newName,
-                                           ImportNameVersion nameVersion) {
+      forEachDistinctName(decl, [&](ImportedName newName,
+                                    ImportNameVersion nameVersion) {
         // Quickly check the context and bail out if it obviously doesn't
         // belong here.
         if (auto *importDC = newName.getEffectiveContext().getAsDeclContext())
@@ -7806,7 +7794,7 @@ ClangImporter::Implementation::loadAllMembers(Decl *D, uint64_t extra) {
     if (!nd || nd != nd->getCanonicalDecl())
       continue;
 
-    forEachDistinctName(*this, nd,
+    forEachDistinctName(nd,
                         [&](ImportedName name, ImportNameVersion nameVersion) {
       auto member = importDecl(nd, nameVersion);
       if (!member)

lib/ClangImporter/ImporterImpl.h

@@ -1169,6 +1169,23 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
   /// Determine the effective Clang context for the given Swift nominal type.
   EffectiveClangContext getEffectiveClangContext(NominalTypeDecl *nominal);
 
+  /// Attempts to import the name of \p decl with each possible
+  /// ImportNameVersion. \p action will be called with each unique name.
+  ///
+  /// In this case, "unique" means either the full name is distinct or the
+  /// effective context is distinct. This method does not attempt to handle
+  /// "unresolved" contexts in any special way---if one name references a
+  /// particular Clang declaration and the other has an unresolved context that
+  /// will eventually reference that declaration, the contexts will still be
+  /// considered distinct.
+  ///
+  /// The names are generated in the same order as
+  /// forEachImportNameVersionFromCurrent. The current name is always first.
+  void forEachDistinctName(
+      const clang::NamedDecl *decl,
+      llvm::function_ref<void(importer::ImportedName,
+                              importer::ImportNameVersion)> action);
+
   /// Dump the Swift-specific name lookup tables we generate.
   void dumpSwiftLookupTables();
 

test/APINotes/Inputs/custom-frameworks/APINotesFrameworkTest.framework/Headers/APINotesFrameworkTest.apinotes

@@ -103,6 +103,9 @@ SwiftVersions:
           - Name: "importantClassProperty"
             PropertyKind: Class
             SwiftName: "swift3ClassProperty"
+          - Name: "importantInstanceProperty"
+            PropertyKind: Instance
+            SwiftName: "swift3InstanceProperty"
     Protocols:
       - Name: ProtoWithVersionedUnavailableMember
         Methods:

test/APINotes/Inputs/custom-frameworks/APINotesFrameworkTest.framework/Headers/Classes.h

@@ -14,6 +14,7 @@
 
 - (void)doImportantThings __attribute__((swift_name("finalDoImportantThings()")));
 @property (class, nullable) id importantClassProperty __attribute__((swift_name("finalClassProperty")));
+@property (nullable) id importantInstanceProperty __attribute__((swift_name("finalInstanceProperty")));
 @end
 
 #pragma clang assume_nonnull end

test/APINotes/versioned-objc-dynamic-lookup.swift

@@ -0,0 +1,35 @@
+// RUN: rm -rf %t && mkdir -p %t
+
+// RUN: not %target-swift-frontend -typecheck -F %S/Inputs/custom-frameworks -swift-version 4 %s 2>&1 | %FileCheck -check-prefix=CHECK-DIAGS -check-prefix=CHECK-DIAGS-4 %s
+// RUN: not %target-swift-frontend -typecheck -F %S/Inputs/custom-frameworks -swift-version 3 %s 2>&1 | %FileCheck -check-prefix=CHECK-DIAGS -check-prefix=CHECK-DIAGS-3 %s
+
+// REQUIRES: objc_interop
+
+import APINotesFrameworkTest
+
+func testRenamedClassMembers(obj: AnyObject) {
+  // CHECK-DIAGS-3: [[@LINE+1]]:{{[0-9]+}}: error: 'doImportantThings()' has been renamed to 'swift3DoImportantThings()'
+  obj.doImportantThings()
+  // CHECK-DIAGS-4: [[@LINE-1]]:{{[0-9]+}}: error: 'doImportantThings()' has been renamed to 'finalDoImportantThings()'
+
+  // CHECK-DIAGS-3-NOT: :[[@LINE+1]]:{{[0-9]+}}:
+  obj.swift3DoImportantThings()
+  // CHECK-DIAGS-4: [[@LINE-1]]:{{[0-9]+}}: error: 'swift3DoImportantThings()' has been renamed to 'finalDoImportantThings()'
+
+  // CHECK-DIAGS-3: [[@LINE+1]]:{{[0-9]+}}: error: 'finalDoImportantThings()' has been renamed to 'swift3DoImportantThings()'
+  obj.finalDoImportantThings()
+  // CHECK-DIAGS-4-NOT: :[[@LINE-1]]:{{[0-9]+}}:
+
+
+  // CHECK-DIAGS-3: [[@LINE+1]]:{{[0-9]+}}: error: 'importantInstanceProperty' has been renamed to 'swift3InstanceProperty'
+  _ = obj.importantInstanceProperty
+  // CHECK-DIAGS-4: [[@LINE-1]]:{{[0-9]+}}: error: 'importantInstanceProperty' has been renamed to 'finalInstanceProperty'
+
+  // CHECK-DIAGS-3-NOT: :[[@LINE+1]]:{{[0-9]+}}:
+  _ = obj.swift3InstanceProperty
+  // CHECK-DIAGS-4: [[@LINE-1]]:{{[0-9]+}}: error: 'swift3InstanceProperty' has been renamed to 'finalInstanceProperty'
+
+  // CHECK-DIAGS-3: [[@LINE+1]]:{{[0-9]+}}: error: 'finalInstanceProperty' has been renamed to 'swift3InstanceProperty'
+  _ = obj.finalInstanceProperty
+  // CHECK-DIAGS-4-NOT: :[[@LINE-1]]:{{[0-9]+}}:
+}