Bazel 6 `--incompatible_unambiguous_label_stringification` causes Go tests to not be considered under the same package as the code it's testing

[JamyDev]

Description of the bug:

After upgrading to Bazel 6.0 in our Go repo we started encountering warnings about using unexported variables from the main code:

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

In a go _test.go file, try to use an unexported const from the main package file. This is allowed, but the IDE inspection complains. Setting --noincompatible_unambiguous_label_stringification fixes it, but in all likelihood this won't be available for too much longer.

Which Intellij IDE are you using? Please provide the specific version.

Goland 2022.3.1

What programming languages and tools are you using? Please provide specific versions.

bazel, golang

What Bazel plugin version are you using?

2023.01.10

Have you found anything relevant by searching the web?

N/A root cause likely in how we set the TargetIdeInfo label (now it has an @ prefix in front of the //

Any other information, logs, or outputs that you want to share?

No response

[sgowroji]

Hi @JamyDev, Could you please provide a sample example to reproduce the above issue. Thanks!

[JamyDev]

Test repo: https://github.com/JamyDev/go-bazel-sample, just clone and use the .bazelproject file in the root.

Once opened, sync and navigate to src/jamy.be/sample/sample_test.go. You'll see that _variable has an error that say "Unexported const"

Now go to .bazelrc and uncomment the first line, sync again. Now it does not have the error.

[JamyDev]

Root cause seems to be https://github.com/bazelbuild/intellij/pull/3913 @Wyverald

[JamyDev]

Dug a little deeper and the root cause of the inconsistency is these labels using str() whereas here and here we use stringify_label.

Now I do want to argue that we shouldn't be using stringify_label now that bazel 6.0 is making this change to prefix all local paths with @//. It seems like the original addition of stringify_label was added in #3913 to avoid having to update the test cases to support this new label type. I propose we fix those test cases.

[Wyverald]

Ah, thanks for digging. It might be more prudent to use the "stringify_label" trick everywhere, since inside Google --incompatible_unambiguous_label_stringification is disabled.

[JamyDev]

@Wyverald can you clarify why we need to strip the @ prefix for local labels? From what I could gather from your PR is that it was purely for the test cases.

[Wyverald]

Yeah it is just for test cases. The problem is that the test cases ask for, for example, "give me the target //foo:bar from the test fixture" (without the leading @(s)), so we need to strip them in the fixture generation code.

Now we could just have the test cases ask for @//foo:bar instead and change the fixture generation code to just use str(). But in Google internal code, --incompatible_unambiguous_label_stringification is disabled, so running the same test fixture generation code in Google would still yield //foo:bar, even if we switch to str().

AFAIU a lot of code is shared between the Google version and the public version of the intellij plugin, and I don't know how easy it is to maintain a discrepancy here. So I'll leave it up to the maintainers to decide what to do.

[blorente]

AFAIU a lot of code is shared between the Google version and the public version of the intellij plugin, and I don't know how easy it is to maintain a discrepancy here. So I'll leave it up to the maintainers to decide what to do.

@mai93 I don't have insight into how much overlap there is, so I think your advice would be useful here.

[mai93]

This should be fixed with 3ce133cc9e0bc4962671647b01e6514f95821612