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

[SR-7083] lazy properties can't have observers #49631

Closed
jckarter opened this issue Feb 26, 2018 · 12 comments
Closed

[SR-7083] lazy properties can't have observers #49631

jckarter opened this issue Feb 26, 2018 · 12 comments

Comments

@jckarter
Copy link
Member

jckarter commented Feb 26, 2018

Previous ID SR-7083
Radar None
Original Reporter @jckarter
Type Bug
Status Resolved
Resolution Done
Additional Detail from JIRA
Votes 4
Component/s Compiler
Labels Bug, StarterBug
Assignee @theblixguy
Priority Medium

md5: 97bf74aed4a6125643746316d5582ad4

Issue Description:

The compiler won't let you put didSet/willSet observers on a lazy property. Since lazy properties are supposed to look and feel like stored properties, they should be able to have observers.

@jckarter
Copy link
Member Author

jckarter commented Feb 26, 2018

Possible implementation strategy: Lazy variables are turned into computed properties during type checking in TypeCheckDecl.cpp's DeclChecker::visitBoundVariable method:

    // Synthesize accessors for lazy, all checking already been performed.
    if (VD->getAttrs().hasAttribute<LazyAttr>() && !VD->isStatic() &&
        !VD->getGetter()->hasBody())
      TC.completeLazyVarImplementation(VD);

    // If this is a willSet/didSet property, synthesize the getter and setter
    // decl.
    if (VD->hasObservers() && !VD->getGetter()->getBody())
      synthesizeObservingAccessors(VD, TC);

The lazy variable checking happens right before observers are normally synthesized. When we create the getter/setter for lazy variables inside TypeChecker::completeLazyVarImplementation, we should check for observers at that point and incorporate calls to the willSet/didSet methods in the lazy var's synthesized setter instead of calling synthesizeTrivialSetter. We should then suppress the error you get when observers are seen on a lazy property, since we know they're now taken care of as part of lazy property synthesis.

@swift-ci
Copy link
Collaborator

swift-ci commented Feb 26, 2018

Comment by Jeff Johnson (JIRA)

Strangely, this pattern currently works

class Yep {
    lazy var foo:String = "Hi"
}
class Sub:Yep {
    override var foo:String {
        willSet {
            print("willSet")
        }
    }
}

even though this pattern doesn't:

class Nope {
    lazy var foo:String = "Hi" {
        willSet {
            print("willSet")
        }
    }
}

@swift-ci
Copy link
Collaborator

swift-ci commented Mar 3, 2018

Comment by Ray Fix (JIRA)

A couple of us are looking at this at an try! Swift workshop in Japan this morning.

@swift-ci
Copy link
Collaborator

swift-ci commented Apr 30, 2018

Comment by Mohit Athwani (JIRA)

Going to try working on this as my starter bug... Any tips from you guys?

@swift-ci
Copy link
Collaborator

swift-ci commented Jul 31, 2018

Comment by Mohit Athwani (JIRA)

Is any body still working on this? Should I start looking into it?

@theblixguy
Copy link
Collaborator

theblixguy commented Apr 15, 2019

PR here: #24043

This type checks now, but causes an issue with DI. I don't think it's a DI problem, I guess I messed up something in the type checker, so need to look into that.

@belkadan
Copy link
Contributor

belkadan commented Jul 27, 2019

@theblixguy, are you still working on this one?

(I'm checking in on all the StarterBugs that haven't been touched in over a month; it's totally fine if you just haven't had time but still want to keep it.)

@theblixguy
Copy link
Collaborator

theblixguy commented Jul 27, 2019

I have a PR open (link above) - the only thing left to do is to figure out why DefiniteInitialization is complaining about the lazy property not being initialised when you add an observer. I’ll try to figure out today/tomorrow but I’m not very familiar with SIL so some help would be appreciated!

@theblixguy
Copy link
Collaborator

theblixguy commented Jul 27, 2019

Hmm it seems like that DI issue is gone, but there's still an issue with SILGen. @slavapestov since you recently did some work on lazy properties, do you mind having a look at my PR? Thank you!

@theblixguy
Copy link
Collaborator

theblixguy commented Apr 21, 2020

Decided to revisit this and I’ve now got it working: #31097

@theblixguy
Copy link
Collaborator

theblixguy commented Apr 29, 2020

Fixed on master. @jckarter please verify using the next available snapshot!

@jckarter
Copy link
Member Author

jckarter commented Jun 9, 2020

@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
Projects
None yet
Development

No branches or pull requests

4 participants