Repository navigation
x/tools/gopls: improve handling for build tags #29202
Description
We need to handle build tags correctly in the LSP. If a user is on Linux, they should still see completions, diagnostics, etc. for files tagged with any other OS.
Activity
glerchundi commentedon Apr 24, 2019
Bitten by this also, lost an hour trying to figure out why after migrating to
goplsbasic functionalities didn't work...no import-organize, no format, no nothing. Afterwards I found that it's was working properly for everything except for files with build tags as mentioned in #31286.myitcv commentedon Apr 28, 2019
I'm wondering whether this is the right issue to become the umbrella issue for our long-running discussion about context?
sjdweb commentedon May 1, 2019
This is killing me with integration tests that have build tags. Any workarounds known?
christiankiely commentedon May 1, 2019
@sjdweb current workaround I'm using is to set the GOFLAGS environment variable. This is the configuration I'm using in VS Code:
myitcv commentedon May 2, 2019
This was brought up on Tuesday's golang-tools call.
@christiankiely - the "editor level"
GOFLAGSapproach your describe works in some cases, but not all. Because setting the flag in that way sets the flag for all modules and packages, i.e. for thegoplsinstance used by the editor.In discussions we've had thus far it seems for a complete solution we need to be able to define the sets of valid build flags at various levels:
We also need:
goplsThe conclusion on Tuesday's call was broadly that there are other higher priority issues right now; add to that, a solution that doesn't involve LSP spec changes isn't exactly jumping out at us.
cc @ianthehat @dominikh
breml commentedon May 11, 2019
Bitten by this as well. I use build tags to distinguish between normal builds and debug builds. Now VSCode reports
logError redeclared in this blockerror and the file is constantly marked as having errors.gracenoah commentedon May 15, 2019
Another populate use case: when using wire, there are two copies of the initializers:
gopls highlights them as duplicate definitions because it ignores build tags.
Is there a short term workaround in this simpler use case? Some way to make gopls ignore files with the wireinject build tag?
stamblerre commentedon May 15, 2019
Thank you for sharing this info. We realize that this is an issue that affects a lot of people, so we're trying to come up with a viable solution. It's a complicated problem, but to start, I think we can try to disable diagnostics on files that would be excluded given your GOOS/GOARCH.
evanphx commentedon May 25, 2019
As a data point, I'm experiencing this today in any package that picks up
os/userbecause it uses build tags to decide how to wire in OS user lookup. In my case, it's that analysis fails becausecgoLookupHostisn't defined as files aren't being evaluated.I understand that you'd like for gopls to allow for reading and analysis of files intended for a different GOOS that the one that the editor is on, but given how packages are structured, you'd having to perform analysis on the matrix of build tags. That seems.... time consuming at best. Plumbing through a default tag set seems like an easy way to at least get folks moving again.
I'm happy to look into how to do that if need be! Please just let me know.
evanphx commentedon May 25, 2019
Looks like it's also firing from
netlooking like:/usr/local/Cellar/go/1.12/libexec/src/net/lookup_unix.go:81:24: undeclared name: cgoLookupHost89 remaining items
findleyr commentedon Jan 11, 2024
@meling you're right, we could do that if there is only one build tag, though probably not anything more complicated. For example, we do have support for tags that are used to isolate scripts, such as
//go:build ignore, via the"standaloneTags"configuration, and that looks for files with a build constraint consisting of a single tag.meling commentedon Jan 12, 2024
@findleyr thanks for the pointer. Would it be possible to lift the restriction: Gopls considers a file to be a standalone main file if and only if it has package name "main" ?? Maybe then my use case would work. Or would it be preferable to support my use case via a separate configuration option?
Either way I appreciate the work you are doing to improve the plugin and gopls. Thank you so much!
arnevm123 commentedon Jan 12, 2024
@findleyr I am trying out the new gopls and it seems to work perfectly.
The only thing that I'm wondering is if I'm using go to definition on a function that is both declared in a file with a Windows and Linux build flag it goes to the Linux implementation (which makes sense, I'm on Linux).
Is it meant to show both definitions (similar to go to implementation) or is this expected behaviour?
findleyr commentedon Jan 12, 2024
@arnevm123 thanks for testing, and regarding your question this is expected behavior. For each open file there is one "default" build, which is first and foremost the default build on your system, followed by the first match from here. This ordering is somewhat arbitrary, though first class ports are preferred. We do need a fixed order for determinism.
I'd prefer if we could return all definitions, but unfortunately for technical reasons this is really intractable. Note that a file without any build constraints matches all ports. We can't feasibly return every single definition across any port, because this would be much more expensive for little value in the common case. The toolchain (
go listand the type checker) only operates on one build context at a time, so to answer questions about multiple build contexts we need to do additional work for each. It is infeasible to do 50x the work (there are around 50 supported ports), so we'd have to try to be smarter by e.g. looking at ignored files in the same directory as the definition to see if there are other declarations of the same symbol. At that point our heuristic is liable to be flaky and/or buggy, and is tightly coupled to the go command (taking us even farther from supporting other build systems). Still, it may be worth pursuing at some point.For now, I think we should keep it simple. We're going from gopls completely not working to mostly working, albeit with some surprises. Of course we'd greatly appreciate any alternative suggestions, or ways that we could make the current behavior more transparent.
findleyr commentedon Jan 12, 2024
@meling
I think there's a fundamental difference between scripts, for which there may be many in the same directory, each consisting of a single file, and your use case, for which you want to have at most two versions of your package in the directory. In your use case, I assume you want gopls to resolve packages declarations contained in other unconstrained files in the same directory (for example a
common.gofile that is shared by both thesolutionand!solutionbuilds).meling commentedon Jan 12, 2024
Yes, we do have common unconstrained file!
findleyr commentedon Jan 12, 2024
@meling I think the problem is that we could perhaps make
//go:build solutionwork automatically by creating a new build with-tags=solutionoverlaid, but then users may be confused when opening a file tagged//go:build (solution || experiment)doesn't work automatically, and we don't want to get into the business of solving constraint equations. The special handling for//go:build ignore, is justifiable because it's an extremely common pattern for scripts.If enough users want this, we can reconsider. But for now, for the same reasons as #29202 (comment), let's keep it simple.
(and thanks again for the feedback!)
findleyr commentedon Feb 16, 2024
gopls@v0.15.0 will be released soon (likely next week) containing the improvements discussed above. If you're interested to try it out, please install the latest prerelease, which is the release candidate:
(this differs from -pre.3 by just one minor CL)
With this release, I think working on multiple GOOS and GOARCH combinations is much better, though still not perfect. To follow up, I filed:
This is in addition to @meling's:
I think we're ready to close this issue in favor of the more specific follow-up items above. Please try out the release and let us know if you have feedback, either by commenting on or 👍'ing one of the issues above, or filing a new issue describing the improvement you'd like to see (or bug you observe). Thanks!
findleyr commentedon Feb 16, 2024
To clarify one point: the issue title "improve handling for build tags" could certainly be interpreted as also referring to better support for custom build tags, in addition to multiple GOOS/GOARCH. However, the issue description specifically mentions "completions, diagnostics, etc. for files tagged with any other OS", and I think we've achieved that in the v0.15.0 release.
I think better support for user-defined tags is covered by these two issues specifically:
Those issues both address ways that gopls could support working on multiple sets of user-defined tags. In #65089, it is proposed that gopls could automatically detect when there is one tag that separates files in a package. In #65757, it is proposed that users could explicitly configure multiple sets of tags.
tests: Fix lint issues
tests: Fix lint issues