Merge pull request swiftlang#63355 from apple/es-export-pr

elsh · web-flow · commit 258b13b05df5 · 2023-02-03T15:20:29.000-08:00
Add access level and scope checks to package types
diff --git a/include/swift/AST/AccessScope.h b/include/swift/AST/AccessScope.h
@@ -85,13 +85,13 @@ class AccessScope {
     return !Value.getPointer() && Value.getInt() == AccessLimitKind::Package;
   }
 
-  /// Returns true if this scope is more restrictive than the argument scope.
-  /// It's often used to compute the min access scope. The order of restrictiveness
-  /// is: private (most restrictive), fileprivate, internal, package, public (least restrictive).
-  /// \see DeclContext::isChildContextOf
+  /// Returns true if the context of this (use site) is more restrictive than
+  /// the argument context (decl site). This function does _not_ check the
+  /// restrictiveness of the access level between this and the argument. \see
+  /// AccessScope::isInContext
   bool isChildOf(AccessScope AS) const {
-    if (isInternalOrLess()) {
-      if (AS.isInternalOrLess())
+    if (isInContext()) {
+      if (AS.isInContext())
         return allowsPrivateAccess(getDeclContext(), AS.getDeclContext());
       else
         return AS.isPackage() || AS.isPublic();
@@ -103,7 +103,27 @@ class AccessScope {
     return false;
   }
 
-  bool isInternalOrLess() const { return getDeclContext() != nullptr; }
+  /// Result depends on whether it's called at a use site or a decl site:
+  ///
+  /// For example,
+  ///
+  /// ```
+  /// public func foo(_ arg: bar) {} // `bar` is a `package` decl in another
+  /// module
+  /// ```
+  ///
+  /// The meaning of \c isInContext changes whether it's at the use site or the
+  /// decl site.
+  ///
+  /// The use site of \c bar, i.e. \c foo, is "in context" (decl context is
+  /// non-null), regardless of the access level of \c foo (\c public in this
+  /// case).
+  ///
+  /// The decl site of \c bar is only "in context" if the access level of the
+  /// decl is \c internal or more restrictive. The context at the decl site is\c
+  /// FileUnit if the decl is \c fileprivate or \c private; \c ModuleDecl if \c
+  /// internal, and null if \c package or \c public.
+  bool isInContext() const { return getDeclContext() != nullptr; }
 
   /// Returns the associated access level for diagnostic purposes.
   AccessLevel accessLevelForDiagnostics() const;
diff --git a/lib/AST/ASTVerifier.cpp b/lib/AST/ASTVerifier.cpp
@@ -944,12 +944,19 @@ class Verifier : public ASTWalker {
 
       if (D->hasAccess()) {
         PrettyStackTraceDecl debugStack("verifying access", D);
-        if (!D->getASTContext().isAccessControlDisabled() &&
-            D->getFormalAccessScope().isPublic() &&
-            D->getFormalAccess() <= AccessLevel::Package) {
-          Out << "non-public decl has no formal access scope\n";
-          D->dump(Out);
-          abort();
+        if (!D->getASTContext().isAccessControlDisabled()) {
+          if (D->getFormalAccessScope().isPackage() &&
+              D->getFormalAccess() < AccessLevel::Package) {
+            Out << "non-package decl has no formal access scope\n";
+            D->dump(Out);
+            abort();
+          }
+          if (D->getFormalAccessScope().isPublic() &&
+              D->getFormalAccess() < AccessLevel::Public) {
+            Out << "non-public decl has no formal access scope\n";
+            D->dump(Out);
+            abort();
+          }
         }
         if (D->getEffectiveAccess() == AccessLevel::Private) {
           Out << "effective access should use 'fileprivate' for 'private'\n";
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -3793,6 +3793,7 @@ getAccessScopeForFormalAccess(const ValueDecl *VD,
   case AccessLevel::Internal:
     return AccessScope(resultDC->getParentModule());
   case AccessLevel::Package:
+    return AccessScope::getPackage();
   case AccessLevel::Public:
   case AccessLevel::Open:
     return AccessScope::getPublic();
@@ -3828,8 +3829,15 @@ static bool checkAccessUsingAccessScopes(const DeclContext *useDC,
   if (accessScope.getDeclContext() == useDC) return true;
   if (!AccessScope(useDC).isChildOf(accessScope)) return false;
 
+  // useDC is null only when caller wants to skip non-public type checks.
+  if (!useDC) return true;
+
+  // Check package access; accessing package decl should not be allowed if package names are different
+  if (accessScope.isPackage())
+    return VD->getDeclContext()->getParentModule()->getPackageName() == useDC->getParentModule()->getPackageName();
+
   // Check SPI access
-  if (!useDC || !VD->isSPI()) return true;
+  if (!VD->isSPI()) return true;
   auto useSF = dyn_cast<SourceFile>(useDC->getModuleScopeContext());
   return !useSF || useSF->isImportedAsSPI(VD) ||
          VD->getDeclContext()->getParentModule() == useDC->getParentModule();
diff --git a/lib/AST/DeclContext.cpp b/lib/AST/DeclContext.cpp
@@ -583,7 +583,7 @@ void DeclContext::dumpContext() const {
 void AccessScope::dump() const {
   llvm::errs() << getAccessLevelSpelling(accessLevelForDiagnostics()) << ": ";
 
-  if (isPublic()) {
+  if (isPublic() || isPackage()) {
     llvm::errs() << "(null)\n";
     return;
   }
@@ -1194,7 +1194,7 @@ AccessScope::AccessScope(const DeclContext *DC, AccessLimitKind limitKind)
     isPrivate = true;
   }
   if (!DC || isa<ModuleDecl>(DC))
-    assert(!isPrivate && "public or internal scope can't be private");
+    assert(!isPrivate && "public, package, or internal scope can't be private");
 }
 
 bool AccessScope::isFileScope() const {
@@ -1210,6 +1210,8 @@ bool AccessScope::isInternal() const {
 AccessLevel AccessScope::accessLevelForDiagnostics() const {
   if (isPublic())
     return AccessLevel::Public;
+  if (isPackage())
+    return AccessLevel::Package;
   if (isa<ModuleDecl>(getDeclContext()))
     return AccessLevel::Internal;
   if (getDeclContext()->isModuleScopeContext()) {
diff --git a/lib/DriverTool/swift_symbolgraph_extract_main.cpp b/lib/DriverTool/swift_symbolgraph_extract_main.cpp
@@ -181,6 +181,7 @@ int swift_symbolgraph_extract_main(ArrayRef<const char *> Args,
         llvm::StringSwitch<AccessLevel>(A->getValue())
             .Case("open", AccessLevel::Open)
             .Case("public", AccessLevel::Public)
+            .Case("package", AccessLevel::Package)
             .Case("internal", AccessLevel::Internal)
             .Case("fileprivate", AccessLevel::FilePrivate)
             .Case("private", AccessLevel::Private)
diff --git a/lib/Frontend/CompilerInvocation.cpp b/lib/Frontend/CompilerInvocation.cpp
@@ -1347,6 +1347,7 @@ static void ParseSymbolGraphArgs(symbolgraphgen::SymbolGraphOptions &Opts,
         llvm::StringSwitch<AccessLevel>(A->getValue())
             .Case("open", AccessLevel::Open)
             .Case("public", AccessLevel::Public)
+            .Case("package", AccessLevel::Package)
             .Case("internal", AccessLevel::Internal)
             .Case("fileprivate", AccessLevel::FilePrivate)
             .Case("private", AccessLevel::Private)
diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp
@@ -2067,6 +2067,7 @@ bool Parser::parseDocumentationAttributeArgument(Optional<StringRef> &Metadata,
       llvm::StringSwitch<Optional<AccessLevel>>(ArgumentValue)
         .Case("open", AccessLevel::Open)
         .Case("public", AccessLevel::Public)
+        .Case("package", AccessLevel::Package)
         .Case("internal", AccessLevel::Internal)
         .Case("private", AccessLevel::Private)
         .Case("fileprivate", AccessLevel::FilePrivate)
@@ -2728,6 +2729,7 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
       .Case("private", AccessLevel::Private)
       .Case("fileprivate", AccessLevel::FilePrivate)
       .Case("internal", AccessLevel::Internal)
+      .Case("package", AccessLevel::Package)
       .Case("public", AccessLevel::Public)
       .Case("open", AccessLevel::Open);
 
diff --git a/lib/Sema/TypeCheckAccess.cpp b/lib/Sema/TypeCheckAccess.cpp
@@ -182,7 +182,7 @@ void AccessControlCheckerBase::checkTypeAccessImpl(
     return;
   // Don't spend time checking local declarations; this is always valid by the
   // time we get to this point.
-  if (!contextAccessScope.isPublic() &&
+  if (contextAccessScope.isInContext() &&
       contextAccessScope.getDeclContext()->isLocalContext())
     return;
 
@@ -2106,6 +2106,8 @@ static void checkExtensionGenericParamAccess(const ExtensionDecl *ED) {
     desiredAccessScope = AccessScope(ED->getModuleContext());
     break;
   case AccessLevel::Package:
+    desiredAccessScope = AccessScope::getPackage();
+    break;
   case AccessLevel::Public:
   case AccessLevel::Open:
     break;
diff --git a/test/Sema/accessibility.swift b/test/Sema/accessibility.swift
