Don’t repeat a function in incomingCalls if it contains multiple calls to the same function

[ahoppen]

Eg. if we have the following, and we get the call hierarchy of foo, we only want to show bar once, with multiple fromRanges instead of having two entries for bar in the call hierarchy.

func foo() {}
func bar() {
  foo()
  foo()
}

[bnbarham]

Sort of unrelated to this PR, but it'd be good to add a bunch of test cases for properties:

struct Foo {
  var member: Int = {
    return bar()
  }();

  func bar() {}
}

Call hierarchy for bar probably should give member there. member isn't really the caller, but... I think it's what people would expect to see in the UI. Though probably want to extend that to something accessing member, as I wouldn't necessarily say the call hierarchy should continue after member (or if it did, it should go to the init).

Also just a regular getter:

struct Foo {
  var member: Int {
    return bar()
  }

  func bar() {}
}

Plus explicit getter/setter/etc:

struct Foo {
  var member: Int {
    get {
      return bar()
    }
  }

  func bar() {}
}

Also CC @hamishknight as he's been looking at this a bit recently (both to comment on the above and also a review of this PR in general).

[ahoppen]

We do already have a test for valuable getters FWIW: https://github.com/apple/sourcekit-lsp/blob/4ed62392e7b4f889d44c09e4d6857a0eb28975c3/Tests/SourceKitLSPTests/CallHierarchyTests.swift#L414-L452

I agree that the rest would be good to test

[bnbarham]

Extending that with a use of foo seems like a good thing to do as well.

[hamishknight]

I think it would be better to check $0.roles here, with swiftlang/swift#72930 it ought to be sufficient to check for .containedBy, which would handle the case where the parent is a property (note that isCallable would be false in that case). Though for compatibility with older toolchains it may be better to check if the role contains .calledBy, and if that's empty, fallback to checking for .containedBy.

[ahoppen]

Good suggestion with checking $0.roles.

There is a subtle difference between calledBy and containedBy relations.

func foo() {}

var testVar: Int {
  get {
    let myVar = foo()
    return 2
  }
}

func test() {
  _ = testVar
}

The call to foo in let myVar = foo() is called by s:4test0A3VarSivg (demangled test.testVar.getter : Swift.Int, display name getter:testVar) while it is contained by s:4test0A3VarSivg02myB0L_ytvp (demangled myVar #1 : () in test.testVar.getter : Swift.Int / display name testVar). But s:4test0A3VarSivg02myB0L_ytvp doesn’t have any callers while s:4test0A3VarSivg does show the call inside test(). So I’ll go with just checking the calledBy relation. Or do you see any advantage of containedBy over calledBy?

[hamishknight]

while it is contained by s:4test0A3VarSivg02myB0L_ytvp

That should have been fixed by swiftlang/swift#72930, is it still an issue? The main advantage of containedBy is that it works for cases like:

struct Foo {
  var member: Int = {
    return bar()
  }()

  func bar() {}
}

Even though member isn't a callable symbol, it probably makes sense to consider it the caller (as we don't really have anything else to anchor it on). Generally, I think the right check is probably "called by, or container as a fallback"

[ahoppen]

Oh, right, sorry, you mentioned that. I was using an open source toolchain snapshot.

Let me try again once use have a toolchain snapshot with swiftlang/swift#72930 because I can’t be bothered to build a toolchain myself.

[bnbarham]

Should we stop the traversal if we use the container instead? Does it make sense to continue from member there? Or should we have a calledBy to the init?

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test Linux