Shorthand optional binding has no reference to original variable

[SimplyDanny]

Describe the bug
The new shorthand syntax for optional binding in if, guard and while conditions does not provide a reference to the original bound variable. This is also not the case for the extended syntax let i = i but with it we are able to jump to the original via the variable reference on the right-hand side.

Steps To Reproduce
Run swift-ide-test -print-indexed-symbols -source-filename for the file main.swift with the content

private class C {
    let i: Int? = nil

    func f() -> Int {
        if let i { return i }
        if let i = i { return i }
        return 0
    }
}

We receive the following output:

1:15 | class/Swift | C | s:14swift_ide_test1C33_655A52213DC570E6117BF12D4C61C186LLC | Def | rel: 0
2:9 | instance-property/Swift | i | s:14swift_ide_test1C33_655A52213DC570E6117BF12D4C61C186LLC1iSiSgvp | Def,RelChild | rel: 1
  RelChild | class/Swift | C | s:14swift_ide_test1C33_655A52213DC570E6117BF12D4C61C186LLC
2:9 | instance-method/acc-get/Swift | getter:i | s:14swift_ide_test1C33_655A52213DC570E6117BF12D4C61C186LLC1iSiSgvg | Def,Impl,RelChild,RelAcc | rel: 1
  RelChild,RelAcc | instance-property/Swift | i | s:14swift_ide_test1C33_655A52213DC570E6117BF12D4C61C186LLC1iSiSgvp
2:9 | instance-method/acc-set/Swift | setter:i | s:14swift_ide_test1C33_655A52213DC570E6117BF12D4C61C186LLC1iSiSgvs | Def,Impl,RelChild,RelAcc | rel: 1
  RelChild,RelAcc | instance-property/Swift | i | s:14swift_ide_test1C33_655A52213DC570E6117BF12D4C61C186LLC1iSiSgvp
2:12 | struct/Swift | Int | s:Si | Ref,RelCont | rel: 1
  RelCont | instance-property/Swift | i | s:14swift_ide_test1C33_655A52213DC570E6117BF12D4C61C186LLC1iSiSgvp
4:10 | instance-method/Swift | f() | s:14swift_ide_test1C33_655A52213DC570E6117BF12D4C61C186LLC1fSiyF | Def,Dyn,RelChild | rel: 1
  RelChild | class/Swift | C | s:14swift_ide_test1C33_655A52213DC570E6117BF12D4C61C186LLC
4:17 | struct/Swift | Int | s:Si | Ref,RelCont | rel: 1
  RelCont | instance-method/Swift | f() | s:14swift_ide_test1C33_655A52213DC570E6117BF12D4C61C186LLC1fSiyF
6:20 | instance-property/Swift | i | s:14swift_ide_test1C33_655A52213DC570E6117BF12D4C61C186LLC1iSiSgvp | Ref,Read,RelCont | rel: 1
  RelCont | instance-method/Swift | f() | s:14swift_ide_test1C33_655A52213DC570E6117BF12D4C61C186LLC1fSiyF
6:20 | instance-method/acc-get/Swift | getter:i | s:14swift_ide_test1C33_655A52213DC570E6117BF12D4C61C186LLC1iSiSgvg | Ref,Call,Impl,RelCall,RelCont | rel: 1
  RelCall,RelCont | instance-method/Swift | f() | s:14swift_ide_test1C33_655A52213DC570E6117BF12D4C61C186LLC1fSiyF
1:15 | constructor/Swift | init() | s:14swift_ide_test1C33_655A52213DC570E6117BF12D4C61C186LLCADycfc | Def,Impl,RelChild | rel: 1
  RelChild | class/Swift | C | s:14swift_ide_test1C33_655A52213DC570E6117BF12D4C61C186LLC

Notice that while the binding let i = i references the original class property i this is not the case for the binding let i.

This is an issue in editors where a "Jump to Definition" doesn't work anymore. Even more problematic, however, it is for linter tools which are now unable to answer questions like "Are all declared variables used somewhere?". I especially want to mention SwiftLint, the unused_declaration rule of which is currently not working.

Expected behavior
A redeclared variable should have a reference to its original.

Writing this sentence I feel like this is more a feature request than a bug. It would also be helpful in code like

func f(a: Int) {
   // ...
   var a = 1
   // ...
}

where "Jump to Definition" on var a would jump to the function parameter.

Environment

• macOS 12.6
• Xcode 14.0.1 14A400
• Swift 5.7 (swiftlang-5.7.0.127.4 clang-1400.0.29.50)

[hamishknight]

cc @bnbarham

[bnbarham]

rdar://100006415

This is also the case for closure capture lists, I suspect it's just very uncommon for that to be anything other than self 😅.

If a shorthand if-let or closure capture shadows a non-local variable (including parameter when there's no explicit label), we'll need to add that local + a relation to the original variable.

@SimplyDanny what were you looking for in:

func f(a: Int) {
   // ...
   var a = 1
   // ...
}

? I wouldn't expect jump to def from the a in var a = 1 to jump to the parameter a in that case, but would for:

func f(a: Int?) {
  if let a { ... }
}

(this is the no explicit label case I'm referring to above)

[SimplyDanny]

@SimplyDanny what were you looking for in:

func f(a: Int) {
   // ...
   var a = 1
   // ...
}

? I wouldn't expect jump to def from the a in var a = 1 to jump to the parameter a in that case [...].

Well, I was thinking about exactly that. From the a in var a = 1 we can jump to the original now shadowed variable. But yeah, I missed that such a declaration is a totally new variable which can even have a different type. So it might be confusing if there is a reference from a variable of type String to one of type Int. For the question "Is this variable shadowing another one?" it would be helpful, however. What are your arguments against it?

If a shorthand if-let or closure capture shadows a non-local variable (including parameter when there's no explicit label), we'll need to add that local + a relation to the original variable.

I don't understand the difference between a parameter with or without a label. Why does it need special consideration? In both cases I would expect a reference to the parameter name (not the argument label).

[bnbarham]

For the question "Is this variable shadowing another one?" it would be helpful, however. What are your arguments against it?

We could add a relation for the variable being shadowed in general, I can see that being useful. I just wouldn't expect it to show up in either help or jump to definition.

I don't understand the difference between a parameter with or without a label. Why does it need special consideration? In both cases I would expect a reference to the parameter name (not the argument label).

We don't index locals by default (though it is an option). But we do index parameters without labels as they can be referenced externally in the call site. So for the default case (of not indexing locals), we still need to add this information to the index, despite it being a local.

[SimplyDanny]

Both make sense to me now. Thank you!

[ileitch]

This is fixed in 5.8 betas, I assume by 7b4ab8d. Thanks, @bnbarham!

[bnbarham]

Oh thanks for reminding me @ileitch! FWIW I didn't fix this the way I commented on here. It was indeed 7b4ab8d.

Indexing now ignores the if let binding and just treats all references as references to the shadowed-decl. This is technically not "correct", but I think works for most cases since the decl location == the reference location on the binding anyway.

@SimplyDanny does this work for your purposes?

[SimplyDanny]

Yes, works very well now for my use cases.

Thank you, @bnbarham!