Show overriden functions when performing jump-to-definition on a dynamic call #977

ahoppen · 2023-11-23T04:02:01Z

Fixes #809
rdar://114864256

ahoppen · 2023-11-23T16:01:46Z

@swift-ci Please test

ahoppen · 2023-11-27T23:37:46Z

@swift-ci Please test

ahoppen · 2023-11-27T23:57:42Z

@swift-ci Please test Windows

Sources/SourceKitLSP/SourceKitServer.swift

bnbarham · 2023-11-28T00:46:25Z

Sources/SourceKitLSP/SourceKitServer.swift

      logger.info("performing indexed jump-to-def with usr \(usr)")
      var occurs = index.occurrences(ofUSR: usr, roles: [.definition])
      if occurs.isEmpty {
        occurs = index.occurrences(ofUSR: usr, roles: [.declaration])
      }
+      if symbolDetails.isDynamic {
+        occurs += occurs.flatMap {
+          index.occurrences(relatedToUSR: $0.symbol.usr, roles: .overrideOf)


Ideally we should filter by receivers here, ie.

Get the set of types to filter (receivers + their subtypes)

For each override check if it's a child of one of the types in the filter

Added a filter by receiver types.

bnbarham · 2023-11-28T00:47:50Z

Sources/SourceKitLSP/SourceKitServer.swift

@@ -1725,12 +1725,18 @@ extension SourceKitServer {
    }

    let resolved = self.extractIndexedOccurrences(symbols: symbols, index: index, useLocalFallback: true) {


We could just skip this if the symbol isn't dynamic - the index will be at most up to date as the location in the module, or worse. Though that would change if background indexing was added (or preparation, if we go that route instead).

Ah, I just noticed this is on SourceKitServer and not the language service. Would still be nice to skip for Swift IMO. As for clangd not returning dynamic, that's something we could change (the request we're using is one we specifically added for this use case - https://reviews.llvm.org/D54799). But in the meantime, I'd default to returning overrides if isDynamic isn't set.

Changed it so that we don’t do an index lookup if SymbolRequest returns a location.

ahoppen · 2023-11-29T02:59:12Z

@swift-ci Please test

…mic call Fixes swiftlang#809 rdar://114864256

`extractIndexedOccurences` mostly dealt with how to create a fallback value and we didn’t support fallback values in 3/4 cases. Remove it, simplifying the callers of `extractIndexedOccurances` along the way.

ahoppen · 2023-11-29T18:21:02Z

@swift-ci Please test

ahoppen · 2023-11-29T18:21:16Z

@swift-ci Please test Windows

bnbarham · 2023-11-29T19:23:59Z

Sources/SourceKitLSP/SourceKitServer.swift

+      return []
+    }
+
+    if let symbol = symbols.first, let bestLocalDeclaration = symbol.bestLocalDeclaration, !(symbol.isDynamic ?? false)


Shouldn't we default isDynamic to true so that it's always doing an index lookup (this would be the clang case)?

Yes, you’re right. I added a test case for this.

Sources/SourceKitLSP/SourceKitServer.swift

bnbarham · 2023-11-30T22:15:40Z

Sources/SourceKitLSP/SourceKitServer.swift

+    guard let symbol = symbols.first else {
+      return []
+    }


I realize the old code only handled a single result, but is there any reason not to just handle multiple for all of these?

There is not. I thought we had an issue for this but looks like we don’t. Filed rdar://119163908

bnbarham · 2023-11-30T22:16:42Z

Sources/SourceKitLSP/SourceKitServer.swift

+        // if first resolved location is in `.swiftinterface` file. Use moduleName to return
+        // textual interface


What does "first" here mean?

Oh, it used to refer to the fact that everything was based on symbols.first. Rephrased the comment slightly.

bnbarham · 2023-11-30T22:17:17Z

Sources/SourceKitLSP/SourceKitServer.swift

+      do {
+        return try await languageService.definition(req)
+      } catch {}


Why are we ignoring the error here?

Changed to orLog and added the following comment:

We are on only calling the language service's definition function if your index-based lookup failed. If this fallback request fails, its error is usually not very enlightening. For example the SwiftLanguageServer will always respond with unsupported method. Thus, only log such a failure instead of returning it to the client.

bnbarham · 2023-11-30T22:20:44Z

Sources/SourceKitLSP/SourceKitServer.swift

+        // Only contain overrides that are children of one of the receiver types or their subtypes.
+        return overrides.filter { override in
+          override.relations.contains(where: {
+            $0.roles.contains(.childOf) && transitiveReceiverUsrs.contains($0.symbol.usr)


Suggested change

$0.roles.contains(.childOf) && transitiveReceiverUsrs.contains($0.symbol.usr)

$0.roles.contains(.childOf) && (transitiveReceiverUsrs.isEmpty || transitiveReceiverUsrs.contains($0.symbol.usr))

Just noticed that as well. Changed it slightly so that transitiveReceiverUsrs is nil if not receivers were passed.

ahoppen · 2023-12-05T02:49:50Z

@swift-ci Please test

ahoppen · 2023-12-05T18:03:06Z

@swift-ci Please test

ahoppen requested a review from benlangmuir as a code owner November 23, 2023 04:02

ahoppen requested review from bnbarham and hamishknight November 23, 2023 04:02

hamishknight approved these changes Nov 27, 2023

View reviewed changes

ahoppen force-pushed the ahoppen/jump-to-override branch from 5e89ed3 to a3f7f11 Compare November 27, 2023 23:37

ahoppen changed the title ~~Show overriden functions when performing jump-to-definition on a dynamic call 🚥 #975~~ Show overriden functions when performing jump-to-definition on a dynamic call Nov 27, 2023

bnbarham reviewed Nov 28, 2023

View reviewed changes

ahoppen force-pushed the ahoppen/jump-to-override branch from a3f7f11 to 1ccccc3 Compare November 28, 2023 03:56

ahoppen added 3 commits November 29, 2023 09:02

Show overriden functions when performing jump-to-definition on a dyna…

4052465

…mic call Fixes swiftlang#809 rdar://114864256

Remove extractIndexedOccurences

1ffa825

`extractIndexedOccurences` mostly dealt with how to create a fallback value and we didn’t support fallback values in 3/4 cases. Remove it, simplifying the callers of `extractIndexedOccurances` along the way.

Filter overrides in DefinitionRequest by receiver types

84fdea9

ahoppen force-pushed the ahoppen/jump-to-override branch from 9aa0257 to 84fdea9 Compare November 29, 2023 18:20

bnbarham reviewed Nov 30, 2023

View reviewed changes

Consider all symbol replies from clangd dynamic

c7bd8c9

ahoppen merged commit ed410e6 into swiftlang:main Dec 6, 2023

ahoppen deleted the ahoppen/jump-to-override branch December 6, 2023 00:18

		@@ -1725,12 +1725,18 @@ extension SourceKitServer {
		}

		let resolved = self.extractIndexedOccurrences(symbols: symbols, index: index, useLocalFallback: true) {

		// if first resolved location is in `.swiftinterface` file. Use moduleName to return
		// textual interface

	$0.roles.contains(.childOf) && transitiveReceiverUsrs.contains($0.symbol.usr)
	$0.roles.contains(.childOf) && (transitiveReceiverUsrs.isEmpty \|\| transitiveReceiverUsrs.contains($0.symbol.usr))

Show overriden functions when performing jump-to-definition on a dynamic call #977

Show overriden functions when performing jump-to-definition on a dynamic call #977

Uh oh!

Conversation

ahoppen commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented Nov 23, 2023

Uh oh!

ahoppen commented Nov 27, 2023

Uh oh!

ahoppen commented Nov 27, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Nov 29, 2023

Uh oh!

ahoppen commented Nov 29, 2023

Uh oh!

ahoppen commented Nov 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Dec 5, 2023

Uh oh!

ahoppen commented Dec 5, 2023

Uh oh!

Uh oh!

ahoppen commented Nov 23, 2023 •

edited

Loading