[Parse] [Sema] [SR-3174] Ignore local variables in selector

[rmnblm]

Declaring a variable and assigning a selector that refers to a method with the same name as the variable is currently not possible. The compiler will complain with an error message.

func foo() {
  let bar = #selector(bar) // Error: Variable used within its own initial value
}

func bar() { }

However, the compiler should realise that not the variable, but instead, the method bar() was meant to be used. A local variable can't even be a selector.

The subexpression of the #selector expression must be a reference to an objc method. SE-0022

Again, the compiler will complain with an appropriate error message.

func foo() {
  let t = { 
    print("Swift is awesome!")
  }
  let bar = #selector(t) // Error: Argument of '#selector' cannot refer to variable 't'
}

This pull request changes this behaviour and allows the usage of the same name in a selector.

func foo() {
  let bar = #selector(bar) // All good, no error anymore
}

func bar() { }

The compiler does this now in two steps: Firstly, the parser ignores the DisabledVars property if a selector is currently being parsed. In addition, while the type checker walks the AST and stumbles across an ObjCSelectorExpr, it ignores local variables when trying to resolve an UnresolvedDeclRefExpr.

Resolves SR-3174.

Can you take a look at it, @DougGregor or @jrose-apple? 🙏

[CodaFi]

Can you leave a short comment explaining why selector references are exempt from circularity checks.

[rmnblm]

I just added a short comment in the code (see latest commit).

[CodaFi]

In addition to these tests, please add one with nested declarations and another that uses a top-level selector.

[CodaFi]

@swift-ci please smoke test

[rmnblm]

@CodaFi I added another test, you can smoke test again.

[CodaFi]

@swift-ci please smoke test

[jrose-apple]

Thanks for working on this! This is what I called the "basic fix", which disallows this currently-legal code:

import Foundation
func test(_ obj: NSObject) {
  let x = #selector(obj.copy)
  print(x)
}
test(NSObject())

As such, we'd need to limit the change to Swift 4 mode. You can test TC.getLangOptions().isSwiftVersion3() for that.

[jrose-apple]

What does being a method have to do with ignoring local variables?

[rmnblm]

Well, correct me if I'm wrong but if it's a getter/setter selector, ignoring local variables doesn't make sense so there is only the method selector left where conflicts can occur with local variable names.

[rmnblm]

To limit the change to Swift 4, should I wrap lines 821 to 828 with, would this be sufficient?

if !TC.getLangOptions().isSwiftVersion3() { 
  ...
}

[jrose-apple]

Local variables still won't have selectors.

import Foundation
class Foo: NSObject {
  @objc var value: Int = 0
  func test() {
    var value = 42
    print(#selector(getter:value))
  }
}

[jrose-apple]

And yes, that seems like the right way to limit it to Swift 4.

[jrose-apple]

@DougGregor ping

[rmnblm]

@jrose-apple I tried your currently-legal code that is now disallowed due to this PR (copied it to the end of the file test/expr/unary/selector/selector.swift) on the master branch and it gives me the same error, am I missing something here? (The code runs fine in a playground.)

In the screenshot below you can see the command I executed.

[jrose-apple]

Ah, that test probably runs against our fake version of the Mac SDK (in test/Inputs/clang-importer-sdk/), which wouldn't have all of NSObject's methods declared. It doesn't really matter which method you pick here; the point is it's accessing something on a local variable.

[CodaFi]

@rmnblm I'm going to close this out since it's been two years, but I think the idea you've got is correct and can probably be retailored to fix this SR. If you're interested in picking this back up, either reopen this pull request after a rebase or open a new one. I'm also resetting the assignee for this SR so somebody else can take a look.

Thanks for the work you've done so far. Hopefully it can help another contributor out in the future.