Associate PackageUnit with ModuleDecl

* Weakly reference ModuleDecl from PackageUnit
* Add PackageUnit decl context getter and use it for a package AccessScope
* Return module decl referenced by PackageUnit in getModuleScopeContext and getParentModule
* Handle package acl in access scope checkers
* Remove AccessLimitKind
* Fix tests
Resolves rdar://104987295, rdar://105187216, rdar://104723918

Author: elsh

include/swift/AST/AccessScope.h

@@ -20,40 +20,39 @@
 
 namespace swift {
 
-/// Used to provide the kind of scope limitation in AccessScope::Value
-enum class AccessLimitKind : uint8_t { None = 0, Private, Package };
-
 /// The wrapper around the outermost DeclContext from which
 /// a particular declaration can be accessed.
 class AccessScope {
-  /// The declaration context along with an enum indicating the level of
-  /// scope limitation.
-  /// If the declaration context is set, and the limit kind is Private, the
-  /// access level is considered 'private'. Whether it's 'internal' or
-  /// 'fileprivate' is determined by what the declaration context casts to. If
-  /// the declaration context is null, and the limit kind is None, the access
-  /// level is considered 'public'. If the limit kind is Private, the access
-  /// level is considered SPI. If it's Package, the access level is considered
-  /// 'package'. Below is a table showing the combinations.
+  /// To validate access of a decl, access scope check for both decl site
+  /// and use site needs to be done. The underlying mechanism uses a
+  /// <DeclContext*, bool> pair to determine the scope, as shown
+  /// in the table below.
   ///
-  /// AccessLimitKind       DC == nullptr               DC != nullptr
-  /// ---------------------------------------------------------------------------
-  ///  None                    public                  fileprivate or internal (check DC to tell which)
-  ///  Private               `@_spi` public                private
-  ///  Package                 package                    (unused)
-
-  llvm::PointerIntPair<const DeclContext *, 2, AccessLimitKind> Value;
+  /// <DeclContext*, bool>     AccessScope      AccessLevel
+  /// --------------------------------------------
+  ///  <nullptr, false>                Public             public or open
+  ///  <nullptr, true>                  Public             public `@_spi`
+  ///  <PackageUnit*, _>             Package             package
+  ///  <ModuleDecl*, _>                Module               internal
+  ///  <FileUnit*, false>           FileScope           fileprivate
+  ///  <FileUnit*, true>              Private               private
+  ///
+  /// For example, if a decl with `public` access level is referenced outside of
+  /// its defining module, it will be maped to the <nullptr, false> pair during
+  /// the access scope check. This pair is determined based on the decl's access
+  /// level in  \c getAccessScopeForFormalAccess and passed to
+  /// \c checkAccessUsingAccessScope which compares access scope of the
+  /// use site and decl site.
+  ///
+  /// \see AccessScope::getAccessScopeForFormalAccess
+  /// \see AccessScope::checkAccessUsingAccessScope
+  /// \see DeclContext::ASTHierarchy
+  llvm::PointerIntPair<const DeclContext *, 1, bool> Value;
 
 public:
-  AccessScope(const DeclContext *DC,
-              AccessLimitKind limitKind = AccessLimitKind::None);
+  AccessScope(const DeclContext *DC, bool isPrivate = false);
 
-  static AccessScope getPublic() {
-    return AccessScope(nullptr, AccessLimitKind::None);
-  }
-  static AccessScope getPackage() {
-    return AccessScope(nullptr, AccessLimitKind::Package);
-  }
+  static AccessScope getPublic() { return AccessScope(nullptr, false); }
 
   /// Check if private access is allowed. This is a lexical scope check in Swift
   /// 3 mode. In Swift 4 mode, declarations and extensions of the same type will
@@ -66,63 +65,60 @@ class AccessScope {
   bool operator==(AccessScope RHS) const { return Value == RHS.Value; }
   bool operator!=(AccessScope RHS) const { return !(*this == RHS); }
   bool hasEqualDeclContextWith(AccessScope RHS) const {
-    if (isPublic())
-      return RHS.isPublic();
-    if (isPackage())
-      return RHS.isPackage();
     return getDeclContext() == RHS.getDeclContext();
   }
 
-  bool isPublic() const {
-    return !Value.getPointer() && Value.getInt() == AccessLimitKind::None;
-  }
-  bool isPrivate() const {
-    return Value.getPointer() && Value.getInt() == AccessLimitKind::Private;
-  }
+  bool isPublic() const { return !Value.getPointer(); }
+  bool isPrivate() const { return Value.getPointer() && Value.getInt(); }
   bool isFileScope() const;
   bool isInternal() const;
-  bool isPackage() const {
-    return !Value.getPointer() && Value.getInt() == AccessLimitKind::Package;
-  }
+  bool isPackage() const;
 
-  /// 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
+  /// Checks if the DeclContext of this (use site) access scope is more
+  /// restrictive than that of the argument (decl site) based on the DeclContext
+  /// hierarchy: (most to least restrictive)
+  /// decl/expr (e.g. ClassDecl) -> FileUnit -> ModuleDecl -> PackageUnit -> nullptr
+  ///
+  /// A few things to note:
+  /// 1. If both have the same DeclContext, returns false as one is _not_ a
+  /// child of the other.
+  /// 2. This function does _not_ check the restrictiveness of the _access
+  /// level_ between two decls.
+  /// 3. The DeclContext of this (use site) may not be null even if the use site has
+  /// a `public` access level.
+  ///
+  /// Here's an example while typechecking a file with the following code.
+  ///
+  /// ```
+  /// import OtherModule
+  ///
+  /// // `Foo` is a `public` struct defined in `OtherModule`
+  /// public func myFunc(_ arg: OtherModule.Foo) {}
+  /// ```
+  ///
+  /// The use site of `Foo`is a function `myFunc`, and its DeclContext is non-null
+  /// (FileUnit) even though the function decl itself has a `public` access level.
+  /// When `isChildOf` is called, the argument passed in is a pair <nullptr,
+  /// false> created in \c getAccessScopeForFormalAccess based on the access
+  /// level of the decl `Foo`. Since FileUnit is a child of nullptr in the DeclContext
+  /// hierarchy (described above), it returns true.
+  ///
+  /// \see AccessScope::getAccessScopeForFormalAccess
+  /// \see AccessScope::checkAccessUsingAccessScope
+  /// \see DeclContext::ASTHierarchy
   bool isChildOf(AccessScope AS) const {
-    if (isInContext()) {
+    if (isPackage()) { // This needs to be checked first before isInContext
+      return AS.isPublic();
+    } else if (isInContext()) {
       if (AS.isInContext())
         return allowsPrivateAccess(getDeclContext(), AS.getDeclContext());
       else
-        return AS.isPackage() || AS.isPublic();
+        return AS.isPublic();
+    } else {// It's public, so can't be a child of the argument scope
+      return false;
     }
-    if (isPackage())
-      return AS.isPublic();
-    // If this is public, it can't be less than access level of AS
-    // so return false
-    return false;
   }
 
-  /// 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.

include/swift/AST/DeclContext.h

@@ -229,6 +229,15 @@ struct FragileFunctionKind {
 /// and therefore can safely access trailing memory. If you need to create a
 /// macro context, please see GenericContext for how to minimize new entries in
 /// the ASTHierarchy enum below.
+///
+/// The hierarchy between DeclContext subclasses is set in their ctors. For example,
+/// FileUnit ctor takes ModuleDecl as its parent DeclContext. The hierarchy from the most
+/// to least restrictive order is:
+///  decl/expr (e.g. ClassDecl) -> FileUnit -> ModuleDecl -> PackageUnit -> nullptr
+///
+/// There's an exception, however; the parent of ModuleDecl is set nullptr, not set to
+/// PackageUnit; ModuleDecl has a pointer to PackageUnit as its field, and it is treated as
+/// the enclosing scope of ModuleDecl.
 class alignas(1 << DeclContextAlignInBits) DeclContext
     : public ASTAllocated<DeclContext> {
   enum class ASTHierarchy : unsigned {
@@ -324,11 +333,7 @@ class alignas(1 << DeclContextAlignInBits) DeclContext
     return getContextKind() <= DeclContextKind::Last_LocalDeclContextKind;
   }
 
-  /// \returns true if this is a context with package-wide scope, e.g. a package,
-  /// a module, or a source file.
-  LLVM_READONLY
-  bool isPackageScopeContext() const; // see swift/AST/Module.h
-
+  /// \returns true if this is a package context
   LLVM_READONLY
   bool isPackageContext() const; // see swift/AST/Module.h
 
@@ -512,9 +517,11 @@ class alignas(1 << DeclContextAlignInBits) DeclContext
     return false;
   }
 
-  /// Returns the package context of the parent module.
+  /// Returns the package unit of this context.
+  /// \p lookupIfNotCurrent If the current decl context is not PackageUnit, look
+  /// it up via parent module
   LLVM_READONLY
-  PackageUnit *getParentModulePackage() const;
+  PackageUnit *getPackageContext(bool lookupIfNotCurrent = false) const;
 
   /// Returns the module context that contains this context.
   LLVM_READONLY

include/swift/AST/DiagnosticsSema.def

@@ -1679,6 +1679,7 @@ ERROR(access_control_open_bad_decl,none,
 WARNING(access_control_non_objc_open_member,none,
       "non-'@objc' %0 in extensions cannot be overridden; use 'public' instead",
       (DescriptiveDeclKind))
+ERROR(access_control_requires_package_name, none, "decl has a package access level but no -package-name was passed", ())
 
 ERROR(invalid_decl_attribute,none,
       "'%0' attribute cannot be applied to this declaration", (DeclAttribute))

include/swift/AST/Module.h

@@ -159,33 +159,48 @@ class OverlayFile;
 /// location.
 class ModuleSourceFileLocationMap;
 
-/// Package unit used to allow grouping of modules by a package name.
-/// ModuleDecl can set PackageUnit as a parent in its DeclContext
-/// See \c ModuleDecl
+/// A unit that allows grouping of modules by a package name.
+/// It's set as a property in ModuleDecl, instead of as its parent decl context,
+/// since otherwise it will break the existing decl context lookups that assume
+/// ModuleDecl as the top level context. See \c ModuleDecl
 class PackageUnit: public DeclContext {
-
+  /// Identifies this package and used for the equality check
   Identifier PackageName;
-
-  PackageUnit(Identifier name);
+  /// Weakly references ModuleDecl that points to this package.
+  /// Instead of having multiple ModuleDecls pointing to one PackageUnit, we
+  /// create and set one PackageUnit per ModuleDecl, to make it easier to look
+  /// up the module pointing to this package; this look up is needed in existing
+  /// DeclContext functions, e.g. \c DeclContext::getModuleScopeContext,
+  /// and \c DeclContext::getParentModule
+  ModuleDecl &SourceModule;
+
+  PackageUnit(Identifier name, ModuleDecl &src)
+      : DeclContext(DeclContextKind::Package, nullptr), PackageName(name),
+        SourceModule(src) {}
 
 public:
-  static PackageUnit *
-  create(Identifier name, ASTContext &ctx) {
-    return new (ctx) PackageUnit(name);
+  static PackageUnit *create(Identifier name, ModuleDecl &src,
+                             ASTContext &ctx) {
+    return new (ctx) PackageUnit(name, src);
   }
 
   static bool classof(const DeclContext *DC) {
     return DC->getContextKind() == DeclContextKind::Package;
   }
 
-  static bool classof(const PackageUnit *PU) {
-    // FIXME: add a correct check
-    return true;
-  }
+  static bool classof(const PackageUnit *PU) { return true; }
 
   Identifier getName() const {
     return PackageName;
   }
+
+  ModuleDecl &getSourceModule() { return SourceModule; }
+
+  /// Equality check via package name instead of pointer comparison.
+  /// Returns false if the name is empty.
+  bool isSamePackageAs(PackageUnit *other) {
+    return !(getName().empty()) && getName() == other->getName();
+  }
 };
 
 /// The minimum unit of compilation.
@@ -202,8 +217,10 @@ class ModuleDecl
   /// The ABI name of the module, if it differs from the module name.
   mutable Identifier ModuleABIName;
 
-  /// The name of the package this module belongs to
-  mutable Identifier PackageName;
+  /// A package this module belongs to. It's set as a property instead of a
+  /// parent decl context; otherwise it will break the existing decl context
+  /// lookups that assume ModuleDecl as the top level context.
+  PackageUnit *Package = nullptr;
 
   /// Module name to use when referenced in clients module interfaces.
   mutable Identifier ExportAsName;
@@ -313,7 +330,7 @@ class ModuleDecl
   /// Used by the debugger to bypass resilient access to fields.
   bool BypassResilience = false;
 
-  ModuleDecl(Identifier name, ASTContext &ctx, ImplicitImportInfo importInfo, PackageUnit *pkg);
+  ModuleDecl(Identifier name, ASTContext &ctx, ImplicitImportInfo importInfo);
 
 public:
   /// Creates a new module with a given \p name.
@@ -322,14 +339,13 @@ class ModuleDecl
   /// imported by each file of this module.
   static ModuleDecl *
   create(Identifier name, ASTContext &ctx,
-         ImplicitImportInfo importInfo = ImplicitImportInfo(),
-         PackageUnit *pkg = nullptr) {
-    return new (ctx) ModuleDecl(name, ctx, importInfo, pkg);
+         ImplicitImportInfo importInfo = ImplicitImportInfo()) {
+    return new (ctx) ModuleDecl(name, ctx, importInfo);
   }
 
   static ModuleDecl *
-  createMainModule(ASTContext &ctx, Identifier name, ImplicitImportInfo iinfo, PackageUnit *pkg = nullptr) {
-    auto *Mod = ModuleDecl::create(name, ctx, iinfo, pkg);
+  createMainModule(ASTContext &ctx, Identifier name, ImplicitImportInfo iinfo) {
+    auto *Mod = ModuleDecl::create(name, ctx, iinfo);
     Mod->Bits.ModuleDecl.IsMainModule = true;
     return Mod;
   }
@@ -440,18 +456,21 @@ class ModuleDecl
     ModuleABIName = name;
   }
 
-  /// Get the package name of the module
-  Identifier getPackageName() const { return PackageName; }
-
-  /// Set the name of the package this module belongs to
-  void setPackageName(Identifier name) {
-    PackageName = name;
-    // TODO: uncomment when PackageUnit gets passed to constructor
-//    PackageUnit *pkgUnit = PackageUnit::create(name, getASTContext());
-//    DeclContext newContext = DeclContext(DeclContextKind::Module, pkgUnit);
-//    setDeclContext(&newContext);
+  /// Get the package name of this module
+  /// FIXME: remove this and bump module version rdar://104723918
+  Identifier getPackageName() const {
+    if (auto pkg = getPackage())
+      return pkg->getName();
+    return Identifier();
   }
 
+  /// Get the package associated with this module
+  PackageUnit *getPackage() const { return Package; }
+
+  /// Set the package this module is associated with
+  /// FIXME: rename this with setPackage(name) rdar://104723918
+  void setPackageName(Identifier name);
+
   Identifier getExportAsName() const { return ExportAsName; }
 
   void setExportAsName(Identifier name) {
@@ -1070,7 +1089,7 @@ inline bool DeclContext::isModuleScopeContext() const {
   return isModuleContext();
 }
 
-inline bool DeclContext::isPackageScopeContext() const {
+inline bool DeclContext::isPackageContext() const {
   return ParentAndKind.getInt() == ASTHierarchy::Package;
 }
 

lib/AST/ASTVerifier.cpp

@@ -963,12 +963,6 @@ class Verifier : public ASTWalker {
       if (D->hasAccess()) {
         PrettyStackTraceDecl debugStack("verifying access", D);
         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";