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-3174] #selector shouldn't look at local variable names #45762

Open
mattneub opened this issue Nov 10, 2016 · 13 comments
Open

[SR-3174] #selector shouldn't look at local variable names #45762

mattneub opened this issue Nov 10, 2016 · 13 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself good first issue Good for newcomers parser Area → compiler: The legacy C++ parser

Comments

@mattneub
Copy link

Previous ID SR-3174
Radar None
Original Reporter @mattneub
Type Bug
Status In Progress
Resolution
Environment

Xcode Version 8.1 (8B62)

Additional Detail from JIRA
Votes 1
Component/s Compiler
Labels Bug, Parser, StarterBug
Assignee None
Priority Medium

md5: 7beceac384430a3c78337307d536dc3e

Issue Description:

This doesn't compile:

func test() {
    let cancel = UIBarButtonItem(barButtonSystemItem: .cancel, target: self, action: #selector(cancel))
}
func cancel() {}

The compiler complains that the name `cancel` is the same as the name of this local variable ("variable used within its own initial value"). But come now, surely the compiler should realize that there is not a chance in the world that I mean the local variable. A local variable can't even_be_ a selector.

Possibly related to https://bugs.swift.org/browse/SR-1038, I can't quite tell.

@belkadan
Copy link
Contributor

Basic fix: jump out of any local scopes before searching.

Full fix: ignore locals when searching. This handles nested classes too.

@swift-ci
Copy link
Collaborator

Comment by Daniel Martín (JIRA)

I'll take a look at this one.

@jtbandes
Copy link
Collaborator

Seems very much related to https://bugs.swift.org/browse/SR-880.

@swift-ci
Copy link
Collaborator

swift-ci commented Dec 6, 2016

Comment by Daniel Martín (JIRA)

From what I see, when the parser encounters "let cancel =", it stores "cancel" inside the "DisabledVars" parser structure so that something like "let cancel = cancel" is rejected if it is found later. This apparently is causing that the "cancel" identifier inside the "#selector" is also rejected. Maybe we should remove the variables from "DisabledVars" just before parsing the selector, and restore them after the selector has been parsed. This would avoid the parser error, if I'm not mistaken.

Is it a reasonable approach? @belkadan

@swift-ci
Copy link
Collaborator

Comment by Pawel Szot (JIRA)

I'm not sure if this should be considered bug.
Currently #selector parser seems to accept any expressions, so something like this works:

let local = Foo()
_ = #selector(local.method)

In this case local variable will be used.

I guess we could do some magic if expression is DeclRef, but that would be similar to:

func foo() {}
func bar() {
    let foo = 1
    foo() // call method foo because local foo is not callable
}

@belkadan
Copy link
Contributor

I think your example is valid, Pawel, because it's referring to a method of a particular local variable. It's only the top-level component I was suggesting to treat specially.

@swift-ci
Copy link
Collaborator

swift-ci commented May 9, 2017

Comment by Roman Blum (JIRA)

I'm currently into this issue right now and as a newbie I'd be glad for some guidances.

Given the following code:

import Foundation

class Hello: NSObject {
  func test() {
    let cancel = #selector(cancel)
  }

  @objc func cancel() { }
}

As proposed by dmc (JIRA User), removing the variables from "DisabledVars" before parsing the selector raises errors in TypeCheckDecl.cpp.

/Users/roman/Downloads/swift-tests/main.swift:5:9: error: 'cancel' used within its own type
    let cancel = #selector(cancel)
        ^
/Users/roman/Downloads/swift-tests/main.swift:5:9: error: could not infer type for 'cancel'
    let cancel = #selector(cancel)
        ^

By changing the selector to "#selector(self.cancel)", everything works fine.

@swift-ci
Copy link
Collaborator

swift-ci commented May 9, 2017

Comment by Roman Blum (JIRA)

Like I said before, when I ignore DisabledVars in the Parser (only when it's a selector), the error gets propagated to the TypeChecker.

After further investigation, the problem arises in NameLookup.cpp.

if (auto *AFD = dyn_cast<AbstractFunctionDecl>(DC)) {
[...]
  namelookup::FindLocalVal localVal(SM, Loc, Consumer);
  localVal.visit(AFD->getBody());
  if (!Results.empty())
    return;
[...]

After the call of "visit", "Results" is not empty anymore because it finds the "cancel" variable in the lookup. However, what we are really looking for, is the cancel-Method in the class "Hello" and not the variable in the method test.

@belkadan Do you have any suggestions if I have to tackle this problem in the NameLookup or is there a way to solve the issue earlier in the Parser?

@belkadan
Copy link
Contributor

belkadan commented May 9, 2017

The Parser is only trying to resolve local names. I think I just wouldn't bother having it resolve this one at all if there's only one component in the selector expression. Then the type-checker can do one of the things I originally said.

@swift-ci
Copy link
Collaborator

swift-ci commented May 9, 2017

Comment by Roman Blum (JIRA)

Thanks for your reply, @belkadan

Edit: Ignore what I wrote before in this post, I just found a possible and promising solution, PR incoming soon. 🙂

@swift-ci
Copy link
Collaborator

Comment by Brian (JIRA)

It'd be great to extend this fix to `#keyPath` rmnblm (JIRA User). I run into this a lot with JSON importing.

@swift-ci
Copy link
Collaborator

Comment by Roman Blum (JIRA)

Hey KingOfBrian (JIRA User), I think you should make a separate issue for that to keep things isolated from each other. As soon as you created the issue, you can tag me and I'll assign myself to the issue to work on it, what do you think @belkadan?

PS: I submitted my PR for this issue here.

@mattneub
Copy link
Author

bugreporter.apple.com has now rejected this bug report (radar 24852794). But I still think I'm right. For example:

let g = MyRecognizer(target: self, action: #selector(grFired)) // compile error
let grFired = "howdy"

There's a compile error because grFired is also a local variable. But as I object on bugreporter, SE-0022 says "The subexpression of the #selector expression must be a reference to an objc method". So the compiler should not consider local variables as a possible candidate; the spec implies that objc methods are the only candidates of interest.

I recognize that I am probably defeated here, but I wanted my objection entered into the record.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
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 good first issue Good for newcomers parser Area → compiler: The legacy C++ parser
Projects
None yet
Development

No branches or pull requests

4 participants