Skip to content
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.

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

OpenDeclarationHyperlinkDetector.detectHyperlinks() handles invalid responses inappropriately, poisoning possibly good results #1169

Open
will-lauer opened this issue Jan 5, 2025 · 3 comments · May be fixed by #1186

Comments

@will-lauer
Copy link

I'm attempting to integrate the Golang LSP (gopls) with eclipse using LSP4E and encountered a problem with hyperlinks. It looks like the current implementation of OpenDeclarationHyperlinkDetector.detectHyperlinks() makes requests for Definitions, Declarations, TypeDefinitions, and Implementations and then merges the results into a single list. Unfortunately, if any of these calls returns an error, the whole call to detectHyperlinks() fails. In the case of gopls, I'm seeing cases when trying to drill down on a function call (named Execute) where Definition and Declaration return good results, but TypeDefinition returns the error "no type definition for Execute" instead of an empty list and Implementations returns the error "Execute is a function, not a method" instead of an empty list.

Looking at this, I think a more appropriate implementation of the merge of the 4 lists of hyperlinks would be to ignore any errors and take only from the successful calls, thus keeping failures from poisoning the overall result.

I'm using version 0.18.12.202408150719 with Eclipse 2024-12 (4.34) and testing with gopls "golang.org/x/tools/gopls v0.17.1"

A simple "hello world" go program that exhibits the problem is

package main

import (
	"fmt"
)

func main() {
	Execute()
}

func Execute() {
	fmt.Println("hello world")
}

when drilling down on Execute in the main function.

@will-lauer
Copy link
Author

As a workaround in my integration, I was able to disable typeDefinition and implementation calls by intercepting the Initialization call to the LSP and overriding the ServerCapabilities that were returned, disabling TypeDefinitionProvider and ImplementationProvider. This allowed my hyperlinks to start working, but at the expense of disabling functionality that I may eventually want elsewhere.

@mickaelistria
Copy link
Contributor

Thanks for the report. As you manage to identify the possible cause, would you like trying to submit a fix as a PR?

@will-lauer
Copy link
Author

I'm swamped with other stuff, so I don't know when I'll get to the point of working on an actual PR. I'll take a look, but please don't wait on me if you see an easy fix.

will-lauer added a commit to will-lauer/lsp4e that referenced this issue Jan 7, 2025
Adding LanguageServers.addAllSuccessful as an alternative to
LanguageServers.addAll so that exceptions from one of the requests being
combined can be ignored. Then using this new method in
OpenDeclarationHyperlinkDetector when combining the results from
definitions, declarations, typeDefinitions, and implementions because
some LSPs return errors instead of empty lists for some of these calls.
Instead, we combine only the successful results when building the list
of hyperlinks, ignoring the errors.

This should resolve eclipse-lsp4e#1169
@will-lauer will-lauer linked a pull request Jan 15, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants