SE-0409: Access-level modifiers on import declarations

Hello Swift community,

The review of SE-0409: Access-level modifiers on import declarations begins now and runs through September 26th, 2023.

Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to me as the review manager via the forum messaging feature or email. When contacting the review manager directly, please put "SE-0409" in the subject line.

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  • What is your evaluation of the proposal?
  • Is the problem being addressed significant enough to warrant a change to Swift?
  • Does this proposal fit well with the feel and direction of Swift?
  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

More information about the Swift evolution process is available at

https://github.com/apple/swift-evolution/blob/main/process.md

Thank you,

Frederick Kellison-Linn
Review Manager

33 Likes

I’m super happy to see this cleanup!

+1, seems sorely needed from our experience and this pitch is inline with expectations.

I’d probably give a vote for either considering adding an additional modifier ‘exported’ (“fused”?!) or using “open” as suggested to also eliminate @_exported, but can see it as a future direction too of course - would just be nice to “finalize” this aspect of the language while doing this.

Also, could you please add to the rationale “why”? This is needed:

  1. All dependencies of a non-resilient module must be loaded by transitive clients.

The other cases outlined some reasoning on why - I assume this is true as a non resilient module would be allowed to change import visibility? But would be nice to be explicit.

This specific rule is a bit unfortunate from the perspective of optimization as it’s quite common - not sure if anything could be done.

I performed a read through of the pitch and have used the existing underscored attributes.

3 Likes

Does this work with imported individual entities or only entire modules? Can I do public import struct Foo.Bar?

About using open as a marker for re-export: I think even public is sufficient here. I would expect that doing public import re-exports open classes as public, while open import re-exports everything as is, without any access restrictions

Overall, I think this is a great and necessary improvement, and it's an important step toward letting developers with large numbers of modules shed dependencies (even if the non-resilient cases need additional work before dependency trimming can be fully realized there).

I also think it's correct to not try to tackle @_exported here. @_exported import is orthogonal to access-level restrictions, so open is (correctly pointed out) not the correct spelling for that. @_exported import is not something that should be easy to reach for. It has valid use cases (preserving source compatibility when moving declarations between modules, creating a convenience "umbrella" module for packages broken down into smaller modules), but we should give more thought to how we want re-exporting to work as a language feature. We already inherit too much of Clang's re-export-the-universe behavior for Objective-C modules, so we should consider the desired behavior for native Swift modules more deliberately.

Aside from that, I have a few questions:

  • I'd like to understand better how access level imports interact with @testable; the proposal touches on this briefly in this section, but a concrete example would be more helpful. More specifically, what would be the behavior of @testable public import X? Should we simply ban that (@testable would only be allowed on internal and lower imports) in Swift 6?

  • swiftc has an -emit-imported-modules flag that produces a dump of the modules imported by the module being compiled, as a simple text file with one module per line. How does this feature interact with that? It would be helpful if that mode could provide additional information about modules, such as the highest access level they are imported under across the compiled sources.

  • Similarly, will this information be surfaced by indexstore? When iterating over the dependencies of a record, if the dependency represents a module it should reflect the access level that was used to import it.

3 Likes

A dependency private to this source file is declared with either the fileprivate or the private modifier. In both cases the access is scoped to the source file declaring the import.

fileprivate import DependencyPrivateToThisFile 
private import OtherDependencyPrivateToThisFile

Is it necessary to support both private and fileprivate if they have the exact same behavior? Could we pick just one to allow here?

8 Likes

I fear there's going to be a bit of confusion about the meaning of those access modifiers. The access levels on declarations currently relate to the visibility of the declaration. If you declare an internal type for instance, the type becomes available anywhere in the module. Wouldn't an internal import makes the imported declarations available anywhere in the module too? This is obviously not what is proposed here.

What is proposed here is not an access modifier for what's imported as it doesn't change the visibility of the imported symbols, it's a maximum-allowed-access modifier for declarations making use of what's imported. I think it's a valuable feature to have, but I fear this isn't what the syntax evokes.

