-
Notifications
You must be signed in to change notification settings - Fork 17.6k
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
x/tools/gopls: jump to function definition via go:linkname #57312
Comments
Thanks, this is an interesting idea, which I think could be feasible. Transferring to the go issue as this would be implemented in gopls. |
Hi, I'd like to give this a try. I made a first attempt (without My idea was that a map of all links can be created ahead of time (and cached?), then used when analyzing a single file to decorate any decls that are part of a "link". How would this work in |
Hi @vikblom, thanks for looking into this! Sorry for the slow response: had to do a bit of research to be able to make an informed response. The compiler documentation for linkname is lacking (@adonovan is currently updating it in https://go.dev/cl/463136!), but I think we need to identify two different patterns for using In this playground link, where package
In bar, use a
In foo, use a
I think we need to treat these two cases differently from gopls: Handling pattern (1): (1) it is pretty straightforward how to find the symbol: in order to jump from Handling pattern (2) is more complicated: in order to jump from I think pattern 1 is far more common than pattern 2, and so it would be OK to only implement support for pattern 1 in the initial implementation. I don't think we should have to manage a linkname index. It would be simpler to just look through the forward dependencies of packages containing the The way to do this in gopls is to first find package information for packages containing the given file: Since metadata and type-checked packages are cached, these should be cache hits most of the time, and therefore it should be fine to implement the search in this way, without any need for additional caching. Sorry for the wall of text. Does that make sense? |
Thank you @findleyr that makes a lot of sense 👍 A note on pattern 2, as I understand it, But anyway, aiming for pattern 1 first seems like a good approach, I will start there. Should this extend "goto definition" or be some kind of navigable hyperlink? If you have a package in mind where this should be implemented that would be helpful. |
Oh, interesting. Thank you, that makes sense. I actually tested exactly that, and saw the compiler error. I did not think of adding a .s file.
I think eventually it would be good to handle a variety of requests related to linknames, but let's start with handling the At that link, gopls delegates to a source.Identifier function to collect information about the identifier at point. This function collects a bunch of information about the identifier at the current cursor position, most of which is not applicable for the case we're considering. (Frankly, Something like this: // At internal/lsp/definition.go:25:
if pkgPath, name := source.ParseLinkname(ctx, snapshot, fh, params.Position); pkgPath != "" {
return source.FindLinkname(ctx, snapshot, fh, pkgPath, name)
}
// Otherwise, proceed as before
ident, err := source.Identifier(...)
// In the internal/lsp/source package:
// ParseLinkname attempts to parse a go:linkname declaration at the given pos.
// If successful, it returns the package path and object name referenced by the second
// argument of the linkname directive.
//
// If the position is not in a go:linkname directive, or parsing fails, it returns "", "".
func ParseLinkname(ctx context.Context, snapshot Snapshot, fh FileHandle, pos protocol.Position) (pkgPath, name string)
// FindLinkname searches dependencies of packages containing fh for an object
// with linker name matching the given package path and name.
func FindLinkname(ctx context.Context, snapshot Snapshot, fh FileHandle, pos protocol.Position, pkgPath, name string) ([]protocol.Location, error) In other words, just intercept the definition request and check whether this new linkname logic applies, before passing off to the normal handling. You can either use In In this way, I think this project can be implemented without having to get too far into the weeds. You don't have to take my suggested API, but I would just advise keeping your change isolated from the existing logic. |
Keeping the changes isolated sounds like a good idea 👍 I will put some effort into this (could take some time) and post back here when I have an update (or questions). |
Thanks @vikblom! This issue is not particularly urgent, but I have added you as assignee. Feel free to ping back here if you have any questions as you work on this. |
Change https://go.dev/cl/463755 mentions this issue: |
For reference, I had a quick look at the standard library for examples. I found directives at: Pattern 1
https://cs.opensource.google/go/go/+/master:src/time/time.go;l=1099 Pattern 2
https://cs.opensource.google/go/go/+/master:src/runtime/mheap.go;l=1654 Edit: I wrote a script to walk through the standard library, checking if linkname directives (1st argument) funcs are with or without bodies, and if the referenced package (2nd argument) is a forward or reverse dependency. The numbers are
|
Enables jump to definition on the second argument in //go:linkname localname importpath.name if importpath is a transitive (possibly reverse) dependency of the package where the directive is located. Updates golang/go#57312 Change-Id: I59fa5821ffd44449cf49045a88b429f21e22febc Reviewed-on: https://go-review.googlesource.com/c/tools/+/463755 Reviewed-by: Robert Findley <[email protected]> gopls-CI: kokoro <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Alan Donovan <[email protected]> Run-TryBot: Robert Findley <[email protected]>
I took the first implementation for a spin and I'd say it works as expected in some/most cases. One problem is that just naively opening a package and invoking Go To Definition on a linkname directive can fail because the referenced package is not found. The jump succeeds after manually opening the referenced package. I'm not sure if its worth chasing down or if I should stick to the plan and look at implementing jump from single argument directives first. @findleyr what do you think is the next move here? |
For the record, this (tzdata) is almost an example of the second scenario in the doc---the upper package grabbing a symbol from the lower one---except that in this case the upper package doesn't actually have an import dependency on the lower one, presumably just because it doesn't need it. But that does mean gopls won't be able to find one from the other.
I suspect this is a relatively rare case and not worth worrying about. What would "jump from" look like. Changing the 'references' query to regard linknames as references? |
Hi @adonovan For reference here is one error I zeroed in on:
Sine (I'm assuming)
and vice versa. Is there some alternative approach to finding the symbol referenced in the linkname directive?
Sorry I'm not getting which you're referring to, the tzdata example or single-arg directives? About the "jump from" see rfindley's post above, at "Handling pattern (2)". |
You could filter AllMetadata based on the PkgPath named in the directive, perhaps as a third option if ReverseDependencies finds nothing.
I mean tzdata is a rare example in which there is no import relationship between the two packages. I agree with Rob that supporting case 1 (as you have already done) is the more important case by far. Case 2 requires that you search the source code of dependencies to find a 2-arg directive containing the right package name, whereas the search from the 2-arg directive can be done on the metadata. I'm inclined to say the complexity is not worth the trouble.
I wasn't sure whether you were proposing to treat linkname like a reference, but I would prefer that we don't. |
Okok, focusing on the common case makes sense!
I can give this a go and post back here if I come up with something or get stuck. Thanks! |
@adonovan AllMetadata works like a charm 👍 While implementing this I bumped into a problem with
Best I can tell this is because definition gets a nil hover back from a Hover query, which uses source.Identifier. Both you and @findleyr mentioned that source.Identifier would be the correct place for linkname logic (but we're staying out of there while its being refactored). Is this something to troubleshoot+patch right now or is it a side effect of being "outside" source.Identifier that could wait until its ready to integrate? Edit: I'm not familiar with Hover but I noticed that |
@vikblom I am eliminated source.Identifier in https://go.dev/cl/464415, which should be ready soon. I'll take a look at your example to make sure it works. |
Change https://go.dev/cl/469695 mentions this issue: |
…ctive. Handle the case where //go:linkname localname importpath.name is used in a package with no transitive dependency, forwards or reverse, to importpath. Updates golang/go#57312 Change-Id: I0033b166558275931a371a967caa6044a1b089f3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/469695 Run-TryBot: Alan Donovan <[email protected]> gopls-CI: kokoro <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Auto-Submit: Alan Donovan <[email protected]> Reviewed-by: Than McIntosh <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
Change https://go.dev/cl/479695 mentions this issue: |
Hi @findleyr I just uploaded a patch to support hover on linkname directives. I stumbled across a problem in Emacs LSP Mode while testing this patch. When using this hover feature there is some kind of race and the editor drops into a busy loop sending and cancelling requests. Both Emacs with Eglot and VS Code (very basic testing) works fine as far as I can tell, no loss of responsive-ness. Should this kind of potential client regression be handled before review? Before submit? I could try to raise it in the LSP Mode issue tracker after narrowing it down some more. Edit: This might just have been poor handling of protocol.Range on my part, cannot reproduce with the latest submitted patch. |
Enables hover on the second argument in go:linkname directives. Make findLinkname a bit more generic so it can support both (goto) definition and hover. Updates golang/go#57312 Change-Id: I9ac7cfceb62dada69bf7fced1ac03877947bb615 Reviewed-on: https://go-review.googlesource.com/c/tools/+/479695 gopls-CI: kokoro <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Run-TryBot: Robert Findley <[email protected]> Reviewed-by: Alan Donovan <[email protected]> Reviewed-by: Robert Findley <[email protected]>
Thanks very much @vikblom for this contribution. It looks like this issue is complete! |
Is your feature request related to a problem? Please describe.
//go:linkname
is one method to export one function as a different name or path.I think we should support the jumping to/back navigation, it will be much more convenient to read the source code, especially go source.
Describe the solution you'd like
Go AST parser extracts this go:linkname directives and generate navigation link.
When user hovers on the function name, it display a link, so user can ctrl+click to jump to the definition location.
Describe alternatives you've considered
No alternatives considered now.
Additional context
No additional context now.
The text was updated successfully, but these errors were encountered: