Add '@_requiresConcreteImplementation' and use it

[xwu]

This PR adds a new attribute, @_requiresConcreteImplementation, and adopts it for the one protocol requirement in RandomNumberGenerator.

Motivation

The motivation for adding this facility is a scenario as follows—

• Protocol P has a requirement func frobnicate(_: Int).
• Protocol P can offer a protocol extension method func frobnicate<T>(_: T) which is implemented by calling frobnicate(_: Int).

Currently, the type checker regards the protocol extension method as a valid witness for the protocol requirement, causing a recursive default implementation. Users implementing P are misled, thinking that they have implemented all necessary protocol requirements, only to be faced with a runtime error.

A concrete example of this problem exists in the standard library: RandomNumberGenerator has a requirement next() -> UInt64 which is unwittingly witnessed by a protocol extension method next<T: FixedWidthInteger & UnsignedInteger>() -> T. Conforming types that do not supply a concrete implementation of the non-generic method will compile but cause crashes at runtime.

It should not be necessary to name two semantically identical methods differently just to work around this problem. As the core team stated in swiftlang/swift-evolution#912:

The issue with accidental infinite recursion occurs in other APIs and can't be fixed so easily in those cases, so ideally we'd find a way of enhancing the language to help users avoid this situation in a more general way.

See also: https://forums.swift.org/t/how-to-best-fix-this-old-random-api-bug/34526

Solution

The current solution takes inspiration from existing attributes such as @_nonoverride and @_implements. The new attribute does what it says on the tin. It can be used on any (non-associatedtype) protocol requirement so that Sema rejects an implementation in a protocol extension as an acceptable witness.

Unfortunately, it appears that we are running out of numbers to serialize attributes. I have had to twiddle with DeclTypeRecordNodes.def in order to make room; there's now room for a handful more attributes, but this is clearly not sustainable—nor am I certain about the implications of renumbering all decl type record nodes for attributes in this way.

---

There is an additional (aspirational) goal for this solution which is interesting to consider:

It is permitted to add a requirement to a resilient protocol, but only if a default implementation is also provided. One may envision scenarios where a default implementation for such a requirement is possible for all conforming types but greatly suboptimal.

It would be ideal to support scenarios where the default implementation can be made available for all existing clients (i.e., for resiliency purposes), but nonetheless to require all new conformances going forward to provide concrete implementations. The @_requiresConcreteImplementation attribute could enable this possibility.

I am not nimble enough in the inner workings of the compiler but suspect that the implementation presented here would not be sufficient to enable this use case as-is. It would be necessary to record a default implementation as a valid witness for runtime purposes but invalid at compile time conformance checking. Something like this must already be present in the codebase for availability annotations.

[xwu]

@DougGregor because I don't actually know what I'm doing while rooting around Sema. @airspeedswift as he's been involved with the motivating issue as regards to the standard library.

I'll also tag @jens-bc here as he raised the issue regarding RandomNumberGenerator on GitHub. Also, @belkadan I know you're not in the same role anymore, but I wonder if you might be able to comment on any thoughts related to the utility of something like this for resilient libraries.

[xwu]

APIBreakingToAdd?

[xwu]

I am not proud of this, but there are 99 existing decl attrs, so with one more we would otherwise crash into FIRST_PATTERN(PAREN, 200).

[xwu]

@swift-ci Please test

[xwu]

@swift-ci Please test

[xwu]

Third time's the charm.
@swift-ci Please test

[belkadan]

Personally, I'm pretty against this as a general-purpose solution. There's no reason (in a perfect world) why every concrete type would have to avoid getting its implementation from a protocol extension. I'd say it's the implementation who bears this responsibility, because it is a valid witness to a requirement in the protocol it is extending. So I'd prefer an attribute on the implementation that says "this implementation must not satisfy a requirement with the same full name in the protocol it's extending" (and have it be a warning, not an error, so as not to break source compatibility). That's effectively similar to the (conservative) infinite recursion checking we already have—"the body of this method may call the requirement in an unguarded way, so they better not be the same"—which makes me feel a lot better about it being a reasonable fix to the problem.

