Warning for retroactive conformances if library evolution is enabled

Development Compiler
#1

Hey folks!

I've got a PR up now to add a warning, and I wanted to get some feedback here before hitting merge. It's about everyone's favorite feature: retroactive conformances.

Specifically, retroactive conformances that conform a type defined outside the current module to a protocol defined outside the current module. For example, the following extension declares such a conformance (poorly, I might add):

extension String: Identifiable {
  public var id: Int { self.count }
}

Such an extension may seem fine -- I would like to provide a custom notion of identity. But the issue is that these conformances are global. There can only be one conformance of any given type to any given protocol at runtime. If the standard library chooses to add such a conformance in a later OS release, that will result in duplicate symbols at load time conformances at runtime, and the dynamic loader Swift runtime's dynamic dispatch logic will just :sparkles: pick :sparkles: one.

If an app defines one of these, the issue is limited in scope -- this will just result in undefined behavior for that app until they recompile against the new SDK and delete their conformance.

Where it becomes a much larger issue is with distributed libraries and frameworks declaring these conformances. Given that there can only be one such conformance at runtime, if two frameworks declare the same conformance, then every app that uses them both will have the same issue. This might still not be a huge issue -- I'd be willing to assume the vast majority of Swift frameworks are developed and bundled within a single app.

So because of that, this warning mostly applies to libraries that are meant to be distributed. As such, I'd like to emit a warning for an extension when:

  • The module has library evolution enabled (which includes those having adopted Build Libraries for Distribution in Xcode)
  • The type being extended comes from a module outside the current compiled module (or the underlying Clang module, if this is an overlay)
  • The extension introduces a new conformance to any protocol that comes from outside the current module (or the underlying Clang module, if this is an overlay).

For the above code, the warning will read:

warning: extension declares a conformance of imported type 'String' to imported protocol 'Identifiable'; this is not supported when library evolution is enabled

And this warning can be silenced by explicitly module-qualifying all the types involved that come from outside the module. The example above would need to be rewritten:

extension Swift.String: Swift.Identifiable {
  public var id: Int { self.count }
}

And the compiler will offer a fix-it to insert the appropriate qualification.

So my main questions here are:

  • Do folks think this is a good thing to warn about?
  • Is the scope of when the warning applies too broad, or not broad enough?
  • Is the proposed way to silence the warning too easy to silence?

Thanks!

3 Likes
#2

What is the rationale behind having this be the mechanism? I understand that we, unlike Clang, currently have a policy of not providing fine-grained mechanisms to adjust individual warnings (and there have been multiple discussions on being able to silence at least some warnings, particularly deprecation), but carving out exceptions to that policy (and IMO with unintuitive mechanisms such as this one) doesn't seem like the right thing to do...

You mention

After feedback from @airspeedswift I'm moving from @_retroactiveConformance to requiring you to module-qualify everything from outside your module explicitly

In the PR, but there's not more information there.

#3

but carving out exceptions to that policy (and IMO with unintuitive mechanisms such as this one) doesn't seem like the right thing to do...

We currently have a few places through the compiler where making a somewhat-innocuous syntactic change silences the warning, like adding ()s around casts or introducing a self. to make accesses explicit. This was somewhat of a way to make code look a little stranger and force reviewers to take another look at it.

I'm hesitant to support a generalized mechanism for silencing individual warnings, except for deprecation warnings.

#4

We pervasively allow the silencing of specific instances of warnings at the point of use by, e.g., parens. We deliberately do not have toggles to turn off all instances of a specific warning. Those are two very different things.

It would seem imminently reasonable to me that the way to acknowledge that your protocol and type come from outside modules is to write out the name of the outside modules.

4 Likes
#5

This is not strictly true. Although there's no exposed syntax for disambiguating them, conformances defined in different places are modeled as distinct entities, and multiple conformances can (mostly) coexist in the same binary. Dynamic casting protocol lookup is the primary place there's an unavoidable need to pick one arbitrarily, but anywhere you satisfy a protocol constraint statically, the compiler can pick a different one in different contexts. Nonetheless, I'm still in favor of discouraging retroactive conformance until we have better language support for handling multiple conformances (and the compiler stops cheating in some places where it assumes conformances are global).

