[SR-11298] Writable property declaration in a conditional-conforming protocol extension has incorrect mutability

[igor-makarov]

Previous ID	SR-11298
Radar	None
Original Reporter	@igor-makarov
Type	Bug
Status	Resolved
Resolution	Done

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Bug
Assignee	@theblixguy
Priority	Medium

md5: 24c2c75eac517f7dee964b09386971e2

duplicates:

• SR-4541 Class-Only protocol inheriting from normal protocol require to be var to allow assign to their properties.

Issue Description:

Attached snippet:

protocol Protocol {}
// If constrained to class, it works
// protocol Protocol: class {}

class ClassA: Protocol {
    // Mutable instance property
    var property: String = ""
}

extension Protocol where Self: ClassA {
    // Mutable wrapper property
    var wrappingProperty: String {
        get { return property }
        set { property = newValue }
    }
}

let instance = ClassA()

// Direct property access allowed
instance.property = ""

// error: Cannot assign to property: 'instance' is a 'let' constant
instance.wrappingProperty = ""

Because `wrappingProperty` is declared in a conditional conformance to `ClassA`, a class, `wrappingProperty` should be mutable, just like `property`.

In fact, if `Protocol` is declared as `class`, the code compiles without error.

Occurs in Swift 5.0 and 5.1.

Additional example, this time a read only getter:

protocol Protocol {}
// If constrained to class, it works
// protocol Protocol: class {}

class ClassA: Protocol {
    // Mutable instance property
    var property: String = ""
}

extension Protocol where Self: ClassA {
    // Mutable wrapper property
    var wrappingProperty: String {
        get { return property }
        set { property = newValue }
    }
}

class ClassB {
    private var classA = ClassA()
    var classAReadOnly: ClassA {
        return classA
    }
}
let instance = ClassB()

// Direct property access allowed
instance.classAReadOnly.property = "A"

// error: Cannot assign to property: 'classAReadOnly' is a get-only property
instance.classAReadOnly.wrappingProperty = "B"

[belkadan]

This is correct behavior; see SR-4541 for another example of it.

[theblixguy]

I have a fix for this, but I am not sure if its a reasonable approach (although it works).

cc @slavapestov does this look good to you? master...theblixguy:fix/SR-11298

[theblixguy]

@jckarter said on twitter that it seems like a bug: https://twitter.com/igormaka/status/1161223172278116352

[belkadan]

Oh, whoops, I misread where the property was declared. Yes, this would be a bug and not the same as SR-4541. Thanks, @theblixguy!

[theblixguy]

@belkadan No problem! Does my implementation look good to you? I can create a PR if it looks good.

[belkadan]

@slavapestov or maybe @xymus would have a better idea of whether this is the right place to do that work, but you can create the PR regardless.

[slavapestov]

The code called from typeCheckDecl() only runs on declarations in the primary file, and they're visited in order. So if you use the storage from another file, or you have a forward reference in a primary file, the code you added won't run. Please take a look at incorporating it into SetterMutatingRequest::evaluate() instead.

However I'm surprised this doesn't already work. There might be a bug elsewhere. In any case, feel free to put up a PR and we can discuss it in more detail there. Thanks for taking a look!

[theblixguy]

Thanks! PR here: #26669

[theblixguy]

Fixed on master. Please verify using the next available master snapshot!

[igor-makarov]

Change got reverted - #26695

[theblixguy]

Yes, it’s currently being discussed on Swift forums because there’s some source breakage. I think we could un-revert it once it’s clear how to best handle this.

[igor-makarov]

Can you post a link to the discussion?

[theblixguy]

https://forums.swift.org/t/class-constrained-protocol-extension-has-incorrect-mutatingness/27962

[theblixguy]

Fixed on master: #27057

[belkadan]

Reverting again, unfortunately (#27611 The fix doesn't just change the behavior of setters; it also changes the behavior of explicitly mutating functions in class-constrained protocol extensions. @rjmccall has clarified that the core team's original ruling was only about making setters non-mutating by default in class-constrained protocol extensions.

(I'm personally worried that this means we need mutating set to be fully consistent, but maybe not.)

[theblixguy]

Hmm, I guess we need to special case it for just setters then?

[belkadan]

I think you could keep your change for the default behavior but have the behavior with explicit mutating stay the way it was before. That would also simplify the workaround for matching the old behavior to "add mutating" like you originally wanted.

[theblixguy]

Okay, I'll put up a new fix shortly. Could you provide an example where the behaviour shouldn't change? Just so I can add it to the tests.

[belkadan]

mutating func foo() and arguably also an explicitly written mutating set (turns out we do accept it already)

[theblixguy]

Fixed again by #27639

[swift-ci]

Comment by Joseph Twomey (JIRA)

@swift-ci create