Merge pull request #23702 from rjmccall/implementation-only-diagnostics

[WIP] implementation-only import checking

Author: rjmccall

include/swift/AST/DiagnosticsSema.def

@@ -4099,6 +4099,11 @@ WARNING(resilience_decl_unavailable_warn,
         "should not be referenced from " FRAGILE_FUNC_KIND "3",
         (DescriptiveDeclKind, DeclName, AccessLevel, unsigned, bool))
 
+ERROR(inlinable_decl_ref_implementation_only,
+      none, "%0 %1 cannot be used in an inlinable "
+      "function because its module was imported implementation-only",
+      (DescriptiveDeclKind, DeclName))
+
 #undef FRAGILE_FUNC_KIND
 
 NOTE(resilience_decl_declared_here_public,

include/swift/AST/Module.h

@@ -959,6 +959,10 @@ class SourceFile final : public FileUnit {
   /// May be -1, to indicate no association with a buffer.
   int BufferID;
 
+  /// Does this source file have any implementation-only imports?
+  /// If not, we can fast-path module checks.
+  bool HasImplementationOnlyImports = false;
+
   /// The list of protocol conformances that were "used" within this
   /// source file.
   llvm::SetVector<NormalProtocolConformance *> UsedConformances;
@@ -1047,6 +1051,12 @@ class SourceFile final : public FileUnit {
   hasTestableOrPrivateImport(AccessLevel accessLevel, const ValueDecl *ofDecl,
                              ImportQueryKind kind = TestableAndPrivate) const;
 
+  bool hasImplementationOnlyImports() const {
+    return HasImplementationOnlyImports;
+  }
+
+  bool isImportedImplementationOnly(const ModuleDecl *module) const;
+
   void clearLookupCache();
 
   void cacheVisibleDecls(SmallVectorImpl<ValueDecl *> &&globals) const;

lib/AST/Module.cpp

@@ -1421,6 +1421,14 @@ void SourceFile::addImports(ArrayRef<ImportedModuleDesc> IM) {
   assert(iter == newBuf.end());
 
   Imports = newBuf;
+
+  // Update the HasImplementationOnlyImports flag.
+  if (!HasImplementationOnlyImports) {
+    for (auto &desc : IM) {
+      if (desc.importOptions.contains(ImportFlags::ImplementationOnly))
+        HasImplementationOnlyImports = true;
+    }
+  }
 }
 
 bool SourceFile::hasTestableOrPrivateImport(
@@ -1485,6 +1493,43 @@ bool SourceFile::hasTestableOrPrivateImport(
                      });
 }
 
+bool SourceFile::isImportedImplementationOnly(const ModuleDecl *module) const {
+  // Implementation-only imports are (currently) always source-file-specific,
+  // so if we don't have any, we know the search is complete.
+  if (!hasImplementationOnlyImports())
+    return false;
+
+  auto isImportedBy = [](const ModuleDecl *dest, const ModuleDecl *src) {
+    // Fast path.
+    if (dest == src) return true;
+
+    // Walk the transitive imports, respecting visibility.
+    // This return true if the search *didn't* short-circuit, and it short
+    // circuits if we found `dest`, so we need to invert the sense before
+    // returning.
+    return !const_cast<ModuleDecl*>(src)
+              ->forAllVisibleModules({}, [dest](ModuleDecl::ImportedModule im) {
+      // Continue searching as long as we haven't found `dest` yet.
+      return im.second != dest;
+    });
+  };
+
+  // Look at the imports of this source file.
+  for (auto &desc : Imports) {
+    // Ignore implementation-only imports.
+    if (desc.importOptions.contains(ImportFlags::ImplementationOnly))
+      continue;
+
+    // If the module is imported this way, it's not imported
+    // implementation-only.
+    if (isImportedBy(module, desc.module.second))
+      return false;
+  }
+
+  // Now check this file's enclosing module in case there are re-exports.
+  return !isImportedBy(module, getParentModule());
+}
+
 void SourceFile::clearLookupCache() {
   if (!Cache)
     return;

lib/Sema/ResilienceDiagnostics.cpp

@@ -102,6 +102,8 @@ bool TypeChecker::diagnoseInlinableDeclRef(SourceLoc loc,
                                            const DeclContext *DC,
                                            FragileFunctionKind Kind,
                                            bool TreatUsableFromInlineAsPublic) {
+  // Do some important fast-path checks that apply to all cases.
+
   // Local declarations are OK.
   if (D->getDeclContext()->isLocalContext())
     return false;
@@ -110,6 +112,23 @@ bool TypeChecker::diagnoseInlinableDeclRef(SourceLoc loc,
   if (isa<AbstractTypeParamDecl>(D))
     return false;
 
+  // Check whether the declaration is accessible.
+  if (diagnoseInlinableDeclRefAccess(loc, D, DC, Kind,
+                                     TreatUsableFromInlineAsPublic))
+    return true;
+
+  // Check whether the declaration comes from a publically-imported module.
+  if (diagnoseDeclRefExportability(loc, D, DC))
+    return true;
+
+  return false;
+}
+
+bool TypeChecker::diagnoseInlinableDeclRefAccess(SourceLoc loc,
+                                           const ValueDecl *D,
+                                           const DeclContext *DC,
+                                           FragileFunctionKind Kind,
+                                           bool TreatUsableFromInlineAsPublic) {
   // Public declarations are OK.
   if (D->getFormalAccessScope(/*useDC=*/nullptr,
                               TreatUsableFromInlineAsPublic).isPublic())
@@ -178,3 +197,38 @@ bool TypeChecker::diagnoseInlinableDeclRef(SourceLoc loc,
   return (downgradeToWarning == DowngradeToWarning::No);
 }
 
+bool TypeChecker::diagnoseDeclRefExportability(SourceLoc loc,
+                                               const ValueDecl *D,
+                                               const DeclContext *DC) {
+  // We're only interested in diagnosing uses from source files.
+  auto userSF = DC->getParentSourceFile();
+  if (!userSF)
+    return false;
+
+  // If the source file doesn't have any implementation-only imports,
+  // we can fast-path this.  In the current language design, we never
+  // need to consider the possibility of implementation-only imports
+  // from other source files in the module (or indirectly in other modules).
+  // TODO: maybe check whether D is from a bridging header?
+  if (!userSF->hasImplementationOnlyImports())
+    return false;
+
+  auto userModule = userSF->getParentModule();
+  auto definingModule = D->getModuleContext();
+
+  // Nothing to diagnose in the very common case of the same module.
+  if (userModule == definingModule)
+    return false;
+
+  // Nothing to diagnose in the very common case that the module is
+  // imported for use in signatures.
+  if (!userSF->isImportedImplementationOnly(definingModule))
+    return false;
+
+  // TODO: different diagnostics
+  diagnose(loc, diag::inlinable_decl_ref_implementation_only,
+           D->getDescriptiveKind(), D->getFullName());
+
+  // TODO: notes explaining why
+  return true;
+}

lib/Sema/TypeChecker.h

@@ -1933,6 +1933,19 @@ class TypeChecker final : public LazyResolver {
                                 FragileFunctionKind Kind,
                                 bool TreatUsableFromInlineAsPublic);
 
+private:
+  bool diagnoseInlinableDeclRefAccess(SourceLoc loc, const ValueDecl *D,
+                                      const DeclContext *DC,
+                                      FragileFunctionKind Kind,
+                                      bool TreatUsableFromInlineAsPublic);
+
+public:
+  /// Given that a declaration is used from a particular context which
+  /// exposes it in the interface of the current module, diagnose if it cannot
+  /// reasonably be shared.
+  bool diagnoseDeclRefExportability(SourceLoc loc, const ValueDecl *D,
+                                    const DeclContext *DC);
+
   /// Given that \p DC is within a fragile context for some reason, describe
   /// why.
   ///

test/Sema/Inputs/implementation-only-imports/directs.swift

@@ -0,0 +1,30 @@
+@_exported import indirects
+
+public struct StructFromDirect {
+  public func method() {}
+  public var property: Int {
+    get { return 0 }
+    set {}
+  }
+  public subscript(index: Int) -> Int {
+    get { return 0 }
+    set {}
+  }
+}
+public typealias AliasFromDirect = StructFromDirect
+public typealias GenericAliasFromDirect<T> = (StructFromDirect, T)
+
+public func globalFunctionFromDirect() {}
+public var globalVariableFromDirect = 0
+
+extension StructFromIndirect {
+  public func extensionMethodFromDirect() {}
+  public var extensionPropertyFromDirect: Int {
+    get { return 0 }
+    set {}
+  }
+  public subscript(extensionSubscript index: Int) -> Int {
+    get { return 0 }
+    set {}
+  }
+}

test/Sema/Inputs/implementation-only-imports/indirects.swift

@@ -0,0 +1,8 @@
+public struct StructFromIndirect {
+  public init() {}
+}
+public typealias AliasFromIndirect = StructFromIndirect
+public typealias GenericAliasFromIndirect<T> = (StructFromIndirect, T)
+
+public func globalFunctionFromIndirect() {}
+public var globalVariableFromIndirect = 0

test/Sema/Inputs/implementation-only-imports/secondary_file.swift

@@ -0,0 +1 @@
+@_exported import indirects

test/Sema/implementation-only-import-inlinable-indirect.swift

@@ -0,0 +1,43 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module -o %t/indirects.swiftmodule %S/Inputs/implementation-only-imports/indirects.swift
+// RUN: %target-swift-frontend -emit-module -o %t/directs.swiftmodule -I %t %S/Inputs/implementation-only-imports/directs.swift
+
+// RUN: %target-swift-frontend -typecheck -verify %s -I %t
+
+@_implementationOnly import directs
+
+// Types
+
+@inlinable
+public func testStructFromIndirect() {
+  _ = StructFromIndirect() // expected-error {{cannot be used in an inlinable function because its module was imported implementation-only}}
+}
+
+@inlinable
+public func testAliasFromIndirect() {
+  _ = AliasFromIndirect() // expected-error {{cannot be used in an inlinable function because its module was imported implementation-only}}
+}
+
+@inlinable
+public func testGenericAliasFromIndirect() {
+  _ = GenericAliasFromIndirect<Int>() // expected-error {{cannot be used in an inlinable function because its module was imported implementation-only}}
+}
+
+// Functions
+
+@inlinable
+public func testFunctionFromIndirect() {
+  globalFunctionFromIndirect() // expected-error {{cannot be used in an inlinable function because its module was imported implementation-only}}
+}
+
+// Variables
+
+@inlinable
+public func testVariableFromIndirect_get() {
+  _ = globalVariableFromIndirect // expected-error {{cannot be used in an inlinable function because its module was imported implementation-only}}
+}
+
+@inlinable
+public func testVariableFromIndirect_set() {
+  globalVariableFromIndirect = 5 // expected-error {{cannot be used in an inlinable function because its module was imported implementation-only}}
+}

test/Sema/implementation-only-import-inlinable-multifile.swift

@@ -0,0 +1,104 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module -o %t/indirects.swiftmodule %S/Inputs/implementation-only-imports/indirects.swift
+// RUN: %target-swift-frontend -emit-module -o %t/directs.swiftmodule -I %t %S/Inputs/implementation-only-imports/directs.swift
+
+// RUN: %target-swift-frontend -typecheck -verify -primary-file %s %S/Inputs/implementation-only-imports/secondary_file.swift -I %t
+
+@_implementationOnly import directs
+// 'indirects' is imported for re-export in a secondary file
+
+// Types
+
+@inlinable
+public func testStructFromDirect() {
+  _ = StructFromDirect() // expected-error {{cannot be used in an inlinable function because its module was imported implementation-only}}
+}
+
+@inlinable
+public func testStructFromIndirect() {
+  _ = StructFromIndirect()
+}
+
+@inlinable
+public func testAliasFromDirect() {
+  _ = AliasFromDirect() // expected-error {{cannot be used in an inlinable function because its module was imported implementation-only}}
+}
+
+@inlinable
+public func testAliasFromIndirect() {
+  _ = AliasFromIndirect()
+}
+
+@inlinable
+public func testGenericAliasFromDirect() {
+  _ = GenericAliasFromDirect<Int>() // expected-error {{cannot be used in an inlinable function because its module was imported implementation-only}}
+}
+
+@inlinable
+public func testGenericAliasFromIndirect() {
+  let tmp: GenericAliasFromIndirect<Int>?
+  _ = tmp
+}
+
+// Functions
+
+@inlinable
+public func testFunctionFromDirect() {
+  globalFunctionFromDirect() // expected-error {{cannot be used in an inlinable function because its module was imported implementation-only}}
+}
+
+@inlinable
+public func testFunctionFromIndirect() {
+  globalFunctionFromIndirect()
+}
+
+// Variables
+
+@inlinable
+public func testVariableFromDirect_get() {
+  _ = globalVariableFromDirect // expected-error {{cannot be used in an inlinable function because its module was imported implementation-only}}
+}
+
+@inlinable
+public func testVariableFromIndirect_get() {
+  _ = globalVariableFromIndirect
+}
+
+@inlinable
+public func testVariableFromDirect_set() {
+  globalVariableFromDirect = 5 // expected-error {{cannot be used in an inlinable function because its module was imported implementation-only}}
+}
+
+@inlinable
+public func testVariableFromIndirect_set() {
+  globalVariableFromIndirect = 5
+}
+
+// Extensions
+
+@inlinable
+public func testExtensionMethod(s: inout StructFromIndirect) {
+  s.extensionMethodFromDirect() // expected-error {{cannot be used in an inlinable function because its module was imported implementation-only}}
+}
+
+@inlinable
+public func testExtensionProperty_get(s: inout StructFromIndirect) {
+  _ = s.extensionPropertyFromDirect // expected-error {{cannot be used in an inlinable function because its module was imported implementation-only}}
+}
+
+@inlinable
+public func testExtensionProperty_set(s: inout StructFromIndirect) {
+  s.extensionPropertyFromDirect = 5 // expected-error {{cannot be used in an inlinable function because its module was imported implementation-only}}
+}
+
+@inlinable
+public func testExtensionSubscript_get(s: inout StructFromIndirect) {
+  // FIXME: why is this error being double-emitted?
+  _ = s[extensionSubscript: 0] // expected-error 2 {{cannot be used in an inlinable function because its module was imported implementation-only}}
+}
+
+@inlinable
+public func testExtensionSubscript_set(s: inout StructFromIndirect) {
+  // FIXME: why is this error being double-emitted?
+  s[extensionSubscript: 0] = 5 // expected-error 2 {{cannot be used in an inlinable function because its module was imported implementation-only}}
+}