Skip to content
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.

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

[NFC] Add 'package' access modifier to AccessLevel #62652

Merged
merged 5 commits into from
Jan 20, 2023
Merged

[NFC] Add 'package' access modifier to AccessLevel #62652

merged 5 commits into from
Jan 20, 2023

Conversation

elsh
Copy link
Contributor

@elsh elsh commented Dec 16, 2022

This PR adds a new access modifier package but doesn't do anything with it yet.

Ref: swiftlang/swift-evolution#1877
The package access level will be similar to public in that it's accessible from other modules and subclassable only within the defining module as long as the modules are in the same package.
The upcoming PRs will address properly type-checking with the package access modifier.

Resolves rdar://103462581, rdar://104198440

@elsh elsh requested review from rintaro, ahoppen and bnbarham December 16, 2022 23:52
@elsh
Copy link
Contributor Author

elsh commented Dec 17, 2022

@swift-ci test

@elsh elsh changed the title Add 'package' access modifier as a contextual keyword Add 'package' access modifier to AccessLevel Jan 12, 2023
@elsh
Copy link
Contributor Author

elsh commented Jan 12, 2023

This PR contains changes in #62704, which will be rebased after it merges.

@elsh
Copy link
Contributor Author

elsh commented Jan 12, 2023

@swift-ci test

@elsh elsh requested a review from beccadax January 12, 2023 23:42
@elsh elsh force-pushed the es-pkg-acl branch 2 times, most recently from f324a65 to 169cb0f Compare January 12, 2023 23:58
@elsh
Copy link
Contributor Author

elsh commented Jan 12, 2023

@swift-ci test

@elsh elsh changed the title Add 'package' access modifier to AccessLevel [NFC] Add 'package' access modifier to AccessLevel Jan 12, 2023
@ahoppen
Copy link
Member

ahoppen commented Jan 13, 2023

Could you also add a test case in the new parser to make sure we parse it correctly there as well?

@bnbarham
Copy link
Contributor

Could you also add a test case in the new parser to make sure we parse it correctly there as well?

@elsh already did swift-syntax: swiftlang/swift-syntax#1156 and swiftlang/swift-syntax#1163

@ahoppen
Copy link
Member

ahoppen commented Jan 13, 2023

Oh, nice. I didn’t remember those PRs.

@xedin xedin removed their request for review January 14, 2023 18:43
@elsh
Copy link
Contributor Author

elsh commented Jan 20, 2023

@swift-ci test

@elsh
Copy link
Contributor Author

elsh commented Jan 20, 2023

@swift-ci test

@@ -1742,6 +1742,7 @@ SDKContext::shouldIgnore(Decl *D, const Decl* Parent) const {
case AccessLevel::Private:
case AccessLevel::FilePrivate:
return true;
case AccessLevel::Package:
case AccessLevel::Public:
case AccessLevel::Open:
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to revisit this later. I believe this logic is used by the ABI checker to report changes that would break clients that were built against an old ABI and couldn't use the new ABI at runtime. Package clients should always be version locked, the ABI checker can likely ignore package ABI breaks.

Copy link
Contributor

Choose a reason for hiding this comment

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

As is it's more restrictive, so making it more permissive later shouldn't be a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know. Will add a note to keep track.

@@ -1516,6 +1517,7 @@ SubclassScope SILDeclRef::getSubclassScope() const {
// SILModule, so we don't need to do anything.
return SubclassScope::NotApplicable;
case AccessLevel::Internal:
case AccessLevel::Package:
case AccessLevel::Public:
// If the class is internal or public, it can only be subclassed from
// the same AST Module, but possibly a different SILModule.
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole function is interesting for what we discussed about packageopen.

@elsh elsh merged commit 91f7854 into main Jan 20, 2023
@elsh elsh deleted the es-pkg-acl branch January 20, 2023 21:58
@nkcsgexi
Copy link
Contributor

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants