[AccessScope] Treat PackageUnit as enclosing scope of ModuleDecl

[elsh]

• Create PackageUnit when package-name input is non-nil as a property of ModuleDecl
• Treat PackageUnit as the enclosing scope of ModuleDecl
• Create an access scope with PackageUnit as context for the package access level
• Add a PackageUnit context check in access scope functions
• Return Module Decl from PackageUnit context (weakly referenced) in DeclContext::getModuleScopeContext and DeclContext::getParentModule to avoid breaking existing code
• Add doc comments and fix tests

Resolves rdar://104987295, rdar://105187216, rdar://104723918

[elsh]

@swift-ci smoke test

[beccadax]

This is…pretty weird. Wouldn’t we prefer to have one instance of PackageUnit shared by all of the ModuleDecls in the same package? That way you could == the DeclContext pointers to see if they match, just like other DeclContexts.

[elsh]

I found this way easier to look up the module from a package unit in DeclContext::getModuleScopeContext and DeclContext::getParentModule; otherwise will have to compare ModuleDecl pointers in an array

[beccadax]

That kind of just moves the question. Why do getParentModule() and getModuleScopeContext() return the module when you call them on the package? That seems, if anything, backwards from what you’d want. (And yes, I know you’re trying to avoid making the package a parent of the module, but making it a child instead seems pretty strange.)

[elsh]

The existing code assumes ModuleDecl is the top level context as long as there's non-null DeclContext (otherwise public) and performs isa/cast without checking if it's null, which is everywhere. With a package context now introduced, those existing functions are still called on the target DeclContext that may or may not be package, and will crash on isa/cast if the returned context is not module. Either we'll have to add assert or null check before casting or add package specific handling before each; by returning the module mapped to a package as in this PR, the changes required are minimal.

[elsh]

Also added doc comments in AccessScope, DeclContext, PackageUnit, ModuleDecl with more info.

[elsh]

I actually tried other PackageUnit-ModuleDecl mappings first but there were issues with each as follows.

1. Set PackageUnit as ModuleDecl's parent context (most clean, straightforward hierarchy)
   • Can only pass PackageUnit to ModuleDecl's ctor directly for the main module; for loaded modules, a PackageUnit needs to be created and set as a new DeclContext.
   • setDeclContext does not seem to actually override the Context field in DeclContext once set (not sure why), thus looking up the new context returns garbage.
   • Still need a way to look up ModuleDecl from PackageUnit for the existing module getters in DeclContext to avoid crashes as mentioned in the previous comment.
2. Create a PackageUnit-ModuleDecl map in ASTContext (reasonable, no circular reference b/t the two classes)
   • Adding the entry to the map when PackageUnit is created from either class results in calling DeclContext::getASTContext() which calls DeclContext::getParentModule(), which then calls DeclContext::getASTContext(), resulting in a cycle.

Thus, resorted to having PackageUnit as a property in ModuleDecl and have a weak reference back to ModuleDecl from PackageUnit. Lmk if if the way it references is not actually a weak reference though.

[elsh]

@swift-ci smoke test

[elsh]

@swift-ci smoke test

[elsh]

@swift-ci smoke test

[xymus]

This looks like a forgotten long line.

[elsh]

@xymus Thanks for the review. Your comments are addressed in #64373.