2 Likes
#6

This is not strictly true.

Oh! My apologies, then. I was under a mistaken impression about these symbols that it was an error at load time. Thanks for the correction! I've edited the post.

#7

You can correct me if I'm wrong, but this isn't true for most warnings, is it? Most warnings cannot be turned off using parens. If that's the case, I don't agree with the "pervasive" aspect of your statement.

I very deliberately chose the wording "fine-grained mechanisms to adjust individual warnings" to avoid this discussion. I'm not talking about providing a flag to turn off a warning over a whole module (or similar). I'm not even suggesting an alternative.

That said, from what has been presented so far, it seems like the attribute solution is at least clearer in its intent that something non-trivial is going on (and easier to Google for).

I agree, but I don't see what that has to do with turning off a warning? Maybe I'm not wracking my brain hard enough, but I can't think of any other PL where using explicit qualification turns off a warning. With parens, at least there's the precedent of Clang (and I think GCC too) which has similar functionality.

I'm not suggesting we should support a generalized mechanism for silencing individual warnings. But I do think we should at least consider this situation and the deprecation case together instead of in isolation.

#8

It's not about the quantity of different warnings that can be silenced, it's that as a design principle Swift allows many warnings which users reasonably would want to be able to silence to be silenced. I did not say that they could all be turned off with parens: that was one example. Others include _ = to silence discarded result warnings, as Any to silence warnings about printing optional values, etc.

Never mind other programming languages, we already have such a feature in Swift itself: @warn_unqualified_access.

1 Like
#9

Interesting, I didn't know about this attribute. Given that, I'm okay with this change (from an initial -1).

#10

One eventual solution to multiple conformance disambiguation would be to allow conformances to be given names, sort of like ObjC categories, except we'd provide some syntax to let you specify you want to use a specific conformance in a generic call site or type instantiation:

// strawman "named category" style syntax
extension String: Identifiable (IDByCount) {
    public var id: Int { self.count }
}

// explicitly use this conformance in a generic function
func foo<T: Identifiable>(x: T)

foo("xyz" by IDByCount)

We could then require that retroactive conformances (as well as protocol extension conformances, private conformances, or other cases of conformance that we currently disallow because they have no hope of being "the" canonical Default Conformance for a type) be named. If we want a way to suppress the warning, maybe we could introduce the named conformance syntax and encourage people to start naming their non-canonical conformances.

2 Likes
#11

I am in favor of this warning. When I see PRs go up with a retroactive conformance I always have to go explain why it is bad and the risks.

1 Like
#12

I would like to propose lifting this restriction and emitting the warning in all cases, or alternatively providing a tool that the Swift Package Manager can use to trigger this same checking.

This issue does not only affect libraries in library evolution mode. For those in source-available mode, such as when compiling in the Swift package manager, the issue isn't necessarily that undefined behaviour is triggered. Instead, the issue is that the change appears to be a semver minor change that can break downstream compiles. This is because in many cases Swift cannot break the ambiguity as to which implementation it should use, and so will refuse to compile.

It would be good to find a way to discourage this usage in all libraries that are intended to be distributed, even if they aren't being distributed in binary form.

3 Likes
#13

I fully agree that this warning applies to all distributable libraries, whether they be source or binary. Would you suggest a -warn-external-retroactive-conformances flag that we could teach SwiftPM to pass? My worry is that we don’t have a great signal within the compiler that a given module is a library that intends to be distributed, beyond library evolution.

#14

I like such a flag, but I don't do enough work on SwiftPM to have an informed opinion about how practical that would be. I think we should summon @NeoNacho, who will have a better idea.

#15

I think this phrasing of the warning is misleading because it is supported (hence why it is a warning and can be silenced), it just might not be a great idea.

1 Like
Terms of Service

Privacy Policy

Cookie Policy