[ClangImporter] Don't use local scope decls to find a function's home (swiftlang#27440)

In C, a function can be declared at local scope:

    void aFunctionInBase(void) {
      void theFunctionInQuestion(int);
    }

and then again in a different header at top-level scope:

    void theFunctionInQuestion(int);

If the first one appears first, it becomes what Clang considers the
"canonical" declaration, which (up until now) Swift has been using to
decide what module to import a function into. (Since a C function can
be redeclared arbitrarily many times, we have to pick one.) This is
important for diagnostics and anything else that might ask "where did
this Swift declaration come from". Instead of the very first
redeclaration, use the first non-local one to determine the "home"
module.

(The standard library wants a guarantee that forward declarations they
put in SwiftShims won't interfere with declarations found elsewhere. I
don't think Clang can /ever/ provide that, so if there's ever a
mismatch between the standard library's forward declarations and the
"real" declarations the standard library's might win out at the LLVM
level---say, in terms of attributes. But this at least removes a place
where that could be visible to users even when it isn't otherwise a
problem.)

Author: jrose-apple

lib/ClangImporter/ClangAdapter.cpp

@@ -77,6 +77,17 @@ importer::getDefinitionForClangTypeDecl(const clang::Decl *D) {
   return None;
 }
 
+const clang::Decl *
+importer::getFirstNonLocalDecl(const clang::Decl *D) {
+  D = D->getCanonicalDecl();
+  auto iter = llvm::find_if(D->redecls(), [](const clang::Decl *next) -> bool {
+    return !next->isLexicallyWithinFunctionOrMethod();
+  });
+  if (iter == D->redecls_end())
+    return nullptr;
+  return *iter;
+}
+
 Optional<clang::Module *>
 importer::getClangSubmoduleForDecl(const clang::Decl *D,
                                    bool allowForwardDeclaration) {
@@ -91,7 +102,7 @@ importer::getClangSubmoduleForDecl(const clang::Decl *D,
   }
 
   if (!actual)
-    actual = D->getCanonicalDecl();
+    actual = getFirstNonLocalDecl(D);
 
   return actual->getImportedOwningModule();
 }

lib/ClangImporter/ClangAdapter.h

@@ -60,6 +60,34 @@ struct PlatformAvailability;
 Optional<const clang::Decl *>
 getDefinitionForClangTypeDecl(const clang::Decl *D);
 
+/// Returns the first redeclaration of \p D outside of a function.
+///
+/// C allows redeclaring most declarations in function bodies, as so:
+///
+///     void usefulPublicFunction(void) {
+///       extern void importantInternalFunction(int code);
+///       importantInternalFunction(42);
+///     }
+///
+/// This should allow clients to call \c usefulPublicFunction without exposing
+/// \c importantInternalFunction . However, if there is another declaration of
+/// \c importantInternalFunction later, Clang still needs to treat them as the
+/// same function. This is normally fine...except that if the local declaration
+/// is the \e first declaration, it'll also get used as the "canonical"
+/// declaration that Clang (and Swift) use for uniquing purposes.
+///
+/// Every imported declaration gets assigned to a module in Swift, and for
+/// declarations without definitions that choice is somewhat arbitrary. But it
+/// would be better not to pick a local declaration like the one above, and
+/// therefore this method should be used instead of
+/// clang::Decl::getCanonicalDecl when the containing module is important.
+///
+/// If there are no non-local redeclarations, returns null.
+/// If \p D is not a kind of declaration that supports being redeclared, just
+/// returns \p D itself.
+const clang::Decl *
+getFirstNonLocalDecl(const clang::Decl *D);
+
 /// Returns the module \p D comes from, or \c None if \p D does not have
 /// a valid associated module.
 ///

test/ClangImporter/Inputs/custom-modules/LocalVsFileScope.h

@@ -0,0 +1,3 @@
+#include "LocalVsFileScopeBase.h"
+
+void theFunctionInQuestion(int);

test/ClangImporter/Inputs/custom-modules/LocalVsFileScopeBase.h

@@ -0,0 +1,3 @@
+void aFunctionInBase(void) {
+  void theFunctionInQuestion(int);
+}

test/ClangImporter/Inputs/custom-modules/module.map

@@ -233,3 +233,11 @@ module ConditionallyFoo {
 module ForwardDeclarationsHelper {
   header "ForwardDeclarationsHelper.h"
 }
+
+module LocalVsFileScopeBase {
+  header "LocalVsFileScopeBase.h"
+}
+module LocalVsFileScope {
+  header "LocalVsFileScope.h"
+  export *
+}

test/ClangImporter/cfuncs_scope.swift

@@ -0,0 +1,18 @@
+// RUN: not %target-swift-frontend -typecheck %s -I %S/Inputs/custom-modules 2>&1 | %FileCheck %s
+
+import LocalVsFileScope
+
+func testLocalVsFileScope() {
+  LocalVsFileScopeBase.theFunctionInQuestion()
+  // CHECK: :[[@LINE-1]]:3: error: module 'LocalVsFileScopeBase' has no member named 'theFunctionInQuestion'
+
+  theFunctionInQuestion()
+  // CHECK: :[[@LINE-1]]:25: error: missing argument
+  // CHECK: LocalVsFileScope.theFunctionInQuestion:1:{{[0-9]+}}: note:
+  // This is not a wonderful test because it's relying on the diagnostic
+  // engine's synthesis of fake declarations to figure out what module the
+  // importer assigned the function to. But, well, that's what we get.
+
+  aFunctionInBase() // just make sure it's imported
+  // CHECK-NOT: :[[@LINE-1]]:{{[0-9]+}}:
+}