-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Adopt MemberImportVisibility #8525
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
Adopt MemberImportVisibility #8525
Conversation
5bc35e0
to
9820f44
Compare
9820f44
to
a96a733
Compare
@swift-ci test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Can I ask you to factor the enableExperimentalFeature("MemberImportVisibility")
statements into a common swiftSettings array at the top of the package, though, as in Swift Build?
We'll want to add more of these over time, and it's much easier if we can apply to the whole package at once without having to copy-paste 20 times.
Looks like there is a compile failure on macOS when adopting this feature on 6.0.3. Bisected it down to adding |
…ity on targets that fail to compile with 6.0.3
@swift-ci please test windows platform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me, and this change can make unexpected usages much more apparent moving forward.
I've added a question and one other comment, nothing blocking.
@swift-ci please test windows |
@swift-ci test windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I very much favour this change to make import more explicit.
My primary concern is posted as a comment in Package.swift
.
@@ -152,7 +165,7 @@ let package = Package( | |||
.target( | |||
name: "PackageDescription", | |||
exclude: ["CMakeLists.txt"], | |||
swiftSettings: [ | |||
swiftSettings: commonExperimentalFeatures + [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: should we be using swift6CompatibleExperimentalFeatures
instead of commonExperimentalFeatures
in the event a developer is using Swift 6.0.x to build the project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want these to apply to any toolchain. The swift6CompatibleExperimentalFeatures
var only exists for targets that currently fail to build with MemberImportVisibility
on in 6.0.3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we understand why those targets fail to build on 6.0.3? Can we raise an issue to ensure we get this addressed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They don't fail on 6.1, so my assumption is it's already fixed.
Adopt the experimental feature MemberImportVisibility.
Motivation:
Due to a weakness in the existing Swift language implementation, a public extension declared in a library module can be accessible to source code that doesn’t directly import that module.
Modifications:
Adopt MemberImportVisibility on all targets and adjust imports to be explicit. There were a few places where ambiguities were exposed. The most apparent one is the ambiguity between Basics.AbsolutePath and TSCBasics.AbsolutePath. In this case they're functionally equivalent since the Basics version just type aliases the TSCBasics version. There were a few others where identically named methods were defined in both a SwiftPM module and an imported dependency.
Result:
SwiftPM compiles with no code included without a corresponding explicit import.