This proposal avoids touching @_exported, but at the same time it takes the spelling public import which is a very natural spelling for @_exported. Some precedents: D's public import and Rust's pub use are equivalent to @_exported. If public import is to have a different meaning in Swift, should't it be justified in some way? At the very least it would be wise to figure out which spelling we want for @_exported before taking away public import.

9 Likes

This already exists in Swift today, because the same case could be made for any fileprivate or private declaration at file scope. Unfortunately, I think that ship has sailed and having the inconsistency for imports would put us in a worse state than allowing the redundancy.

This concern was raised in the pitch thread as well, but to reiterate what I said there, I don't think there's a world where it's desirable to have imports from one file affect type checking in other files in the same module. That would make it very difficult to reason about the contents of a file on its own and would also be harmful to incremental compilation.

I think the proposed spelling is both concise and clear ("if you import M with access level L, then you can use M's types in the signatures of your own L or lower declarations") and I don't think we'd gain clarity by making it more verbose. public import will be very common in Swift 6 when using imported APIs as part of your own, and I'm not convinced that the use of pub(lic) by other languages to mean @_exported is evidence enough that we should use public import for that in Swift and have to come up with another, more complicated way to spell the proposed public import.

2 Likes

Yes, thank you! This has been corrected.

For example, when type-checking a source file with an internal import SomeModule , we consider all declarations imported from SomeModule to have an access level of internal in the context of the file. In this case, type-checking will enforce that declarations imported as internal are only referenced from internal or lower declaration signatures and in regular function bodies. They cannot appear in public declaration signatures, @usableFromInline declaration signatures, or inlinable code.

There are situations where you want SomeModule to be prohibited from appearing in a module's public interface, but you still want to use it as an implementation detail in inlinable function bodies and data structures.

I think it would be useful to allow @usableFromInline to be attached to these internal imports - i.e:

@usableFromInline internal import SomeModule

If I understand the proposal correctly, the only way to use another module in an inlinable function or data structure is to make the import public, which removes all interface-level diagnostics. Looking at the issues mentioned in the motivation section:

The author of a library may have a different intent for each of the library dependencies; some are expected to be known to the library clients while other are for implementation details internal to the package, module, or source file. Without a way to enforce the intended access level of dependencies it is easy to make a mistake and expose a dependency of the library to the library clients by referencing it from a public declaration even if it's intended to remain an implementation detail.

These interface-level diagnostics appear to me to be equally valuable, regardless of whether the function body or data members can be inlined.

2 Likes

Question #1

I’d naïvely expect this to show up in a package manifest, not in source code: a library exposing its dependencies to clients is module-level information, so it would make sense for it to appear in Package.swift. I didn’t see any discuss of this in the proposal. (Apologies if I missed it.) What’s the rationale?

I imagine it’s necessary for type-checking references to imported modules…? It does make sense to check API access on a file-by-file basis.

If that’s correct, is there any interaction between access modifiers on imports in source code and how dependencies are declared in Package.swift? Any possibility for creating an access level mismatch between the manifest and the file-level imports?


Question #2

The proposal says:

At the module level we only take into account the most permissive access level.

I assume, however, that at the file level, only the import within that file applies. For example, if Foo.swift declares private import Dependency but Bar.swift in the same module declares public import Dependency, then is Dependency still type-checked as private for declarations within Foo even though it is a public dependency of the module as a whole?

3 Likes

I'll look into adding a short explanation on that bullet point and have it point to the future directions for more context.

Was the description of the problem in the future directions section clear enough in your opinion? I've investigated possible solutions so I could provide more thoughts about it here if there's interest.

The access-level is applied to the whole module, not the individual declaration.

Absolutely, a forward-reference would suffice for sure. Thanks!

And of course if you have thoughts on solutions to share that’s be interesting- it would be a great future direction, we’d surely benefit from it :-)

I suppose a more precise name would be something along the lines of @usableFromInline -- as in, a @usableFromPublic import, @usableFromInternal import, etc.

But while it is very, very clear, it's also pretty awkward.

I'm not such a big fan of public import for either of those things, really.

The reason is that importing and exporting are different -- importing relates to what the code uses, and exporting relates to what the code provides to other code. Access control is generally about what is exported, so applying it before the term "import" feels incongruous.

