SE-0364: Warning for Retroactive Conformances of External Types

Hello, Swift community!

The review of SE-0364: Warning for Retroactive Conformances of External Types begins now and runs through July 27th, 2022.

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 or DM. When contacting the review manager directly, please keep the proposal link at the top of the message and put "SE-0364" 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,

Steve Canon
Review Manager

14 Likes

Overall +1. This has long been a weird space in the language—the compiler happily allows such constructions but whenever it comes up in a thread in #swift-users the overwhelming message from the language designers is "don't ever do this!"

To that point, I do wonder if this proposal should take a more hard-line stance.

  • To the extent that this is a particularly bad problem for ABI-stable libraries, why should this be a warning rather than an error? IOW, under what circumstances might this:

    If absolutely necessary, clients can silence this warning by explicitly module-qualifying both of the types in question, to explicitly state that they are intentionally declaring this conformance:

    extension Foundation.Date: Swift.Identifiable {
        // ...
    }
    

    be truly necessary?

  • Should the language simply take a stance that these constructions are not supported, and make retroactive a warning (or error!) for all code? I appreciate that there may be more concerns about source compatibility in such cases, but given that it's pretty much the universal opinion of everyone close to the language that these conformances are dangerous even apart from ABI concerns, IMO it is worth seriously considering whether we should just disallow the pattern entirely.

4 Likes

I think there are some valid use cases, like someone who owns both frameworks in question from a project management perspective, but not from a module perspective. They may have one umbrella module that exports a bunch of child modules, and put this definition in the umbrella.

// ModuleA

protocol ModuleAProtoocol { ... }

// ModuleB

struct ModuleBStruct { ... }

// ModuleC

@_exported import ModuleA
@_exported import ModuleB

extension ModuleB.ModuleBStruct: ModuleA.ModuleAProtocol { ... }

Such a construction is otherwise valid, so I don't think it's great to ban this behavior. Doing so would require a hard dependency of ModuleB to ModuleA, which may not be desirable.

EDIT: I found one of these in the Apple SDK. The cross-import overlay of PhotosUI with SwiftUI declares this:

extension Photos.PHLivePhoto : CoreTransferable.Transferable

This is totally valid, since the PhotosUI team owns the Photos module, and is making the intentional decision to add this conformance inside _PhotosUI_SwiftUI.framework.

14 Likes

This is a small but very important step in the right direction.

The source breaking potential of declaring retroactive conformances of foreign types is well illustrated in the proposal, but I'd like to point out an even deadlier potential problem: any binary that includes such conformances for ABI stable types may stop working when a future version of the corresponding library gains its own version of the same conformance.

We can easily end up in a situation where the executable is relying on the precise behavior of its own conformance, while it's also linking ABI stable OS components that are depending on the behavior of the newly introduced upstream implementation. The Swift Runtime needs to select exactly one conformance as active, and no matter which one it chooses, the executable will break, leading to a potentially unresolvable binary compatibility issue.

Here is a somewhat contrived example, for a conformance I'd love to add to the stdlib.

An app executable compiled with Swift 5.6 may include the following conformance:

extension String.Index: CustomStringConvertible {
    public var description: String { "\(encodedOffset)" }
}

Code in the same app may also depend on this exact implementation, for whatever reason:

let str = "\("hello".endIndex)" // "5"
let value = String(str, radix: 10)! // 5
...

A future version of the Swift Standard Library will (hopefully!) gain its own CustomStringConvertible conformance on String.Index:

// In the Standard Library:
@available(macOS x.y, iOS u.w, *)
extension String.Index: CustomStringConvertible {
    public var description: String {
      // Something returning a string of the form "5[utf8]"
    }
}

If this conformance takes precedence, the binary will no longer work:

let str = "\("hello".endIndex)" // "5[utf8]"
let value = String(str, radix: 10)! // 💥
...

If the app's conformance takes precedence, then similar issues may instead happen inside any OS component that gets loaded into the same process.

