Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 12, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 36 additions & 30 deletions Sources/SourceKitLSP/SourceKitLSPServer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2126,27 +2126,44 @@ extension SourceKitLSPServer {
callableUsrs += index.occurrences(ofUSR: data.usr, roles: .overrideOf).flatMap { occurrence in
occurrence.relations.filter { $0.roles.contains(.overrideOf) }.map(\.symbol.usr)
}
// callOccurrences are all the places that any of the USRs in callableUsrs is called.
// We also load the `calledBy` roles to get the method that contains the reference to this call.
Copy link
Contributor

@bnbarham bnbarham Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let callOccurrences = callableUsrs.flatMap { index.occurrences(ofUSR: $0, roles: .calledBy) }
let calls = callOccurrences.flatMap { occurrence -> [CallHierarchyIncomingCall] in
guard let location = indexToLSPLocation(occurrence.location) else {
return []

// Maps functions that call a USR in `callableUSRs` to all the called occurrences of `callableUSRs` within the
// function. If a function `foo` calls `bar` multiple times, `callersToCalls[foo]` will contain two call
// `SymbolOccurrence`s.
// This way, we can group multiple calls to `bar` within `foo` to a single item with multiple `fromRanges`.
var callersToCalls: [Symbol: [SymbolOccurrence]] = [:]

for call in callOccurrences {
// Callers are all `calledBy` relations of a call to a USR in `callableUsrs`, ie. all the functions that contain a
// call to a USR in callableUSRs. In practice, this should always be a single item.
let callers = call.relations.filter { $0.roles.contains(.calledBy) }.map(\.symbol)
for caller in callers {
callersToCalls[caller, default: []].append(call)
}
return occurrence.relations.filter { $0.symbol.kind.isCallable }
.map { related in
// Resolve the caller's definition to find its location
let definition = index.primaryDefinitionOrDeclarationOccurrence(ofUSR: related.symbol.usr)
let definitionSymbolLocation = definition?.location
let definitionLocation = definitionSymbolLocation.flatMap(indexToLSPLocation)

return CallHierarchyIncomingCall(
from: indexToLSPCallHierarchyItem(
symbol: related.symbol,
containerName: definition?.containerName,
location: definitionLocation ?? location // Use occurrence location as fallback
),
fromRanges: [location.range]
)
}
}

let calls = callersToCalls.compactMap { (caller: Symbol, calls: [SymbolOccurrence]) -> CallHierarchyIncomingCall? in
// Resolve the caller's definition to find its location
let definition = index.primaryDefinitionOrDeclarationOccurrence(ofUSR: caller.usr)
let definitionSymbolLocation = definition?.location
let definitionLocation = definitionSymbolLocation.flatMap(indexToLSPLocation)

let locations = calls.compactMap { indexToLSPLocation($0.location) }.sorted()
guard !locations.isEmpty else {
return nil
}

return CallHierarchyIncomingCall(
from: indexToLSPCallHierarchyItem(
symbol: caller,
containerName: definition?.containerName,
location: definitionLocation ?? locations.first!
),
fromRanges: locations.map(\.range)
)
}
return calls.sorted(by: { $0.from.name < $1.from.name })
}
Expand Down Expand Up @@ -2455,17 +2472,6 @@ extension IndexSymbolKind {
return .null
}
}

var isCallable: Bool {
switch self {
case .function, .instanceMethod, .classMethod, .staticMethod, .constructor, .destructor, .conversionFunction:
return true
case .unknown, .module, .namespace, .namespaceAlias, .macro, .enum, .struct, .protocol, .extension, .union,
.typealias, .field, .enumConstant, .parameter, .using, .concept, .commentTag, .variable, .instanceProperty,
.class, .staticProperty, .classProperty:
return false
}
}
}

extension SymbolOccurrence {
Expand Down
57 changes: 36 additions & 21 deletions Tests/SourceKitLSPTests/CallHierarchyTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,9 +275,15 @@ final class CallHierarchyTests: XCTestCase {
"""
func 1️⃣foo() {}

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

func 4️⃣testFunc() {
_ = 5️⃣testVar
}
"""
)
Expand Down Expand Up @@ -310,6 +316,31 @@ final class CallHierarchyTests: XCTestCase {
)
]
)

let testVarItem = try XCTUnwrap(calls?.first?.from)

let callsToTestVar = try await project.testClient.send(CallHierarchyIncomingCallsRequest(item: testVarItem))
XCTAssertEqual(
callsToTestVar,
[
CallHierarchyIncomingCall(
from: CallHierarchyItem(
name: "testFunc()",
kind: .function,
tags: nil,
detail: nil,
uri: project.fileURI,
range: Range(project.positions["4️⃣"]),
selectionRange: Range(project.positions["4️⃣"]),
data: .dictionary([
"usr": .string("s:4test0A4FuncyyF"),
"uri": .string(project.fileURI.stringValue),
])
),
fromRanges: [Range(project.positions["5️⃣"])]
)
]
)
}

func testIncomingCallHierarchyShowsAccessToVariables() async throws {
Expand Down Expand Up @@ -348,24 +379,8 @@ final class CallHierarchyTests: XCTestCase {
"uri": .string(project.fileURI.stringValue),
])
),
fromRanges: [Range(project.positions["3️⃣"])]
),
CallHierarchyIncomingCall(
from: CallHierarchyItem(
name: "testFunc()",
kind: .function,
tags: nil,
detail: nil,
uri: project.fileURI,
range: Range(project.positions["2️⃣"]),
selectionRange: Range(project.positions["2️⃣"]),
data: .dictionary([
"usr": .string("s:4test0A4FuncyyF"),
"uri": .string(project.fileURI.stringValue),
])
),
fromRanges: [Range(project.positions["4️⃣"])]
),
fromRanges: [Range(project.positions["3️⃣"]), Range(project.positions["4️⃣"])]
)
]
)
}
Expand Down