[Swiftify] don't add macro when required type is unavailable

A Swift module only imports a type definition once. This means that some
clang modules may have function signatures imported with
`UnsafePointer<qux>` even if `qux` is only forward declared in that
module (which would normally be imported as `OpaquePointer`), because
some Swift file in the module imported the clang module with the
definition, so ClangImporter sees the definition. The macro expansion
only imports the imports of the clang module it belongs to however, so
namelookup for `qux` fails. This patch detects when this will result in
an error and avoids attaching the macro in those cases.

rdar://165864358

Author: hnrklssn

lib/ClangImporter/SwiftifyDecl.cpp

@@ -24,8 +24,10 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/AST/Type.h"
+#include "clang/Basic/Module.h"
 
 using namespace swift;
 using namespace importer;
@@ -271,14 +273,15 @@ struct CountedByExpressionValidator
 };
 
 
-// Don't try to transform any Swift types that _SwiftifyImport doesn't know how
-// to handle.
-static bool SwiftifiableCountedByPointerType(Type swiftType) {
+static bool IsConcretePointerType(Type swiftType) {
   Type nonnullType = swiftType->lookThroughSingleOptionalType();
   PointerTypeKind PTK;
   return nonnullType->getAnyPointerElementType(PTK) &&
     (PTK == PTK_UnsafePointer || PTK == PTK_UnsafeMutablePointer);
 }
+
+// Don't try to transform any Swift types that _SwiftifyImport doesn't know how
+// to handle.
 static bool SwiftifiableSizedByPointerType(const clang::ASTContext &ctx,
                                            Type swiftType,
                                            const clang::CountAttributedType *CAT) {
@@ -311,7 +314,7 @@ static bool SwiftifiableCAT(const clang::ASTContext &ctx,
   return CAT && CountedByExpressionValidator().Visit(CAT->getCountExpr()) &&
     (CAT->isCountInBytes() ?
        SwiftifiableSizedByPointerType(ctx, swiftType, CAT)
-     : SwiftifiableCountedByPointerType(swiftType));
+     : IsConcretePointerType(swiftType));
 }
 
 // Searches for template instantiations that are not behind type aliases.
@@ -331,6 +334,36 @@ struct UnaliasedInstantiationVisitor
   }
 };
 