The upshot is that ABI stable modules (such as the Swift Standard Library) cannot currently introduce any new conformances to their own types without risking such issues. The risk varies greatly between particular conformances (it's probably not very high for the conformance above), but to some extent it's always there. It's also difficult to adequately estimate in advance; bincompat issues tend to pop up only after a change has been implemented. (Sadly, the risk increases directly with the desirability of the conformance: the more useful its addition would be, the more likely it is that apps would attempt to fill the gap on their own.)

This warning, as proposed, will not improve this situation at all. ABI stable modules will continue to be unable to add new conformances to their own types without risking binary compatibility breaks.

I think it would be helpful if we started to push back on such conformances now. Therefore, I believe it would be worth considering widening the scope of the proposal, by one or more of

  1. enabling the warning in all contexts,
  2. making it unconditional (while letting module maintainers declare a list of other modules that are allowed to add conformances), and/or
  3. turning it into an error in Swift 6 mode.

It's better doing this late than never, but the cat is out of the bag now -- we have about half a decade's worth of existing binaries with such conformances that we need to keep working, and the list grows larger every day.

That said, even without widening its scope, this warning will still be extremely helpful in preventing the accidental introduction of such conformances in ABI stable library code, helping to eliminate binary compatibility issues arising from such unintentional mistakes. This very much a worthy benefit on its own, if only of interest to people maintaining such libraries.

11 Likes

I am still wary of making this specific heuristic a full error, because of the umbrella module construction I discussed above. Cross-import overlays, for example, can absolutely define these conformances and it should be fine for them to do so.

I wonder if the correct way to make this an error needs to come after we formalize the notion of resilience domains. We could upgrade this to an error if multiple modules within the same resilience domain could freely conform their types without error.

I do think I would support enabling this in all contexts, and just knowing that it's going to cause a bit of churn in application clients, because as you said, it's still invalid for app clients to do this because it prevents the standard library from evolving in the future.

10 Likes

The warning is good.

The fix seems like an abstruse hack, and is demonstrative that we need another proposal for a better way to silence warnings than, "add the explicit typing that you probably would normally have left out".

3 Likes

The initial draft was a @_retroactiveConformance attribute you had to add, but that seemed too easy to add. There's lots of precedent in the compiler for source changes to silence warnings, many of which amount to "add parens around this expression to silence the warning"

4 Likes

I also had the thought about whether the method of silencing was too ‘soft.’ I really like that it is an extremely clear way for the user to clarify intent—the issue is that the type/protocol are from other modules, so you silence by being very explicit about the modules they’re coming from—but I’m not sure this method of silencing would appropriately convey the implication of such a decision.

Maybe in the world where this only applies to ABI stable libraries that’s okay, since resilience is more of an expert-level feature and we wouldn’t expect those authors to blindly silence a warning. But I worry that if extended this warning to, say, all clients of resilient libraries then users would just Google the “retroactive conformance” warning, slap on the module qualifiers to silence it, and still end up getting broken later on. I suppose we could sleep easier with those breakages since we at least tried to help the user, but that doesn’t actually solve the practical issue of “we want to introduce a new conformance and that might break clients.”

I agree with Karoy that this is a step in the right direction even limited to the case that the proposal addresses as written, but since there’s appetite for a more expansive warning in the future I think we should try to pick a forward-looking silencing mechanism.

2 Likes

I think this is actually a very clever solution, more than meets the eye and quite elegant.

The presence of something typically left out but syntactically valid to include, such as extra parens or a restated module name, draws attention to itself just so. In general, warnings are silenceable when we think it's not necessary to keep displaying it once it's been acknowledged by the author; rather, we want to give authors the ability to make those warnings disappear. It is fitting, then, that we offer unobtrusive ways of silencing the warning such that a casual reader doesn't have to see some dedicated warning-silencing sigil, while a person who is manipulating the code and curious why there is seemingly extraneous notation can simply delete it to recover the silenced warning.

8 Likes

Is this problem limited to conformances though? I can add:

public extension String.Index {
    func foobar() { ... }
}

and get exactly the same set of issues you are describing, no?

In your example, the only code that will ever call foobar is the code that you yourself write. With a retroactive conformance, code that you didn’t write, even possibly OS code, can end up using your conformance.

9 Likes

I agree it might be premature to turn the proposed warning into an error, but it may still be worth considering widening the scope of the warning, possibly with a tweak to the proposed silencer mechanism.

It doesn't help that there is nothing about the conformance declaration syntax that indicates that it may have an effect outside the current module. For example, a stray declaration such as

extension String: Error {}

affects the entire process that loads it, including code that doesn't even import the containing module -- but the declaration doesn't even need to include a public keyword that would hint at this. This feels like a language design defect to me; conformance declarations were given much too lightweight syntax given their global effect.

Forcing people to spell public could perhaps be a better way to silence the new warning.

// I'm doing the wrong thing, but at least I admit this will have an effect
// outside of this module.
public extension String: Error {}

Then again, adding public to the extension can have unintended effects on member declarations. We really missed the mark with the retroactive conformance declaration syntax.

extension String: public Error {} // ???

(I like Harlan's original attribute, too -- at least it makes the intention explicit, and it has the crucial advantage that it would make it very easy to search for alien retroactive conformances in any code base.)

This could be as "simple" as allowing each ABI stable module to optionally come with a list of friend module names that are allowed to declare foreign conformances to their types. Then again, the details would be messy, e.g., what with having to invent a mechanism for declaring such a list (that also works for C/C++ modules), and the consequences are likely to be even messier.

We'd induce significant source compatibility churn, possibly with no immediate benefit. Even if we waved a magic wand and made unblessed foreign conformances an unconditional error today, all the binaries out there that were built in the past could still prevent ABI stable modules from supplying their own substitutes.

(At least, not without considerable limitations. E.g. the stdlib could put its new conformances in a new module that only gets loaded into newly built binaries. But ABI stable frameworks wouldn't be allowed to load that module, meaning that the stdlib API surface will be indefinitely split into a weirdly tiered setup. Most new conformances would also be expected to back-deploy to earlier stdlib releases (or they wouldn't really serve as reasonable alternatives to existing grey market substitutes), which isn't a thing we can express within the language. Still, we may be able to accept the limitations and work through the manual back deployment pain for the stdlib -- but it seems impractical to routinely do this in every release, or to do this for modules other than the stdlib.)

Swift developers currently also have no easy way to replace their ill-advised retroactive conformances. The standard recommendation to avoid them is to create throwaway wrapper types, but having to manually define those, and explicitly wrap/unwrap values is quite heavy boilerplate that (while it is technically the correct solution) doesn't read like a code quality improvement. We should develop language features aimed at reducing the cost of doing the Right Thing, such as some sort of lightweight syntax to introduce such wrapper types. Ideally the eventual compiler diagnostic would come with a fix-it -- although that may be an unrealistic expectation.

So we find ourselves in an unfortunate library evolution dead end, and it would take a significant amount of work to escape from it. Widening the scope/severity of this warning alone would not really solve this issue, but it may be something to consider if/when we've taken more steps to get rid of such conformances.

Anyway, the new warning, exactly as proposed, would still be extremely helpful in preventing dodgy conformances from accidentally leaking into the public API of ABI stable modules.

9 Likes

I think a version of scoped conformance would be a better alternative

4 Likes

Module-scoped conformances would be an interesting solution, if they can be separated at runtime. Then stdlib could add a new conformance, which would get used internally and for clients that haven’t defined their own. If an app has its own conformance, it would be used within that app’s module, but wouldn’t affect stdlib

I have no idea if this is feasible to implement, or if it is, whether it could work retroactively for existing deployed apps on ABI stable platforms.

4 Likes

"reading" and "manipulating" code represent a nonbinary spectrum.

I'm not going to speak to how invisible anyone else's brain can make e.g. the : Voids or the : Anys, but for me, they're never going to be anywhere near as quick and easy to move past as a // swiftlint:disable—especially without this kind of deletion-manipulation, which serves a similar purpose as going to the docs or a dictionary.

1 Like

Scoped conformances would be great, however it's a much larger feature. We need a warning until then so we reduce the number of these that leak into projects (and worse, OSes).

6 Likes

+1 I agree this is needed.

Module-scoped conformances could be a nice way for people to migrate their existing ill-advised conformances to something better. They'd be a rather drastic change to language semantics, and may complicate runtime performance (per-module conformance caches?), but they would potentially allow people to define custom conformances without ABI consequences.

(I imagine the new feature would have to come with new runtime entry points for conformance checking that would not be present/called on systems that shipped with an earlier Swift release -- so back deployment would be a tricky problem.)

As I understand it, this would be a new, additive language feature. As such, while it would provide a new way to define conformances, it would not, on its own, replace the existing (global) conformance behavior. Apps would need to be updated to start using it. Existing binaries would not magically transition to the new behavior. So while local conformances would certainly be a helpful tool in our effort to plug this hole, they would be just a piece of the solution.

And we'd still need the proposed warning (perhaps as an error) to diagnose attempts to globally define foreign retroactive conformances -- otherwise why would folks update their code?

(Implicitly changing the meaning of existing code to make its conformances local could potentially be massively source- (and/or ABI-) breaking. And defining global retroactive conformances on foreign types/protocols must still remain possible: it just needs to be put under the control of the module that owns the type.)

1 Like

+1 on warning :warning:
-1 on error :no_entry:

I guess what I was trying to say is that we should not make it an error until folks have the ability to make the conformance private (somehow)

1 Like

I agree that this is a good idea; developers in large monorepo situations might be building large numbers of modules but not using library evolution at all, because everything is built from source at the same time so there are no ABI compatibility concerns between dependencies. But it's still useful to get those warnings about potentially unsafe patterns and to require the user type something extra to acknowledge what they're doing.

4 Likes