3 Likes

@testable is already more powerful than public. It promotes internal and package declarations from the imported module to public. The access-level then enforces an upper bound on this new level.

I would think that a public testable import is generally a bad practice, but there are use cases for it and I'm not aware of it causing any problems. It is currently very common as it's the only possible configuration.

This mode should still print all local imports. Extending it makes sense to me and should be a simple change. It really depends on the tools using this information as it would likely be a breaking change to the format.

At this time, improvements like this may be preferable to do in the dependency scanner as it provides a wider set of tools to inspect dependencies. Using the flags -scan-dependencies -import-prescan outputs similar information in JSON which could also provide categories of imports.

Do you have a specific use case for that information?

This information is not planned to be surfaced by the index. Mostly because the index doesn't track access levels in general.

Once again I'd be curious to hear if you have a use case in mind.

I think it would be a good improvement for the package manifest to have a way to declare which dependencies should be public or hidden. The compiler could then report any mismatch. It may be more relevant once the package manager supports library-evolution. Until then, in packages this feature is mostly relevant for the per file type-checking.

That's right. Type-checking only takes into account access-level modifiers on the imports from the local file.

3 Likes

What are some example use cases? Using @testable to access internal declarations from within the bodies of test cases makes sense, but I'm struggling to think of an example where we'd want to let @testable imported declarations to then be accessible from the public APIs of the modules doing that import. If the use cases involve test support modules used across multiple tests, why would we want to allow people to do @testable public import instead of taking advantage of the new package visibility?

In Bazel, we use -emit-imported-modules as part of a layering check to diagnose imports of modules that are transitively loadable but not directly depended on by the build target. Separately, we distinguish between deps (dependencies that must be transitively loadable) and private_deps (dependencies that can be trimmed; the naming was established long before this proposal), so being able to distinguish the access level of imports would let us provide better diagnostics and automated dependency management.

I'll investigate whether -scan-dependencies would also work for us, since it has the more flexible output format. Of course, I'd rather upstream the layering check support directly into the frontend one day (flagging modules in the explicit JSON module map as "can be imported" vs. "just here because one of the others needs to load it"), so maybe I should focus my effort there.

Mainly just to have more information available in one place for semantic analysis; indexstore has proven to be a good database for doing whole-file analysis vs. making repeated SourceKit requests. But you're right that it doesn't track other access levels right now, so maybe that's something that should be added holistically instead.

+2

A feature I miss almost every single day of my life, thank you for making this happen :heart_eyes:

2 Likes

I have seen test utility libraries for large projects to centralize testing tools between different test targets. It's not a practice that I would recommend but it can work. I would certainly encourage the use of the package access level for such a use case over @testable.

The main reason why we support hiding dependencies only in resilient modules is because they move a lot of the required information from build time to run time. For non-resilient modules, we need all the information at build time as there's no service at run time providing it. So, for now, we need to load all transitive dependencies for non-resilient modules.

For example, given a struct holding a property of an imported type, the struct memory layout depends on the memory layout defined in the other module. When the module defining the struct is resilient, clients instantiating that struct will query the memory size to allocate at run time. Whereas a non-resilient module will require this information to already be available to the client as it's being built.

There are possible solutions to this:

  • There are scenarios where hiding the dependency from a non-resilient module currently mostly work. We could limit references to that dependency to only supported cases at type-checking. It would basically allow references only from function bodies and reject all uses from struct properties. This can be restrictive, while it could be useful to enable on dependencies impacting build times significantly, we don't want it enabled for all non-public imports automatically.
  • Alternatively, we could copy over the required information between modules. This would require further investigation to cover everything, and it would likely limit some of the optimizations we can perform. This is a feature on its own that would likely require a stabilization period since it impacts widely the internals of the compiler and different language features.
2 Likes

But import statements are not the same because at least when applying private/fileprivate to a declaration there can be a difference when not at file scope.

3 Likes

+1 here - I’m glad to see this ‘@_implementationonly’ being formalised. We used it extensively in a previous project to prevent the leakage of 3rd party dependence’s throughout the codebase and it worked brilliantly.