Should Algorithm's public structs be @frozen?

We distribute an SDK which depends in the Charts library as an external dependency, recent versions of which now depend on Algorithms. Our build process for the SDK archive leverages Carthage, with us setting BUILD_LIBRARY_FOR_DISTRIBUTION to YES as one of the build settings to Carthage.

However, with the recent versions of Charts that now depend on Algorithms, setting BUILD_LIBRARY_FOR_DISTRIBUTION results in the following build errors:

/Users/djs/Library/Caches/org.carthage.CarthageKit/DerivedData/14.0_14A309/Charts/v4.1.0/SourcePackages/checkouts/swift-algorithms/Sources/Algorithms/AdjacentPairs.swift:66:10: error: 'let' property 'base' may not be initialized directly; use "self.init(...)" or "self = ..." instead
    self.base = base
         ^
/Users/djs/Library/Caches/org.carthage.CarthageKit/DerivedData/14.0_14A309/Charts/v4.1.0/SourcePackages/checkouts/swift-algorithms/Sources/Algorithms/AdjacentPairs.swift:61:16: note: 'base' declared here
  internal let base: Base
               ^
/Users/djs/Library/Caches/org.carthage.CarthageKit/DerivedData/14.0_14A309/Charts/v4.1.0/SourcePackages/checkouts/swift-algorithms/Sources/Algorithms/AdjacentPairs.swift:134:10: error: 'let' property 'base' may not be initialized directly; use "self.init(...)" or "self = ..." instead
    self.base = base
         ^
/Users/djs/Library/Caches/org.carthage.CarthageKit/DerivedData/14.0_14A309/Charts/v4.1.0/SourcePackages/checkouts/swift-algorithms/Sources/Algorithms/AdjacentPairs.swift:127:16: note: 'base' declared here
  internal let base: Base
               ^
/Users/djs/Library/Caches/org.carthage.CarthageKit/DerivedData/14.0_14A309/Charts/v4.1.0/SourcePackages/checkouts/swift-algorithms/Sources/Algorithms/AdjacentPairs.swift:135:10: error: 'let' property 'secondBaseIndex' may not be initialized directly; use "self.init(...)" or "self = ..." instead
    self.secondBaseIndex = base.isEmpty

...

Some futzing around has yielded that decorating the culprit Sequences and Collections (e.g. AdjacentPairsSequence , AdjacentPairsCollection , etc) with the @frozen modifier fixes these build errors.

Alas, I concede that reasoning about ABI Stability isn't quite my forte, so I would like to confirm here before doing anything. Does application of the @frozen decorator sound like the Correct™ approach? If so, I'd be happy to follow up with a PR. Otherwise, I'll shift my investigation elsewhere.

yes and no.

with very few exceptions (e.g. structures that need to evolve over time while maintaining binary compatibility) library types should always be @frozen. but swift-algorithms is not my library, so there may be a reason why these types are not @frozen.

cc @nnnnnnnn

at the same time, direct-initialization of struct properties by out-of-module users is usually a bad idea, because the initializer might have special logic to initialize the type’s state. in fact i usually go further and say you shouldn’t do this from outside of the file the type lives in.

so i don’t know why Charts is doing this, it should be calling the appropriate memberwise init, which if the type has frozen layout, should compile to direct initialization anyway.

2 Likes

Agreed. Charts should use existing inits on these structs, and if for some reason an appropriate init doesn't exist, they should make a feature request to add it.

It should be noted that Charts is not initializing nor configuring these structs directly. Rather, it is invoking @inlinable convenience methods from Algorithms itself (namely, indexed() and partitioningIndex(where:)), which appears to be the reason for the aforemented build errors when enabling BUILD_LIBRARY_FOR_DISTRIBUTION.

Ah. Well, then I suppose the question is why BUILD_LIBRARY_FOR_DISTRIBUTION is being set for a library that does not claim to be ABI stable.

Carthage-ism, unfortunately. Our SDK has 4 external dependencies, and for our SDK to support library evolution, we tell Carthage to enable BUILD_LIBRARY_FOR_DISTRIBUTION when building said dependencies. However, it seems to apply transitively as well to the libraries our 4 dependencies depend on (so Charts -> Algorithms).

We acknowledge this may require a deeper (and perhaps much-needed) re-think of our SDK build process, but just trying to exhaust all other resources and options first.

BUILD_LIBRARY_FOR_DISTRIBUTION enables two things: it makes it so that the library can be consumed by different versions of the Swift compiler than was used to build it, and it potentially makes it so that the library can be replaced with a newer version without rebuilding downstream consumers. The first is often desirable even when the second is irrelevant (as it basically always is for everyone not shipping an OS), and doesn't require that the library make any ABI stability promises.

An upstream dependency doesn't need to guarantee ABI stability for a downstream library to provide that (it merely means that the two have to be updated together), but both do have to be built in evolution mode.

cc @nnnnnnnn (Sorry to be a pest; this has been a big blocker and we're trying to figure out next steps. :confused:)

I'm pretty sure that only public inlinable init should have the requirement of making the struct frozen and (at least the first one) is internal, not public:

So at the very least, you should make a bug report at GitHub - apple/swift: The Swift Programming Language

Otherwise adding @frozen to those structs seems reasonable, not gonna make your previous process any worse. I haven't distributed any binary frameworks that depended on other external frameworks, so don't know if there could be any long term issues.

An @inlinable internal method has public ABI but is not visible via the API. That is, it can be called from other @inlinable functions which can be inlined across module boundaries, but not directly from source across module boundaries.

Since a designated initializer directly initializes the stored properties of a type without going through accessors, it depends on the layout of the type. Thus, if the designated initializer is inlinable, the type must be frozen, otherwise there is no way to safely inline the initializer across a module boundary.

4 Likes