Merge pull request swiftlang#68479 from xymus/access-level-fix-private

Sema: Improve diagnostics logic for references to imported decl behind a restrictive import

Author: xymus

include/swift/AST/Decl.h

@@ -2788,6 +2788,16 @@ class ValueDecl : public Decl {
                         bool forConformance = false,
                         bool allowUsableFromInline = false) const;
 
+  using ImportAccessLevel = llvm::Optional<AttributedImport<ImportedModule>>;
+
+  /// Returns the import that may restrict the access to this decl
+  /// from \p useDC.
+  ///
+  /// If this decl and \p useDC are from the same module it returns
+  /// \c llvm::None. If there are many imports, it returns the most
+  /// permissive one.
+  ImportAccessLevel getImportAccessFrom(const DeclContext *useDC) const;
+
   /// Returns whether this declaration should be treated as \c open from
   /// \p useDC. This is very similar to #getFormalAccess, but takes
   /// \c \@testable into account.

include/swift/AST/DiagnosticsSema.def

@@ -2332,12 +2332,12 @@ ERROR(access_level_conflict_with_exported,none,
       (DeclAttribute, DeclAttribute))
 NOTE(module_imported_here,none,
      "module %0 imported as '%select{private|fileprivate|internal|package|%ERROR|%ERROR}1' here",
-     (Identifier, AccessLevel))
+     (const ModuleDecl*, AccessLevel))
 NOTE(decl_import_via_here,none,
      "%kind0 imported as "
      "'%select{private|fileprivate|internal|package|%ERROR|%ERROR}1' "
      "from %2 here",
-     (const ValueDecl *, AccessLevel, Identifier))
+     (const ValueDecl *, AccessLevel, const ModuleDecl*))
 
 // Opaque return types
 ERROR(opaque_type_invalid_constraint,none,

lib/AST/Decl.cpp

@@ -4071,6 +4071,16 @@ getAccessScopeForFormalAccess(const ValueDecl *VD,
     resultDC = resultDC->getParent();
   }
 
+  auto localImportRestriction = VD->getImportAccessFrom(useDC);
+  if (localImportRestriction.has_value()) {
+    AccessLevel importAccessLevel =
+      localImportRestriction.value().accessLevel;
+    if (access > importAccessLevel) {
+      access = std::min(access, importAccessLevel);
+      resultDC = useDC->getParentSourceFile();
+    }
+  }
+
   switch (access) {
   case AccessLevel::Private:
   case AccessLevel::FilePrivate:
@@ -4335,6 +4345,16 @@ bool ValueDecl::isAccessibleFrom(const DeclContext *useDC,
                      [&]() { return getFormalAccess(); });
 }
 
+ImportAccessLevel ValueDecl::getImportAccessFrom(const DeclContext *useDC) const {
+  ModuleDecl *Mod = getModuleContext();
+  if (useDC && useDC->getParentModule() != Mod) {
+    if (auto useSF = useDC->getParentSourceFile()) {
+      return useSF->getImportAccessLevel(Mod);
+    }
+  }
+  return llvm::None;
+}
+
 bool AbstractStorageDecl::isSetterAccessibleFrom(const DeclContext *DC,
                                                  bool forConformance) const {
   assert(isSettable(DC));

lib/AST/Module.cpp

@@ -3155,6 +3155,7 @@ bool SourceFile::hasTestableOrPrivateImport(
   case AccessLevel::Internal:
   case AccessLevel::Package:
   case AccessLevel::Public:
+  case AccessLevel::Open:
     // internal/public access only needs an import marked as @_private. The
     // filename does not need to match (and we don't serialize it for such
     // decls).
@@ -3173,8 +3174,6 @@ bool SourceFile::hasTestableOrPrivateImport(
                    desc.options.contains(ImportFlags::PrivateImport);
           }
         });
-  case AccessLevel::Open:
-    return true;
   case AccessLevel::FilePrivate:
   case AccessLevel::Private:
     // Fallthrough.

lib/Sema/ResilienceDiagnostics.cpp

@@ -61,28 +61,15 @@ bool TypeChecker::diagnoseInlinableDeclRefAccess(SourceLoc loc,
     return false;
 
   // General check on access-level of the decl.
+  auto *DC = where.getDeclContext();
   auto declAccessScope =
-      D->getFormalAccessScope(/*useDC=*/nullptr,
+      D->getFormalAccessScope(/*useDC=*/DC,
                               /*allowUsableFromInline=*/true);
 
-  // If the decl is imported, check if the import lowers it's access level.
-  auto importAccessLevel = AccessLevel::Public;
-  ImportAccessLevel problematicImport = llvm::None;
-
-  auto *DC = where.getDeclContext();
-  auto targetModule = D->getDeclContext()->getParentModule();
-  auto file = where.getDeclContext()->getParentSourceFile();
-  if (targetModule != DC->getParentModule() && file) {
-    problematicImport = file->getImportAccessLevel(targetModule);
-    if (problematicImport.has_value())
-      importAccessLevel = problematicImport->accessLevel;
-  }
-
   // Public declarations are OK, even if they're SPI or came from an
   // implementation-only import. We'll diagnose exportability violations
   // from diagnoseDeclRefExportability().
-  if (declAccessScope.isPublic() &&
-      importAccessLevel == AccessLevel::Public)
+  if (declAccessScope.isPublic())
     return false;
 
   auto &Context = DC->getASTContext();
@@ -119,20 +106,19 @@ bool TypeChecker::diagnoseInlinableDeclRefAccess(SourceLoc loc,
   if (downgradeToWarning == DowngradeToWarning::Yes)
     diagID = diag::resilience_decl_unavailable_warn;
 
-  auto diagAccessLevel = std::min(declAccessScope.accessLevelForDiagnostics(),
-                                  importAccessLevel);
-
+  AccessLevel diagAccessLevel = declAccessScope.accessLevelForDiagnostics();
   Context.Diags.diagnose(loc, diagID, D, diagAccessLevel,
                          fragileKind.getSelector());
 
   Context.Diags.diagnose(D, diag::resilience_decl_declared_here, D);
 
+  ImportAccessLevel problematicImport = D->getImportAccessFrom(DC);
   if (problematicImport.has_value() &&
-      diagAccessLevel == importAccessLevel) {
+      diagAccessLevel == problematicImport->accessLevel) {
     Context.Diags.diagnose(problematicImport->accessLevelLoc,
                            diag::decl_import_via_here, D,
                            problematicImport->accessLevel,
-                           problematicImport->module.importedModule->getName());
+                           problematicImport->module.importedModule);
   }
 
   return (downgradeToWarning == DowngradeToWarning::No);

lib/Sema/TypeAccessScopeChecker.h

@@ -31,7 +31,6 @@ class TypeAccessScopeChecker {
   bool TreatUsableFromInlineAsPublic;
 
   llvm::Optional<AccessScope> Scope = AccessScope::getPublic();
-  ImportAccessLevel ImportRestriction = llvm::None;
 
   TypeAccessScopeChecker(const DeclContext *useDC,
                          bool treatUsableFromInlineAsPublic)
@@ -44,41 +43,22 @@ class TypeAccessScopeChecker {
 
     auto AS = VD->getFormalAccessScope(File, TreatUsableFromInlineAsPublic);
     Scope = Scope->intersectWith(AS);
-
-    auto targetModule = VD->getDeclContext()->getParentModule();
-    if (targetModule != File->getParentModule()) {
-      auto localImportRestriction = File->getImportAccessLevel(targetModule);
-
-      if (localImportRestriction.has_value() &&
-          (!ImportRestriction.has_value() ||
-           localImportRestriction.value().accessLevel <
-             ImportRestriction.value().accessLevel)) {
-        ImportRestriction = localImportRestriction;
-      }
-    }
-
     return Scope.has_value();
   }
 
 public:
 
-  struct Result {
-    llvm::Optional<AccessScope> Scope;
-    ImportAccessLevel Import;
-  };
-
-  static Result
+  static llvm::Optional<AccessScope>
   getAccessScope(TypeRepr *TR, const DeclContext *useDC,
                  bool treatUsableFromInlineAsPublic = false) {
     TypeAccessScopeChecker checker(useDC, treatUsableFromInlineAsPublic);
     TR->walk(TypeReprIdentFinder([&](const IdentTypeRepr *typeRepr) {
       return checker.visitDecl(typeRepr->getBoundDecl());
     }));
-    return {checker.Scope,
-            checker.ImportRestriction};
+    return checker.Scope;
   }
 
-  static Result
+  static llvm::Optional<AccessScope>
   getAccessScope(Type T, const DeclContext *useDC,
                  bool treatUsableFromInlineAsPublic = false) {
     TypeAccessScopeChecker checker(useDC, treatUsableFromInlineAsPublic);
@@ -88,8 +68,7 @@ class TypeAccessScopeChecker {
       return TypeWalker::Action::Stop;
     }));
 
-    return {checker.Scope,
-            checker.ImportRestriction};
+    return checker.Scope;
   }
 };