[swift-dev] Miscompile with protocol compositions

[Slava Pestov]

Hi all,

I’ve spent most of the last two days debugging various issues with property and subscript accesses on class-constrained existentials and I’ve just now realized the root cause is a much more fundamental issue.

Consider the following code:

protocol P {
  var x: Int { get set }

  init()
}

func takesP(p: inout P & AnyObject) {
  p.x = 1
}

Note that ‘takesP’ has a class-constrained existential type as a parameter. You can replace AnyObject with any class protocol, it won’t change anything below.

Also, the init() requirement in the protocol will come into play later — for now, just consider the ‘var x’, which has a mutating setter.

The above code compiles fine. Let’s take a look at the SILGen though:

  %2 = load [copy] %0 : $*AnyObject & P
  %3 = open_existential_ref %2 : $AnyObject & P to $@opened("52860F0E-2B19-11E7-8595-34363BD08DA0") AnyObject & P
...
  %8 = alloc_stack $@opened("52860F0E-2B19-11E7-8595-34363BD08DA0") AnyObject & P
  store %3 to [init] %8 : $*@opened("52860F0E-2B19-11E7-8595-34363BD08DA0") AnyObject & P
  %10 = witness_method $@opened("52860F0E-2B19-11E7-8595-34363BD08DA0") AnyObject & P, #P.x!setter.1, %3 : $@opened("52860F0E-2B19-11E7-8595-34363BD08DA0") AnyObject & P : $@convention(witness_method) <τ_0_0 where τ_0_0 : P> (Int, @inout τ_0_0) -> ()
  %11 = apply %10<@opened("52860F0E-2B19-11E7-8595-34363BD08DA0") AnyObject & P>(%7, %8) : $@convention(witness_method) <τ_0_0 where τ_0_0 : P> (Int, @inout τ_0_0) -> ()

So we load the ‘AnyObject & P’ existential value from our inout argument, then in order to call the protocol requirement for x’s setter, which takes the ‘self’ value as an indirect argument, we allocate a box on the stack, store the reference into the box, call the requirement with the address of the box as the value of ‘self’, and destroy the box. We never write the result back to the inout.

So consider the following implementation of a concrete type conforming to P:

extension P {
  var x: Int {
    get { return 0 }
    set {
      self = Self()
    }
  }
}

class C : P {
  required init() {}
}

Note that the mutating setter for ‘x’ assigns to ‘self’, which is totally fine — it’s defined in a protocol extension of a non-class protocol P.

Let’s try to use our concrete class type with our function takesP():

let c = C()
var cc = c

takesP(p: &cc)

print(c === cc)

When you compile and run this, it outputs ‘true’, which is not what you would expect from reading the code, since the setter re-assigns ‘self’.

Ok, maybe we can just tell people that if your class conforms to a non-class protocol, then assignments to ‘self’ inside implementations of the protocol requirement are not allowed, and are silently dropped.

Now look at how we compile this version of takesP:

extension P {
  mutating func foo() {
    self = Self()
  }
}

func takesP(p: inout P & AnyObject) {
  p.foo()
}

Here instead of calling a mutating setter, we call a mutating method.

If we look at the SILGen, there is no stack allocation anymore, instead we pass the class reference directly as an inout argument, which is wrong:

  %7 = open_existential_ref %6 : $AnyObject & P to $@opened("F78B53CA-2B18-11E7-A73C-34363BD08DA0") AnyObject & P
  %8 = witness_method $@opened("F78B53CA-2B18-11E7-A73C-34363BD08DA0") AnyObject & P, #P.foo!1, %7 : $@opened("F78B53CA-2B18-11E7-A73C-34363BD08DA0") AnyObject & P : $@convention(witness_method) <τ_0_0 where τ_0_0 : P> (@inout τ_0_0) -> ()
  %9 = apply %8<@opened("F78B53CA-2B18-11E7-A73C-34363BD08DA0") AnyObject & P>(%7) : $@convention(witness_method) <τ_0_0 where τ_0_0 : P> (@inout τ_0_0) -> ()

In a no-asserts build, like Swift 3.1 GM, this produces a binary which crashes at runtime. In an asserts build, we hit an assertion in Sema:

Assertion failed: (fromType->is<LValueType>() && "Can only convert lvalues to inout"), function coerceObjectArgumentToType, file /Users/slava/new/swift/lib/Sema/CSApply.cpp, line 6336.

There seem to be two fundamental problems here:

1) Sema models the base expression of a mutating member as an RValue, because it checks if the base expression type has reference semantics, not the *self type of the member*.

2) SIL cannot express an inout access performed on the payload of class existential.

#1 is easy enough to fix, and allows my code examples involving subclass existentials to type check without hitting the LValue assertion.

#2 is more fundamental. At first I thought it would be sufficient to change SILGen to only use class existential representation for protocol compositions where *all* members are class-constrained, instead of protocol compositions where *any* member is class-constrained. However this is insufficient because a class-constrained protocol can refine a non-class constrained protocol.

We could say that a class-constrained protocol only uses class existential representation if all protocols it inherits also use class existential representation. Basically, split up ProtocolDecl::requiresClass() into two predicates — the current requiresClass() that expresses a constraint in the formal type system, and a stricter requiresNonMutatingSelfAccess() or similar which expresses that requirements and extension methods cannot be ‘mutating’. The latter would only be true if all refined protocols are also class constrained, and would be the condition for using a class existential representation.

Now the reason the first example generates valid (but surprising) SIL, while the second one crashes, is that we seem to have a hack somewhere to handle mutating property access on a non-mutating value by consing up a box, copying the value into the box, and tearing down the box. We could conceivably generalize this hack to work on mutating methods as well, but it would lead to the same surprising behavior where the assignment to ’self’ is ‘lost'.

However, there would be no loss of efficiency in that case, as we would still use the class existential representation in all the cases where we can currently use it.

Another potential fix is to simply disallow mutating methods from being called on class-constrained existentials altogether, but that is a source breaking change (even though the old behavior was extremely flaky).

Any thoughts?

Slava