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 the review manager by email. When contacting the review manager directly, please keep the proposal link at the top of the message and put "SE-0386" 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:
-1. I disagree with the characterization of spi as described in the proposal. spienables the sort of secret handshake style library/client communication described, but just as equally it allows for delineating between groups of intended clients. You can write @spi(ForJohnsEyesOnly) as a special in-the-know affordance, or write @spi(GameAuthors) for the use case outlined in the proposal—there is no intrinsic bias towards one or the other.
The proposal implies that naming the desired spi at every import site would be onerous, but I feel the opposite is worse: maintaining a mental model of every possible package level import, without explicit indication, as teammates (or even peer teams) update the package manifest over the lifespan of a project sounds like a nightmare. I have been on projects where I've accidentally called generic-sounding functions only to find out later they came from an unexpected library. Sometimes this is fine and only imposes an issue if/when it comes time to remove the library; but sometimes it causes difficult to track down bugs because the functions only properly handle the cases expected (or enforced) by their containing library, despite their general-seeming names.
On the flip side if I'm on the engine sub-team I want to know with some degree of certainty that only the game sub-team is calling run(), and not the start screen sub-team. If I am publishing the engine module publicly (say as open source) it should still only be the case that a game module can call run(), and not arbitrary other modules that compose the full project. If my engine is sufficiently capable I can even publish, e.g., @spi(2DGame) and @spi(3DGame) which both solves the original problem of the app package having access to semi-internal API, and also prevents game authors from constructing an invalid engine or calling nonsensical methods (this utility being a good idea in the specific context of a game engine notwithstanding—I have done analogous things before).
Generally I'm not against it. It feels like a semi light extension that would make some inconveniences in package development fade. However I‘m too glad to see that there's no packageopen and I really would never ever personally want to see such access modifier added, fileprivate is already enough. If anything it would be great if all access modifiers could eventually be overhauled for a major release (e.g. making private type-private by default, introducing light opt-in constraints, and fixing open in context of protocols to introduce open and closed protocols). Besides that I think package access modifier is more like a patch for the lack of two other missing features, sub-modules and in-line unit testing. Having the ability to expose visibility to sibling targets in the package is still a welcome addition at this moment of time.
The package identity section of the proposal appears inaccurate, as though parts were copied and pasted from SE‐0292 without understanding how the rest of SwiftPM works.
The simplest would be for SwiftPM to pass its own internal identities directly (which are much broader than the subset described in SE‐0292 for registry requests) and for there to be no restrictions. To the compiler, an identity would be whatever arbitrary sequence of bytes were supplied to the command line.
Otherwise the proposal needs to explain an array of other situations already supported by SwiftPM outside the world of registry downloads. For example, what happens when SwiftPM attempts to build something depending on a local repository named Πακέτο, located at /Users/Εγώ/Πράγματα/Πακέτο and referred to using .package(url: "file:///Users/Εγώ/Πράγματα/Πακέτο", from: Version(1, 0, 0)) and .product(name: "Προϊόν", package: "Πακέτο"). Currently such a package is valid.
As a package author, I feel like if we had “package-private modules” (as described as a possible future direction) I could get much of what the motivation section asks for, without my actually using the new visibility modifier at all.
Were package-private modules considered as an alternative approach, leaving the visibility modifier aside entirely?
(I haven’t followed the pitch thread, so my apologies if this is old hat.)
-1. This proposal solves the issue with an access control cutoff that becomes infinitely cyclical.
If we want to restrict an API to a set of modules called a "package", logically we can now view that package itself as a set of modules within which we want to restrict an API, and so we're right back at the initial problem statement, with infinite subdivisions. This proposal just addresses one level, so it's an incomplete solution.
If -package-name is not given, the package access modifier is disallowed.
Does this mean every module should implicitly be a part of a package of one? Otherwise re-organizing code around will require deleting package keywords.
This was pointed out in the proposal thread, adding package will forever be source breaking (like fileprivate), whereas starting off with internal access for packages and re-assessing if in practice a new keyword is necessary is a less risky approach.
Can you clarify how exactly these subdivisions could be infinite? An API can already be restricted within a module with internal, and within a single file with private or fileprivate. What other subdivisions would one ever be interested in other than these?
Not every module, but only those that use package, if I understand your question correctly. Moving code around would require deleting package keywords only when such code is moved outside of package, say to a single-file script, which in practice is quite rare. Off the top of my head I can't say I've done it at least once in my career, and I've been writing in Swift as my primary language almost immediately after it became available.
Can you elaborate how exactly this would be source breaking?
Agree that this seems less-than-ideal. I’m not sure I see a strong motivation for making this an error — it doesn’t seem actively harmful, just redundant / unnecessary. Maybe it could just be a warning instead?
I understand that, I was making the point that this proposal allows for a division into packages one level deep, but doesn't solve the general case of subdividing modules into very precise access restrictions.
I also understand this proposal does not suggest reverting a keyword. I was trying to state that by introducing package immediately, we may be introducing another fileprivate scenario which we cannot seemingly revert.
I went through the proposal text, and sorrily didn’t see package’s advantage over SPI except for a shorter syntax. Most “bonus”es from the pitch can simply apply 1:1 to the SPI world.
The biggest problem of this pitch, IMO, is that it assumes an arbitrary package context for Swift codes, which is absolutely wrong because there’re plenty of Swift codes in the form of standalone script files, CMake(-compatible) projects, Xcode projects, standalone XCTest suites, etc. All of them are not necessarily (for some, almost never) “package”s. I really don’t like the idea of “introducing a keyword in the Swift language, but for SwiftPM packages only”.
What somehow makes things worse is, as @8675309 has pointed out, splitting giant packages (projects) is a best practice in software engineering, but this pitch makes it way harder to do that while SPI just works smoothly.
Below are some other nits of the proposal, which just didn’t change the overall -1 I would give:
After this pitch, the “exported interface” is divided into two layers: public and package, which could have different module dependencies. That means in order to maintain the package-level interface, a target may rely on (and even accidentally expose!) some modules that are totally unused in public APIs, and vice versa.
Also, try to consider “qualified imports” — what if we want to explicitly import only public APIs inside the package for some purpose (eg. providing an example/template target for users)?
@elsh, can you comment on the intended behavior here? It would be good to understand sooner rather than later if the proposal needs to be updated to accommodate the full range of package identities support by SwiftPM. Alternatively, if the intent is that SwiftPM will only enable package for SwiftPM packages that conform to the schema described in SE-0292, the proposal should make that clear so that we can get adequate feedback on the idea.
From a compiler perspective, I have a couple comments:
Treating what's clearly readable text as simply an arbitrary sequence of bytes seems sketchy from a Unicode correctness standpoint. Some filesystems do this for long-term stability reasons, but it seems harder to justify here. I suppose the assumption is that the build system will pass down a stable canonicalization of the string?
I don't know what SwiftPM's "internal identities" are, but (1) passing them to the compiler makes them not purely "internal" anymore, so they would have to be rigidly defined and stably encoded, and (2) future directions like automatically namespacing package-private modules by package name would definitely prefer that the string be relatively stable and compact, rather than including a lot of local paths and other URL fluff.
Names including invalid c99 identifier characters are currently encoded using the rules specified at spm_mangledToC99ExtendedIdentifier. Product and target names are converted to a valid c99 identifier with those rules, as seen here; "Προϊόν" is converted to "Προι_ο_ν".
We will allow the full range of package identities supported by SwiftPM. Will update the proposal accordingly.
The proposal is not arguing that SPI is an unnecessary concept that shouldn't be added to the language; it's just arguing that SPI is not the right tool in every situation. The SPI feature is designed around promoting a tight coupling between a particular interface and its exact expected clients. Sometimes you want that, and in those cases, you should use SPI. However, sometimes you don't want that, and all you want to say is "this interface is just for us, at least for now". Swift's core access control design is based around recognizing existing boundaries that often correspond to a useful sense of "us": the people implementing this specific declaration, or working in this file, or working on this module. In that light, allowing users to recognize a boundary that's broader than a single module but not as broad as the entire program makes sense, because small teams often work on several closely-related modules and should be able to share code without having to make it part of the public API. Different organizations can draw that boundary where they want, but it makes sense for SwiftPM to default to drawing it at the package boundary, and package seems like a good general name for the feature.
It would certainly be more flexible to allow these boundaries to be explicitly named and separately controlled. If you want that, you can use the SPI feature for it. But personally, I think that would not be a good idea. It really needs to be notable for a file to contain an SPI import: seeing one should make programmers and code reviewers immediately question whether it's really needed. That idea is badly undermined if SPI imports are common; if every file contains two or three boilerplate SPI imports just to get access to run-of-the-mill cross-module interfaces, it becomes really easy to miss that one of them is a "true" cross-package SPI usage that should get special attention. In my mind, the only way to avoid that and preserve the value of programmer discipline and code review is to design access control the way Swift does, around ever-larger natural boundaries that programmers intuitively understand. And ultimately, access control within an organization only works because of programmer discipline and code review, because otherwise programmers will just make things public to shut the compiler up.
I don't understand what this conversion is doing. Προι_ο_ν is not a "C99 identifier" in the usual sense of an identifier that is required to be accepted by conforming C99 implementations. Is there a misunderstanding here about what "C99 identifier" means?