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
_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev

Reply via email to