[AST] NFC: Deconflate mutation of self and InOut params

[davezarzycki]

Pull #29579 broke my private research branch and I'd like to upstream the fix.

Background: my project needs mutation to not be conflated with other things (reference semantics, InOut, etc).

While I'm at it, I'd also like to upstream this:

// Destructors are implicitly mutating.
selfAccess = SelfAccessKind::Mutating;

[davezarzycki]

@swift-ci please smoke test

[jckarter]

I don't think this is correct as the terms are used in the compiler now. A mutating func in a type is exactly equivalent to the implicit self argument being inout, which is never true for class methods, because the self reference itself can never be mutated. I would be more comfortable introducing a new concept to indicate instance mutability for classes, maybe something like reference capabilities from Pony or Midori.

[jckarter]

Furthermore, as @gottesmm will attest to, destructors (and maybe initializers too) also already have peculiar semantics that currently require some special case handling in the optimizer. There might be an opportunity to introduce more fine grained capabilities to model these more formally in the AST.

[davezarzycki]

In my experience hacking on an atomicity/mutation model for reference types, I have found that mutating func simply means that the member elements/fields/values are mutable. This is a simple user model and orthogonal to whether a type has value- or reference-semantics.

More so, I've found that the compiler's existing design works fairly well for this approach and the conflation between the mutation model, value-semantics, and InOut is minor.

Finally, and I'd need to check this, I think value-type destructors get an InOut self somehow, so the fact that they're not marked as mutating in this function seems inconsistent.

[jckarter]

In my experience hacking on an atomicity/mutation model for reference types, I have found that mutating func simply means that the member elements/fields/values are mutable. This is a simple user model and orthogonal to whether a type has value- or reference-semantics.

Simple though that may be, there's a lot of existing research into this sort of thing that shows there are a lot of shortcomings to that simple binary approach. Even within the existing feature set of Swift, I think a mutability model for classes that didn't take uniqueness into account, to allow for checking of copy-on-write "mutable only if uniquely referenced" requirements, would be inadequate. That also ends up being pretty close to what reference capability systems model, since in a concurrent world, you don't care only about whether a method modifies the object or not, but also whether it has the only reference currently accessing the object while it's doing so.

More so, I've found that the compiler's existing design works fairly well for this approach and the conflation between the mutation model, value-semantics, and InOut is minor.

My experience with mutating modifiers on class methods has also been the opposite of yours—we've had a long tail of bugs arising from inconsistent application of these modifiers to class methods, leading to things incorrectly being assigned inout semantics during type checking and leading to all sorts of compiler bugs. I think we've mostly stomped those out, but it would behoove us to introduce class mutation modifiers as a separate thing to avoid regressions, and also avoid pre-committing to a particular language and implementation design.

Finally, and I'd need to check this, I think value-type destructors get an InOut self somehow, so the fact that they're not marked as mutating in this function seems inconsistent.

Value type destructors don't exist today. If they did, though, they would be more accurately modeled as __consuming than as mutating.

[davezarzycki]

Hi @jckarter – I think we're on a tangent and this patch works well for my research. That being said and for Swift proper, is this patch net positive, net negative, or neutral? If it's net negative, then why? What harm is done? Thanks :-)

[jckarter]

mutating modifiers on class methods have been a source of bugs in the past, and this use of them is not orthogonal to how the modifier is used in the rest of the language. If this is for research purposes, I would ask that you introduce a new modifier bit to minimize the potential impact to the rest of the compiler.

[davezarzycki]

Hi @jckarter – I did some experiments and this patch actually improves the quality of the AST in the face of other errors, which can of course, potentially improve downstream diagnostics and avoid downstream crashes.

For example, line 270 of test/decl/ext/extensions.swift tries to create an erroneous mutating setter on a class bound protocol. While the error is diagnosed, top-of-tree computeSelfParam nevertheless makes self inout. Whoops.

[jckarter]

That's great! This change might be fine in isolation then. I'd still be wary of building anything on top of it.

[davezarzycki]

I think I learned more from this feedback than the utility in getting this upstream. I might file a new PR with a targeted AST QoI fix. Thanks @jckarter!