None of these solutions solve the general problem of "two requirements and you must implement at least one of them", but that's just gonna be hard regardless.

---

As a side note, it should be fine to renumber all the serialization node numbers as long as you bump SWIFTMODULE_VERSION_MINOR. Those were more important when we thought we were going to have a stable binary format.

[xwu]

So I'd prefer an attribute on the implementation that says "this implementation must not satisfy a requirement with the same full name in the protocol it's extending" (and have it be a warning, not an error, so as not to break source compatibility).

More like an @_implementsNothing type of annotation? Yeah, that's an alternative I thought about. I think in practice it'll work out fairly similarly in terms of the user experience, and I almost wonder if it could be worked into the existing code paths for @_implements. (Or, we could recruit/repurpose @_disfavoredOverload into this, by warning if the best possible witness is a disfavored overload.)

What swayed me more towards the approach given here, I guess, was considering the case of complex protocol hierarchies:

It's quite legitimate for a refining protocol to provide a default implementation for a requirement of a less refined protocol. It's also legitimate to make use of default implementations guaranteed by less refined protocols in building up a more refined one. It would seem to me that authors of protocol extension methods can more easily reason about their code if they don't have to worry about some refining protocol inadvertently throwing a wrench into things in this way, and seems consonant with Swift's general approach of placing such decisions in the hands of those vending the APIs (c.f. open versus public).

have it be a warning, not an error, so as not to break source compatibility

Since we have control over where the attribute is applied, we can still have enforcement be an error; we would only break source compatibility if we applied it somewhere that rendered existing working conformances an error. In this case, no existing code would actually break, because even though conforming RandomNumberGenerator types might compile, they would crash instantly on usage.

[xwu]

Unrelated OS X test failure.

[belkadan]

It's also legitimate to make use of default implementations guaranteed by less refined protocols in building up a more refined one.

I don't understand this point. Default implementations are not guaranteed, and neither of our designs would change that.

It would seem to me that authors of protocol extension methods can more easily reason about their code if they don't have to worry about some refining protocol inadvertently throwing a wrench into things in this way, and seems consonant with Swift's general approach of placing such decisions in the hands of those vending the APIs (c.f. open versus public).

I don't agree. If a superclass wants to invoke a method and be sure a subclass doesn't override it, the other method has to not be open; if a protocol extension method wants to invoke another method and be sure it's not going through the requirement, that other method has to not be named the same as the requirement. Again, though, I don't see what this has to do with whether the attribute belongs on the requirement vs. the default implementation; in neither case do we have a protocol extension method trying to invoke another protocol extension method.

Since we have control over where the attribute is applied, we can still have enforcement be an error; we would only break source compatibility if we applied it somewhere that rendered existing working conformances an error. In this case, no existing code would actually break, because even though conforming RandomNumberGenerator types might compile, they would crash instantly on usage.

It seems nice to me to design the feature so that it can be applied to other protocols where existing implementations might be only partially broken rather than completely broken.

[xwu]

I don't understand this point. Default implementations are not guaranteed, and neither of our designs would change that.

I should rephrase that more carefully: an implementation of the less refined protocol's requirements is guaranteed in that the conforming type would have to conform to the less refined protocol as well; this could be the default one.

Again, though, I don't see what this has to do with whether the attribute belongs on the requirement vs. the default implementation; in neither case do we have a protocol extension method trying to invoke another protocol extension method.

I think I have described the scenario poorly. Let me do so by way of example; it's going to take a beat to come up with a good one, so perhaps it's not a common enough scenario to be concerned about solving. I'll circle back to this.

[xwu]

This has been a while and, I think, has been superseded by the cleverly devised workaround of @_alwaysEmitIntoClient @available(unavailable...) default implementations. I'll close.