[Pre-Pitch] Import access control: a modest proposal

Do we need special keywords here? Can the compiler inspect the API and automatically export those modules exposed publicly and keep those implementation-only which are not?

For my uses this inverts the ordering I want. I'd like to tell the compiler what effect I'd like to achieve (don't make this module part of my API/ABI) and have it police that effect, instead of having the compiler observe the effect I actually caused (forgetting that storing type Foo on my @frozen data type makes it part of my ABI) and assuming that's what I meant.

8 Likes

Reusing the access control markers in some form on import makes sense to me. However, the default behavior of import today is also the "wrong" one from the perspective of trying to minimize implicit API liabilities. Could we conceivably change the default behavior of import in a future language mode? One way we could potentially phase this in could be to start warning about import statements that don't specify a visibility in Swift 6, encouraging existing code to audit the visibility of their imports, and then changing the default in Swift 7 so that imports are implementation-only by default.

17 Likes

It's great to see some efforts toward better imports !

One additional issue concerning imports I've been fighting is the order in which types and modules are resolved at use site.

ex:
Module X contains a type named Profile
Module Profile contains type named A

in module Y :

import Profile
import struct X.Profile

let a = Profile.A() // doesn't work (I want to use the module Profile but swift select the type Profile)

A fix is to create another file and use a typealias for the type but it's not very convenient.

I think it would be nice to be able to alias imports thought just changing the order in which types are resolved would make it work too.

import struct Profile as ProfileType
1 Like

Isn't this what the pitch explicitly suggests (albeit with an even more aggressive timeline), or have I misunderstood?

3 Likes

Ah, so it does. I do wonder whether that's too aggressive a timeline, though; giving people a language version to transition would be nice.

1 Like

Yeah, finding some way to warn about potential issues first would be nice, but IMO the "warning about import statements that don't specify a visibility" strategy is also too aggressive—even moreso than just changing the default in Swift 6. That would mean users would have to update every import statement, even if they really do want the internal import default (and it would require even more work in the Swift 7 update if users wanted to go back to unannotated import where they do want the internal default).

4 Likes

I think calling it a strict quality of life improvement for users of @_implementationOnly import is even understating the benefits a bit. Separate from but related to the API leakage being discussed here, making implementation-only the default import means build systems can change their default behavior to no longer pass the entire transitive closure of a module's Swift and Clang dependencies to compile actions, which will be a significant win for build times of large projects.

Long Swift build times are something we've been struggling with for a while at Google (we're starting to overcome this as we've been enabling more use of explicit modules for Objective-C dependencies), so I'm thrilled to see this being pitched since the language defaults for dependencies should nudge users toward toward the most efficient and least "infectious" behavior—even when there's a migration cost associated with flipping it on.

@beccadax: Aside from the compiler changes, how would Swift Package Manager handle the distinction between kinds of imports? For example, with Bazel, the user has to be explicit when constructing the build graph about which dependencies are regular imports (whose .swiftmodules, .modulemaps, headers, etc. need to be propagated to dependents) and which are implementation-only import (whose artifacts don't need to be propagated further), so we have separate deps and private_deps lists for those, respectively. Do you envision SPM extending targets to have separate dependencies and publicDependencies lists to make similar optimizations to its compiler invocations?

6 Likes

Unless I'm missing something, that's not the case at all. It'd be only authors of libraries (not apps), and--the pitch is not clear on this, so some clarification is in order perhaps--presumably having just one public import per module being imported is sufficient even if other files in the same library import the module only internally.

4 Likes

Right, I was taking for granted that this was all in the context of library authors, but worth calling out.

Yeah, good point. For public imports, one annotation should be sufficient. For internal imports though, users would have to annotate all imports in order to silence the warnings, which seems sub-optimal.

What would be the rationale for there to be a warning for internal imports spelled import? That should not be a thing; it is certainly not required for source migration.

Perhaps I'm misunderstanding the suggestion, but I thought that's what this portion of Joe's phased rollout would entail:

Ah I see; I would discourage that from being part of the proposal for the reason above. It actually makes migration more problematic and it’s pretty unprecedented to warn on completely valid code that we expect people to be able to write both before and after a migration.

2 Likes

That's why I had suggested only introducing the warning as part of moving to a new language version. That way, existing code continues to work without noise, but when you opt into upgrading language version, and your code has to be updated anyway, then you get warnings to update your imports.

2 Likes

Do we have hypotheses about what proportion of bare imports today should be internal imports versus public imports? If we expect there to be a nontrivial number of bare imports that really should have been internal/@_implementationOnly, it still seems like quite the burden to me to have users to add internal to every one, especially when they'll probably just want to take them away in the next language version anyway.

4 Likes

I concur that the only reasonable/pragmatic default is that "bare" imports should imply internal/@_implementationOnly even though that's a breaking change -- but I disagree that we should ever warn on unspecified/bare imports. All App Code doesn't care about the import specifiers, as I understand it, so this would be boilerplate syntax for App Devs -- and boilerplate syntax for the default case in libraries.

Instead, when upgrading from Swift 5 -> Swift 6 the migrator should have a special tool to help library authors detect what extra Library types are being exposed by their library target, and potentially add an Exports.swift file such that their library package can continue to produce the same results that they had before this syntax upgrade arrived.

The advantage of this is a one-time operation during migration that quickly fixes the project, and gives devs clarity, along with a place where they can incrementally clobber unexpected public imports instead of having to do it all at once. This piece of the migrator might actually be useful in general for library authors to understand what is exposed by their library. -- If I have a library package with 20-leaf files, and every one of the leaf files can expose some other library via public import, that's a real bear to manage without some additional CLI or GUI tool for auditing.

8 Likes

I would find private import useful as well. For example, I may want to wrap some functionality to offer some subset of it without exposing it to my entire module.

I ran into this in one of my projects the other day. I have a more project specific wrapper for particular functionality, but the underlying package is leaking into the full module and I have to remember which one to choose...

3 Likes

I’m very glad to see this picked up, and I’m also glad to see everyone sharing their use cases; that’s exactly what should go into design considerations. I like @beccadax‘s plan a lot, at the very least as a place to start from. Some additional thoughts:

  • I’d be really happy to see “implementation-only” become the default in a new language mode, for exactly the reasons stated by @allevato: Swift tries to make it explicit whenever a library author exposes something to their clients, so they don’t end up having to support an implementation detail as public API. When I was involved in the early @_implementationOnly discussions changing the default was very appealing, but would have broken every package out there that expects import Foundation to mean “I can use Data in my public APIs”. Tying it to a language mode gets around that.

  • The semantics don’t quite match up with access control, though. Everywhere else, access control governs visibility; in that model, a private (= fileprivate) import is what we have as the default today (ignoring the extensions-and-operators behavior), an internal import would make a module visible to all files in the target (which has been requested), and a public import would be visible outside of the target as well. Now, we don’t have to follow this strictly, but it has the interesting property that modules always re-export the dependencies of their public APIs. Which is how C-based languages behave, but pretty much no one else, at least not as a requirement.

  • But imports do more than visibility; they’re also adding dependencies, either compile-time or link-time. Let’s set aside the link-time dependencies for now; as long as they’re not transitive (part of the implementation-only protections) they don’t have to affect the compilation model. Our current import thus means “I’m exposing a compile-time dependency on this module, and even making its types available in my public API”. If those types do appear in your public API, though, you’re exposing them to clients as well. As an example, if you return a Foundation.Data from one of your methods, the caller can immediately ask if the Data isEmpty without importing Foundation themselves. Is this a good model? Not sure…but it ties into the extension leakage problem. (Though you could say cross-module extensions are different.)

    The explanation I had at the time was that imports just control visibility; there’s nothing to stop the client from adding their own import Foundation, so there’s no reason to force them to write it either. But it is still kind of weird.

    Anyway, the point I’m trying to make is that the current behavior of import is public in some ways (you can rely on it in public APIs) but not others (it does not itself expose anything to other modules). We could say that’s fine and call the two modes public and open, but it still feels a little funny to me. I don’t have a better spelling, though.

  • That said, I also think this would be a good time to fix the extension and operator leakage. It’ll be annoying but the compiler has the right tools for it now. One thing to be careful of there, though, is how that behavior changes when crossing language modes (e.g. a Swift 6 client of a Swift 5 library and vice versa).

  • @usableFromInline import becomes potentially interesting as a compile-time dependency that you still don’t want to leak into your public API. It probably doesn’t need to be fully nailed down or implemented right now but it’s good to leave space for it, just like @xymus’s @_spiOnly public import.

10 Likes

Why should they want to do that? When in a later language version import and internal import are doing the exact same thing, then it really doesn't matter if they have one or the other. And if it really matters (maybe because of coding style guidelines), it really isn't more than a simple regex replace to change all internal imports to imports again.

Correct—this change would be at compile-time. You’d need a newer compiler at your desk, but not a newer device in your users’ hands.

Probably, but I don’t think we’d actually want the compiler to infer public import any more than we’d want it to infer public class or public struct when you used a type in a public API. Your dependencies are an externally visible aspect of your module; explicitly controlling that aspect is a feature, not a bug.

Huh, that could actually be useful. I think it might be better as a separate proposal, though.

I am very much thinking about solutions to this problem, but it’s a separate issue from what we’re discussing in this thread.

One of the reasons I think we can get away with this change is that in app targets, you almost never need to write public import.* If Swift 6 mode warns about un-annotated imports, the proposal changes from “some imports, mostly in libraries, need to be marked with an additional keyword” to “every import in every file needs to be marked with an additional keyword”, and given how long it took to get from Swift 5 to Swift 6, waiting until Swift 7 to finish the transition could mean we spend several years in that state. I think that might tip this change from a minor inconvenience to a major hassle.

* It actually just occurred to me that even in app targets, retroactively conforming types you don’t own to protocols you don’t own would require a public import of both modules. This is arguably a feature.

My back-of-the-envelope hypothesis is that more than half of imports in libraries will need to become public imports, but very few imports in executables or test suites will need to change. This is not based on any data; I should probably instrument the compiler in a branch (e.g. repurpose the existing @_implementationOnly checking to emit a remark on normal imports that would need to become public) and then build the Source Compatibility Suite to get some numbers.

This is a really good question that I haven’t even thought about.

This is true. I guess my mental model is that <access-level> import M doesn’t mean “make this import visible at <access-level>; it means “import public declarations from M into this file as though they have <access-level> or less”. That is, (absent @exported) import is always inherently a per-file thing, and the access control keyword is being transitively applied to what’s being imported. (Although that gets a little weird with open.)

Incidentally, I learned recently that @hamishknight at least partially fixed the operator visibility logic last year, but there’s some compatibility logic to retain the old behavior and a feature flag to disable it.

8 Likes