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-4865] target self should be illegal in property constructor #47442

Closed
mattneub opened this issue May 11, 2017 · 13 comments
Closed

[SR-4865] target self should be illegal in property constructor #47442

mattneub opened this issue May 11, 2017 · 13 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself diagnostics QoI Bug: Diagnostics Quality of Implementation duplicate Resolution: Duplicates another issue expressions Feature: expressions identifiers Feature: Identifiers missing warning Bug: Missing warning self Feature → expressions: The 'self' expression swift 5.6 type checker Area → compiler: Semantic analysis

Comments

@mattneub
Copy link

mattneub commented May 11, 2017

Previous ID SR-4865
Radar None
Original Reporter @mattneub
Type Bug
Status Resolved
Resolution Duplicate

Attachment: Download

Environment

Xcode 8.3.2

Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Bug
Assignee None
Priority Medium

md5: d3c2dbfe3cffd99ed51448d61dd795be

duplicates:

Issue Description:

This is legal in an instance property declaration:

    let listen : UIBarButtonItem = {
        let b = UIBarButtonItem(barButtonSystemItem: .play, target: self, action: #selector(doListen))
        return b
    }()

It shouldn't be. There is no self at this point. The code compiles but it isn't going to work, so it shouldn't compile at all.

I see a lot of people on Stack Overflow get caught by this sort of thing, but this is the first time I've made the same mistake myself. The usual context is in configuring a Timer.

@belkadan
Copy link
Contributor

self in this case refers to the class, not an instance. This is probably not what the user intended.

@mattneub
Copy link
Author

mattneub commented May 12, 2017

@belkadan It's not so simple. The same code works fine if this is a UIButton instead of a UIBarButtonItem. And there's no class method here, so why don't we crash when the button is tapped?

Moreover, the same code works fine if we change let to lazy var.

These distinctions are very confusing to the beginner (and even the not-so-beginner). If the compiler knows the difference between them, it should at least issue a warning.

Here's a test example (assume this is the root view controller of a navigation controller):

    let bbi1 : UIBarButtonItem = {
        let b = UIBarButtonItem(barButtonSystemItem: .play, target: self, action: #selector(doButton))
        return b
    }()

    lazy var bbi2 : UIBarButtonItem = {
        let b = UIBarButtonItem(barButtonSystemItem: .play, target: self, action: #selector(doButton))
        return b
    }()
    
    let button : UIButton = {
        let b = UIButton(type: .system)
        b.addTarget(self, action: #selector(doButton), for: .touchUpInside)
        b.setTitle("Button", for: .normal)
        b.sizeToFit()
        return b
    }()

    override func viewDidLoad() {
        super.viewDidLoad()
        self.button.frame.origin = CGPoint(x: 100, y: 100)
        self.view.addSubview(self.button)
        self.navigationItem.rightBarButtonItems = [self.bbi1, self.bbi2]
    }

    func doButton ( _ sender : Any ) {
        print("button")
    }

The first of those buttons doesn't do anything when tapped. The other two work fine. I'm not sure I know why. If the compiler knows, it should help out here.

I enclose that code in a test project. TargetSelfTest.zip

@belkadan
Copy link
Contributor

Yes, the whole area is messed up. var vs. lazy var and "using a closure" vs. "not using a closure" all affect this, when it should be consistent across everything (rejected for var and probably allowed for lazy var).

@mattneub
Copy link
Author

@belkadan Yep. Granted all that, I still don't get why it doesn't work for UIBarButtonItem (tapping does nothing) and does work for a UIButton (tapping calls the selector). A selector is a selector, a target is a target, `self` is `self`, `let` is `let`, define-and-call initialization is define-and-call initialization, so what on earth is happening here? Don't answer that, I just wanted to make sure that's in the mix somewhere when we come to tackle this from the compiler's point of view.

@dennisweissmann
Copy link

dennisweissmann commented Feb 8, 2018

I just ran into this issue as well and this is definitely unexpected. I didn't even use a closure, just

private let rightArrow = UIBarButtonItem(title: ">", style: .plain, target: self, action: #selector(rightArrowPressed(_🙂))

I would expect the code to not compile but it compiles just fine and doesn't work.

@mattneub
Copy link
Author

mattneub commented Jun 3, 2018

Caught another one: https://stackoverflow.com/questions/50662302/adding-a-target-to-a-button-programmatically-throws-an-error-unrecognized-selec

Interestingly, he had the issue with a UIButton.

@swift-ci
Copy link
Collaborator

Comment by Yu (JIRA)

This will crash on iOS 8, but works on iOS 9+

@mattneub
Copy link
Author

Here's another one: https://stackoverflow.com/questions/63892806/button-to-instanciate-viewcontroller-is-not-working-after-hiding-it I really wish something would be done about this. Except that then I would not get numerous points on Stack Overflow, answering this over and over, as it catches people day after day... By the way, this was a particularly interesting one; the button works for a while and then stops talking to the target. I have no idea why.

@mattneub
Copy link
Author

Also I would be curious to know why this is marked "resolved", as it is far from resolved. It catches people every day.

@mattneub
Copy link
Author

mattneub commented Sep 16, 2020

I enclose a project that elicits the problem nicely.

TroubleShootingProject.zip

To see the problem, you have to do something slightly elaborate: click in the rectangle, then click on the Tap Me button. A presented view controller appears with chevron button. That button is broken: tapping it does not call its target-action method.

That is because the target-action was set in the instance property initializer. If you change let backButton to lazy var backButton, the problem goes away.

What I am saying here is that the Swift compiler should warn the user that the reference to self in the line v.addTarget(self, action: #selector(backButtonTapped), for: .touchUpInside) is not necessarily going to behave as expected. You cannot refer successfully to self in a nonlazy instance property define-and-call method, and the compiler should discourage you from trying to do so.

@nolanw
Copy link

nolanw commented Dec 14, 2020

I shipped code a few months ago that inadvertently does this, and it seemed to work ok until recently. I learned a bit about what's going on, though I still don't fully understand, and I thought I'd share.

We had something like this modified example from upthread:

class ViewController: UIViewController {
  let button : UIButton = {
    let b = UIButton(type: .system)
    // self below is really self.self aka unbound method UIViewController.`self`
    b.addTarget(self, action: #selector(doButton), for: .touchUpInside)
    return b
  }()

  @objc func doButton() {}
}

The target ends up being the unbound instance method ViewController.`self` (inherited from NSObject). The unbound method gets bridged to a __SwiftValue and passed to UIControl (its debug description is (Function), in case that comes up while poking in the debugger). Since UIControl doesn't retain its targets, the __SwiftValue deallocates shortly, and UIControl's weak reference zeroes out. Now it has a target of nil, meaning "check the responder chain". Since the view controller is in the button's responder chain, everything works out when the button is tapped.

My understanding evaporates when going to iOS's Settings app > Accessibility > Keyboard > Full Keyboard Access and enabling "Full Keyboard Access". Some of our users have this setting enabled and rightfully complained that the button no longer responded to taps. We eventually figured out that setting as the key environmental difference, and it led me to this bug. (This is on iOS 14.2, and it is reproducible both on device and in the simulator.) The __SwiftValue is still deallocated in this scenario, and the button's allTargets contains only an instance of NSNull, so I'm not sure what's going on. [edit: While in the nonfunctional state, enabling VoiceOver and focusing the button (then turning VoiceOver back off) makes it responds to taps again.]

I suggest that it is rarely intentional to access an unbound instance method named self from within a let property initializer, and emitting a warning would be appreciated. If the access is intentional, spelling it with backticks like `self` seems like a reasonable way to quash the warning.

Also, changing the action to #selector(self.doButton) reveals the issue, as the compiler will emit an error "Instance member 'self' cannot be used on type 'ViewController'; did you mean to use a value of this type instead?".

@swift-ci
Copy link
Collaborator

swift-ci commented Apr 8, 2021

Comment by Jack March (JIRA)

@nolanw in my testing – from manually setting a first responder using becomeFirstResponder() – if there is a first responder, then that responder chain is used, otherwise it uses the responder chain of the sender. I would suggest using lldb + a private API to find the first responder:

po [[[UIApplication sharedApplication] keyWindow] performSelector:@selector(firstResponder)];

And seeing if it's different when the user has full keyboard access enabled. I couldn't reproduce any differences with full keyboard access in my test project so not sure if this will help, let me know either way!

@keith
Copy link
Collaborator

keith commented Feb 23, 2022

Note that there is a swiftlint rule for many cases of this that you can enable now before the warning lands in swift 5.6 https://realm.github.io/SwiftLint/self_in_property_initialization.html

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@AnthonyLatsis AnthonyLatsis added duplicate Resolution: Duplicates another issue type checker Area → compiler: Semantic analysis expressions Feature: expressions missing warning Bug: Missing warning identifiers Feature: Identifiers self Feature → expressions: The 'self' expression diagnostics QoI Bug: Diagnostics Quality of Implementation swift 3.0 swift 5.6 and removed swift 3.0 labels May 2, 2023
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself diagnostics QoI Bug: Diagnostics Quality of Implementation duplicate Resolution: Duplicates another issue expressions Feature: expressions identifiers Feature: Identifiers missing warning Bug: Missing warning self Feature → expressions: The 'self' expression swift 5.6 type checker Area → compiler: Semantic analysis
Projects
None yet
Development

No branches or pull requests

7 participants