+struct ForwardDeclaredConcreteTypeVisitor
+    : clang::RecursiveASTVisitor<ForwardDeclaredConcreteTypeVisitor> {
+  bool hasForwardDeclaredConcreteType = false;
+  clang::Module *Owner;
+
+  ForwardDeclaredConcreteTypeVisitor(clang::Module *Owner) : Owner(Owner) {};
+
+  bool VisitRecordType(clang::RecordType *RT) {
+    const clang::RecordDecl *RD = RT->getDecl()->getDefinition();
+    ASSERT(RD && "pointer to concrete type without type definition?");
+    const clang::Module *M = RD->getOwningModule();
+    if (!Owner->isModuleVisible(M)) {
+      hasForwardDeclaredConcreteType = true;
+      DLOG("Imported signature contains concrete type not available in clang module, skipping");
+      LLVM_DEBUG(DUMP(RD));
+      return false;
+    }
+    return true;
+  }
+
+  bool IsIncompatibleImport(Type SwiftTy, clang::QualType ClangTy) {
+    DLOG_SCOPE("Checking compatibility of type: " << ClangTy << "\n");
+    if (!IsConcretePointerType(SwiftTy)) {
+      return false;
+    }
+    TraverseType(ClangTy);
+    return hasForwardDeclaredConcreteType;
+  }
+};
+
 // until CountAttributedType::getAttributeName lands in our LLVM branch
 static StringRef getAttributeName(const clang::CountAttributedType *CAT) {
   switch (CAT->getKind()) {
@@ -375,6 +408,8 @@ static bool swiftifyImpl(ClangImporter::Implementation &Self,
 
   clang::ASTContext &clangASTContext = Self.getClangASTContext();
 
+  ForwardDeclaredConcreteTypeVisitor CheckForwardDecls(ClangDecl->getOwningModule());
+
   // We only attach the macro if it will produce an overload. Any __counted_by
   // will produce an overload, since UnsafeBufferPointer is still an improvement
   // over UnsafePointer, but std::span will only produce an overload if it also
@@ -389,6 +424,10 @@ static bool swiftifyImpl(ClangImporter::Implementation &Self,
     else
       ABORT("Unexpected AbstractFunctionDecl subclass.");
     clang::QualType clangReturnTy = ClangDecl->getReturnType();
+
+    if (CheckForwardDecls.IsIncompatibleImport(swiftReturnTy, clangReturnTy))
+      return false;
+
     bool returnIsStdSpan = printer.registerStdSpanTypeMapping(
         swiftReturnTy, clangReturnTy);
     auto *CAT = clangReturnTy->getAs<clang::CountAttributedType>();
@@ -440,6 +479,10 @@ static bool swiftifyImpl(ClangImporter::Implementation &Self,
       }
       ASSERT(swiftParam);
       Type swiftParamTy = swiftParam->getInterfaceType();
+
+      if (CheckForwardDecls.IsIncompatibleImport(swiftParamTy, clangParamTy))
+        return false;
+
       bool paramHasBoundsInfo = false;
       auto *CAT = clangParamTy->getAs<clang::CountAttributedType>();
       if (CAT && mappedIndex == SwiftifyInfoPrinter::SELF_PARAM_INDEX) {

test/Interop/C/swiftify-import/conflicting-opaqueness.swift

@@ -0,0 +1,148 @@
+// REQUIRES: swift_feature_SafeInteropWrappers
+
+// RUN: %empty-directory(%t)
+// RUN: split-file %s %t
+
+// RUN: %target-swift-frontend -emit-module -plugin-path %swift-plugin-dir -o %t/test.swiftmodule %t/test.swift -I %t -enable-experimental-feature SafeInteropWrappers -strict-memory-safety \
+// RUN:   -verify -verify-additional-file %t%{fs-sep}foo.h -verify-additional-file %t%{fs-sep}bar.h -verify-additional-file %t%{fs-sep}baz.h -verify-additional-file %t%{fs-sep}qux.h -Rmacro-expansions
+
+// Macro expansions in foo.h do not have access to the definition of `struct qux`,
+// so don't attach macro on functions that use contain that type in foo.h.
+// bar.h imports the type, so attach the macro.
+// baz.h imports bar.h which re-exports the type, so attach the macro there too.
+
+//--- foo.h
+#pragma once
+
+#define __counted_by(x) __attribute__((__counted_by__(x)))
+
+struct qux;
+// expected-note@+1{{'foo' declared here}}
+void foo(struct qux *x, int * __counted_by(len) p, int len);
+// expected-note@+1{{'fooReturn' declared here}}
+struct qux * fooReturn(int * __counted_by(len) p, int len);
+
+
+//--- bar.h
+#pragma once
+#include "qux.h"
+
+#define __counted_by(x) __attribute__((__counted_by__(x)))
+
+// expected-expansion@+10:59{{
+//   expected-remark@1{{macro content: |/// This is an auto-generated wrapper for safer interop|}}
+//   expected-remark@2{{macro content: |@_alwaysEmitIntoClient @_disfavoredOverload public func bar(_ x: UnsafeMutablePointer<qux>!, _ p: UnsafeMutableBufferPointer<Int32>) {|}}
+//   expected-remark@3{{macro content: |    let len = Int32(exactly: p.count)!|}}
+//   expected-remark@4{{macro content: |    return unsafe bar(x, p.baseAddress!, len)|}}
+//   expected-remark@5{{macro content: |}|}}
+// }}
+// expected-expansion@+3:6{{
+//   expected-note@1 5{{in expansion of macro '_SwiftifyImport' on global function 'bar' here}}
+// }}
+void bar(struct qux *x, int * __counted_by(len) p, int len);
+// expected-expansion@+10:58{{
+//   expected-remark@1{{macro content: |/// This is an auto-generated wrapper for safer interop|}}
+//   expected-remark@2{{macro content: |@_alwaysEmitIntoClient @_disfavoredOverload public func barReturn(_ p: UnsafeMutableBufferPointer<Int32>) -> UnsafeMutablePointer<qux>! {|}}
+//   expected-remark@3{{macro content: |    let len = Int32(exactly: p.count)!|}}
+//   expected-remark@4{{macro content: |    return unsafe barReturn(p.baseAddress!, len)|}}
+//   expected-remark@5{{macro content: |}|}}
+// }}
+// expected-expansion@+3:14{{
+//   expected-note@1 5{{in expansion of macro '_SwiftifyImport' on global function 'barReturn' here}}
+// }}
+struct qux * barReturn(int * __counted_by(len) p, int len);
+
+
+//--- baz.h
+#pragma once
+
+#include "bar.h"
+
+struct qux;
+// expected-expansion@+10:59{{
+//   expected-remark@1{{macro content: |/// This is an auto-generated wrapper for safer interop|}}
+//   expected-remark@2{{macro content: |@_alwaysEmitIntoClient @_disfavoredOverload public func baz(_ x: UnsafeMutablePointer<qux>!, _ p: UnsafeMutableBufferPointer<Int32>) {|}}
+//   expected-remark@3{{macro content: |    let len = Int32(exactly: p.count)!|}}
+//   expected-remark@4{{macro content: |    return unsafe baz(x, p.baseAddress!, len)|}}
+//   expected-remark@5{{macro content: |}|}}
+// }}
+// expected-expansion@+3:6{{
+//   expected-note@1 5{{in expansion of macro '_SwiftifyImport' on global function 'baz' here}}
+// }}
+void baz(struct qux *x, int * __counted_by(len) p, int len);
+// expected-expansion@+10:58{{
+//   expected-remark@1{{macro content: |/// This is an auto-generated wrapper for safer interop|}}
+//   expected-remark@2{{macro content: |@_alwaysEmitIntoClient @_disfavoredOverload public func bazReturn(_ p: UnsafeMutableBufferPointer<Int32>) -> UnsafeMutablePointer<qux>! {|}}
+//   expected-remark@3{{macro content: |    let len = Int32(exactly: p.count)!|}}
+//   expected-remark@4{{macro content: |    return unsafe bazReturn(p.baseAddress!, len)|}}
+//   expected-remark@5{{macro content: |}|}}
+// }}
+// expected-expansion@+3:14{{
+//   expected-note@1 5{{in expansion of macro '_SwiftifyImport' on global function 'bazReturn' here}}
+// }}
+struct qux * bazReturn(int * __counted_by(len) p, int len);
+
+
+//--- qux.h
+#pragma once
+
+struct qux { int placeholder; };
+
+
+//--- test.swift
+import Foo
+import Bar
+import Baz
+
+func callFoo(_ x: UnsafeMutablePointer<qux>, _ p: UnsafeMutablePointer<CInt>, _ len: CInt) {
+  unsafe foo(x, p, len)
+  let _: UnsafeMutablePointer<qux> = unsafe fooReturn(p, len)
+}
+
+func callFoo2(_ x: UnsafeMutablePointer<qux>, _ p: UnsafeMutableBufferPointer<CInt>) {
+  // expected-error@+2{{missing argument for parameter #3 in call}}
+  // expected-error@+1{{cannot convert value of type 'UnsafeMutableBufferPointer<CInt>' (aka 'UnsafeMutableBufferPointer<Int32>') to expected argument type 'UnsafeMutablePointer<Int32>'}}
+  unsafe foo(x, p)
+  // expected-error@+2{{missing argument for parameter #2 in call}}
+  // expected-error@+1{{cannot convert value of type 'UnsafeMutableBufferPointer<CInt>' (aka 'UnsafeMutableBufferPointer<Int32>') to expected argument type 'UnsafeMutablePointer<Int32>'}}
+  let _: UnsafeMutablePointer<qux> = unsafe fooReturn(p)
+}
+
+
+func callBar(_ x: UnsafeMutablePointer<qux>, _ p: UnsafeMutablePointer<CInt>, _ len: CInt) {
+  unsafe bar(x, p, len)
+  let _: UnsafeMutablePointer<qux> = unsafe barReturn(p, len)
+}
+
+func callBar2(_ x: UnsafeMutablePointer<qux>, _ p: UnsafeMutableBufferPointer<CInt>) {
+  unsafe bar(x, p)
+  let _: UnsafeMutablePointer<qux> = unsafe barReturn(p)
+}
+
+
+func callBaz(_ x: UnsafeMutablePointer<qux>, _ p: UnsafeMutablePointer<CInt>, _ len: CInt) {
+  unsafe baz(x, p, len)
+  let _: UnsafeMutablePointer<qux> = unsafe bazReturn(p, len)
+}
+
+func callBaz2(_ x: UnsafeMutablePointer<qux>, _ p: UnsafeMutableBufferPointer<CInt>) {
+  unsafe baz(x, p)
+  let _: UnsafeMutablePointer<qux> = unsafe bazReturn(p)
+}
+
+
+//--- module.modulemap
+module Foo {
+  header "foo.h"
+}
+module Bar {
+  header "bar.h"
+  export qux
+}
+module Baz {
+  header "baz.h"
+  export Bar
+}
+module qux {
+  header "qux.h"
+}