Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
igor-makarov opened this issue Aug 13, 2019 · 21 comments
Closed

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

igor-makarov opened this issue Aug 13, 2019 · 21 comments
Assignees
@theblixguy
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself

Comments

@igor-makarov
Copy link

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
Copy link
Contributor

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

@theblixguy
Copy link
Collaborator

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
Copy link
Collaborator

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

@belkadan
Copy link
Contributor

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
Copy link
Collaborator

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

@belkadan
Copy link
Contributor

@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
Copy link
Member

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
Copy link
Collaborator

Thanks! PR here: #26669

@theblixguy
Copy link
Collaborator

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

@igor-makarov
Copy link
Author

Change got reverted - #26695

@theblixguy
Copy link
Collaborator

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
Copy link
Author

Can you post a link to the discussion?

@theblixguy
Copy link
Collaborator

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

@theblixguy
Copy link
Collaborator

Fixed on master: #27057

@belkadan
Copy link
Contributor

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
Copy link
Collaborator

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

@belkadan
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor

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

@theblixguy
Copy link
Collaborator

Fixed again by #27639

@swift-ci
Copy link
Collaborator

Comment by Joseph Twomey (JIRA)

@swift-ci create

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@theblixguy theblixguy

Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@slavapestov @belkadan @theblixguy @swift-ci @igor-makarov