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

[mattneub]

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]

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

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

[swift-ci]

Comment by Daniel Martín (JIRA)

I'll take a look at this one.

[jtbandes]

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

[swift-ci]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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.