Skip to content

Don't Serialize Declarations with package Access #70100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 1, 2023
Merged

Don't Serialize Declarations with package Access #70100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions include/swift/AST/Decl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2905,6 +2905,10 @@ class ValueDecl : public Decl {
/// if the base declaration is \c open, the override might have to be too.
bool hasOpenAccess(const DeclContext *useDC) const;

/// Returns whether this declaration should be treated as having the \c
/// package access level.
bool hasPackageAccess() const;

/// FIXME: This is deprecated.
bool isRecursiveValidation() const;

Expand Down
7 changes: 7 additions & 0 deletions lib/AST/Decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4203,6 +4203,13 @@ bool ValueDecl::hasOpenAccess(const DeclContext *useDC) const {
return access == AccessLevel::Open;
}

bool ValueDecl::hasPackageAccess() const {
AccessLevel access =
getAdjustedFormalAccess(this, /*useDC*/ nullptr,
/*treatUsableFromInlineAsPublic*/ false);
return access == AccessLevel::Package;
}

/// Given the formal access level for using \p VD, compute the scope where
/// \p VD may be accessed, taking \@usableFromInline, \@testable imports,
/// \@_spi imports, and enclosing access levels into account.
Expand Down
2 changes: 1 addition & 1 deletion lib/SIL/IR/SILDeclRef.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -844,7 +844,7 @@ IsSerialized_t SILDeclRef::isSerialized() const {
}

// Anything else that is not public is not serializable.
if (d->getEffectiveAccess() < AccessLevel::Public)
if (d->getEffectiveAccess() < AccessLevel::Public || d->hasPackageAccess())
return IsNotSerialized;

// Enum element constructors are serializable if the enum is
Expand Down
3 changes: 2 additions & 1 deletion lib/SIL/IR/SILWitnessTable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,8 @@ bool SILWitnessTable::conformanceIsSerialized(
if (normalConformance && normalConformance->isResilient())
return false;

if (conformance->getProtocol()->getEffectiveAccess() < AccessLevel::Public)
if (conformance->getProtocol()->getEffectiveAccess() < AccessLevel::Public ||
conformance->getProtocol()->hasPackageAccess())
return false;

auto *nominal = conformance->getDeclContext()->getSelfNominalTypeDecl();
Expand Down
4 changes: 2 additions & 2 deletions lib/SILGen/SILGenType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ class SILGenVTable : public SILVTableVisitor<SILGenVTable> {
IsSerialized_t serialized = IsNotSerialized;
auto classIsPublic = theClass->getEffectiveAccess() >= AccessLevel::Public;
// Only public, fixed-layout classes should have serialized vtables.
if (classIsPublic && !isResilient)
if (classIsPublic && !theClass->hasPackageAccess() && !isResilient)
serialized = IsSerialized;

// Finally, create the vtable.
Expand Down Expand Up @@ -1080,7 +1080,7 @@ void SILGenModule::emitNonCopyableTypeDeinitTable(NominalTypeDecl *nom) {
auto serialized = IsSerialized_t::IsNotSerialized;
bool nomIsPublic = nom->getEffectiveAccess() >= AccessLevel::Public;
// We only serialize the deinit if the type is public and not resilient.
if (nomIsPublic && !nom->isResilient())
if (nomIsPublic && !nom->hasPackageAccess() && !nom->isResilient())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible that getEffectiveAccess() == Public && nom->hasPackageAccess()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This PR #69709 changed the effective access level of package declarations to public since those declarations are visible across module boundaries.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be too disruptive to have getEffectiveAccess() return a new enum that does not include 'Package' then? Or maybe move the computation to the SIL linkage level, so that AccessLevel::Package becomes SILLinkage::Public? getEffectiveAccess() is a terrible hack for -enable-testing and we should not hang new behavior off of it. There's also a FormalLinkage concept for type metadata in IRGen, IIRC. It's all too convoluted.

serialized = IsSerialized;
SILMoveOnlyDeinit::create(f->getModule(), nom, serialized, f);
}
Expand Down