cover: use CamelCase for coverage variable

[sluongng]

During the invocation of 'bazel coverage', rules_go will rewrite the
source files using 'go tool cover -var ...' so that the
variable can be extracted for coverage statistic later. For more
information on this, review the relevant Official Go Team's blog post (1).

rules_go currently injecting a variable using Cover_snake_case which
is natural to starlark and python, however this will strip up some
golang static analyzer for not following the CamelCase variable naming
convention. In particular, if the generated source for coverage is run
against staticcheck's ST1003 installed as part of nogo, it will
fail.

Apply snake_case to CamelCase conversion on all coverage variables.

(1): https://go.dev/blog/cover

What type of PR is this?

Uncomment one line below and remove others.

Bug fix

Feature
Documentation
Other

What does this PR do? Why is it needed?

Which issues(s) does this PR fix?

Fixes #2982

Other notes for review

I think the code in go/private/actions/cover.bzl code path is almost(?) irrelevant right now as updating that starlark files does not reflect much on how coverage is being collected. But I still want to provide a fix there just for completeness sake, just in case somebody else started to rely on the starlark code path one day.

[sluongng]

> git diff HEAD@{1}..HEAD
diff --git a/go/private/actions/cover.bzl b/go/private/actions/cover.bzl
index fe0fe8e1..a1dff493 100644
--- a/go/private/actions/cover.bzl
+++ b/go/private/actions/cover.bzl
@@ -31,8 +31,8 @@ def _snake_to_camel(s):
     words = s.split("_")
     out = ""
     for w in words:
-      out += w[:1].upper()
-      out += w[1:].lower()
+        out += w[:1].upper()
+        out += w[1:].lower()
     return out

 def emit_cover(go, source):

force pushed to fix indentation

[sluongng]

@go-maintainers ping as all checks have passed

[robfig]

Thanks for the fix!

I think you had this in an earlier revision, but it feels unnecessarily complicated to generate a snake case identifier and then run that through a case conversion function. What about using e.g. Z instead of _, or any other token which satisfies staticcheck? Then the subsequent conversion could be skipped and it would be a tiny bit simpler.

@jayconrod , can you provide any context on the question raised on the division of responsibility for coverage between starlark vs go ?

[sluongng]

@robfig

it feels unnecessarily complicated to generate a snake case identifier and then run that through a case conversion function

I share this sentiment and was looking for a cheap solution, which is why in #2982 I used a patch that just do a s/_/Z/ over the variable.

Another alternative might be to avoid having to run this entirely

"Cover_%s_%s" % (_sanitize(pkgpath), _sanitize(src.basename[:-3]))
...
fmt.Sprintf("Cover_%s_%d_%s", sanitizePathForIdentifier(importPath), i, sanitizePathForIdentifier(stem))

and instead replace _sanitize with some SnakeCase conversion function.
However that involve more code changes so I was a bit uncertain to take that route (just in case pkgpath or importPath or stem might contains some natural snake case.

Let me know which direction is preferable. I am more than happy to do a fixup and get this merged 😄

[robfig]

Ah, I didn't realize that the sanitize functions also introduced underscores.

I think that there's not much risk in this code churn, so I think it would be fine to fix this up rather than do the minimal patch. It looks like the sanitize functions could be pretty easily modified to use whichever separator character you choose. I think I've seen "Z" used for mangling in the past, and that seems unlikely to violate any checks, so that seems like the best bet?

[robfig]

Do you see any reason why the package name/path are passed separately through the sanitize function, instead of the whole result of fmt.Sprintf being passed through it?

[robfig]

Actually, I think the underlying issue here is that nogo should run on the original source, not the instrumented source. That's what the Go CLI does.

[sluongng]

@robfig so the root of the problem is between

• bazel coverage is invoking go tool cover to rewrite the source code
• nogo is running on the source code post-rewrite

I have only looked into how bazel coverage is working when I created this patch.
It might take a bit more effort to investigate how nogo works and see if it can be made so that its only run on pre-rewrite during bazel coverage invocation. 🤔

---

So I think there are a few ways to move forward here:

1. Short term: To quickly enable folks to use staticchecks passes with nogo and not failing due to bazel coverage
   a. Accept the current PR as-is
   b. Create a different PR that only replace the underscores _ with Z

2. Long term: To ensure that things work as intended: create an issue to investigate running nogo on pre-rewrite source files.

Let me know if you prefer 1a or 1b for the short term. After that, we can create a separate issue for 2 with some additional investigation info.

[robfig]

Even as a "short term" fix, let's replace the _ with Z instead of adding extra complication with case conversions when it's not necessary. Thanks

[sluongng]

@robfig I spent a day looking into making nogo ignoring generated source and I don't think it's too hard to achieve. I see that we need to ignore 2 cases of generated source by rules_go:

1. Coverage variable
2. _empty.go when no source is available.

Changes can be done in go/tools/builders/compilepkg.go and go/tools/builders/compile.go to make this happen.

But I have yet to have time to test my changes locally and these areas don't seem to be well-tested nor are they well documented, so instead I have created #2995 as a relatively cheaper fix right now. A future PR that would help nogo ignore rules_go generated code should have an easy time reverting it.