GOPACKAGESDRIVER_BAZEL_QUERY_SCOPE implementation is not with "./..." and "bultin" args at the same time

[waltercacau]

What version of rules_go are you using?

v0.46.0

What version of gazelle are you using?

v0.35.0

What version of Bazel are you using?

6.3.2

Does this issue reproduce with the latest releases of all the above?

Probably yes, I can try later but my understanding of the code is that it is a problem with how GOPACKAGESDRIVER_BAZEL_QUERY_SCOPE was implemented.

What operating system and processor architecture are you using?

Linux AMD64

Any other potentially useful information about your toolchain?

N/A

What did you do?

While trying to use rules_go gopackagedriver with VSCode IDE, I noticed that if GOPACKAGESDRIVER_BAZEL_QUERY_SCOPE is used we seem to be getting no results back.

Using a wrapper script I was able to determine what the arguments VSCode (through gopls) give to rules_go's gopackagedriver. The arguments were "./..." and "bultin".

Below is the minimum reproducible snippet:

echo {} | GOPACKAGESDRIVER_BAZEL_QUERY_SCOPE=//path/to/my/target bazel run -- @io_bazel_rules_go//go/tools/gopackagesdriver   ./... builtin

Result:

INFO: Invocation ID: 4d3e7b04-138e-4450-a708-3d94669f1d2f
INFO: Analyzed target @io_bazel_rules_go//go/tools/gopackagesdriver:gopackagesdriver (1 packages loaded, 6 targets configured).
INFO: Found 1 target...
Target @io_bazel_rules_go//go/tools/gopackagesdriver:gopackagesdriver up-to-date:
  bazel-bin/external/io_bazel_rules_go/go/tools/gopackagesdriver/gopackagesdriver_/gopackagesdriver
INFO: Elapsed time: 5.895s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
INFO: Running command line: bazel-bin/external/io_bazel_rules_go/go/tools/gopackagesdriver/gopackagesdriver_/gopackagesdriver ./... builtin
Running: [bazel info --tool_tag=gopackagesdriver --ui_actions_shown=0]
INFO: Invocation ID: 144cb01f-1e2a-49bc-b505-314c8cebd449
Running: [bazel query --tool_tag=gopackagesdriver --ui_actions_shown=0 --ui_event_filters=-info,-stderr --noshow_progress --order_output=no --output=label --nodep_deps --noimplicit_deps --notool_deps kind("go_library", attr(importpath, "^.(/.+)?$", deps(//path/to/my/target))) union kind("go_library", attr(importpath, "builtin", deps(//path/to/my/target)))]
{"NotHandled":true,"Compiler":"gc","Arch":"amd64","Packages":[]}
error: unable to lookup package: found no labels matching the requests

I believe the comment in this PR by @fmeum : https://github.com/bazelbuild/rules_go/pull/3688/files#r1321005945 is particularly relevant. There is an implicit assumption that if GOPACKAGESDRIVER_BAZEL_QUERY_SCOPE all queries need to be kind("%s", attr(importpath, "%s", deps(%s))) type of query. I would argue the right behavior is actually intersection with deps($GOPACKAGESDRIVER_BAZEL_QUERY_SCOPE) which seems related to what the comment suggested

You can see in the output above that for handling the builtin request we should definetly not be using kind("go_library", attr(importpath, "builtin", deps(//path/to/my/target)) as searching on importpath = bultin is probably not what was intended.

What did you expect to see?

I was expecting to have at least some packages returned back and for them to be transitive dependencies of the target I gave as the GOPACKAGESDRIVER_BAZEL_QUERY_SCOPE env variable

What did you see instead?

No packages returned.

[waltercacau]

Relevant code in master as of time of this writing 2024-07-30:

rules_go/go/tools/gopackagesdriver/bazel_json_builder.go

Lines 122 to 145 in 634fc28

	func (b *BazelJSONBuilder) queryFromRequests(requests ...string) string {
	ret := make([]string, 0, len(requests))
	for _, request := range requests {
	result := ""
	if strings.HasSuffix(request, ".go") {
	f := strings.TrimPrefix(request, "file=")
	result = b.fileQuery(f)
	} else if bazelQueryScope != "" {
	result = b.packageQuery(request)
	} else if isLocalPattern(request) {
	result = b.localQuery(request)
	} else if request == "builtin" || request == "std" {
	result = fmt.Sprintf(RulesGoStdlibLabel)
	}
	if result != "" {
	ret = append(ret, result)
	}
	}
	if len(ret) == 0 {
	return RulesGoStdlibLabel
	}
	return strings.Join(ret, " union ")
	}

It seems if bazelQueryScope != null we just assume packageQuery and that mishandles builtin and ./...

At a super high level, I suspect this older code from April 2023 is closer to correct:

rules_go/go/tools/gopackagesdriver/bazel_json_builder.go

Lines 89 to 115 in ec5a27c

	func (b *BazelJSONBuilder) queryFromRequests(requests ...string) string {
	ret := make([]string, 0, len(requests))
	for _, request := range requests {
	result := ""
	if request == "." || request == "./..." {
	if bazelQueryScope != "" {
	result = fmt.Sprintf(`kind("go_library", %s)`, bazelQueryScope)
	} else {
	result = fmt.Sprintf(RulesGoStdlibLabel)
	}
	} else if request == "builtin" || request == "std" {
	result = fmt.Sprintf(RulesGoStdlibLabel)
	} else if strings.HasPrefix(request, "file=") {
	f := strings.TrimPrefix(request, "file=")
	result = b.fileQuery(f)
	} else if bazelQueryScope != "" {
	result = b.packageQuery(request)
	}
	if result != "" {
	ret = append(ret, result)
	}
	}
	if len(ret) == 0 {
	return RulesGoStdlibLabel
	}
	return strings.Join(ret, " union ")
	}

Maybe one change I would make is to add all kinds to the bazelQueryScope?

			if bazelQueryScope != "" {
				result = fmt.Sprintf(`kind("%s", %s)`, strings.Join(_defaultKinds, "|"), bazelQueryScope)
			} else {
				result = fmt.Sprintf(RulesGoStdlibLabel)
			}

[fmeum]

CC @JamyDev