-
-
Notifications
You must be signed in to change notification settings - Fork 668
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
cover: use CamelCase for coverage variable #2984
Conversation
During the invocation of 'bazel coverage', rules_go will rewrite the source files using 'go tool cover -var <cover var name> ...' 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
9a7f33e
to
8fd37e3
Compare
force pushed to fix indentation |
@go-maintainers ping as all checks have passed |
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 ? |
I share this sentiment and was looking for a cheap solution, which is why in #2982 I used a patch that just do a Another alternative might be to avoid having to run this entirely
and instead replace Let me know which direction is preferable. I am more than happy to do a fixup and get this merged 😄 |
Ah, I didn't realize that the 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? |
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? |
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. |
@robfig so the root of the problem is between
I have only looked into how So I think there are a few ways to move forward here:
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. |
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 |
During the invocation of `bazel coverage`, rules_go will rewrite the source fils using `go tool cover -var ...` so that the variable can be extracted for coverage statistic after test execution. For more information on this, review the relevant Official Go Team's blog post (1) The variable used by rules_go is following the `Cover_snake_case` naming convention, which is natural to starlark and python, but not Golang. For this reason, some static analysis could get triggered by the usage of snake case naming variable while running over coverage source code. The correct solution for this problem should be to make rules_go's static analysis framework `nogo` to NOT run on the generated source code and instead only run on the original source code. However, replacing underscore separators with character `Z` is a relatively cheap fix that would let us unblock static analysis run during `bazel coverage` for now. See discussion in (2) for more details. (1): https://go.dev/blog/cover (2): bazel-contrib#2984
@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:
Changes can be done in 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. |
During the invocation of `bazel coverage`, rules_go will rewrite the source fils using `go tool cover -var ...` so that the variable can be extracted for coverage statistic after test execution. For more information on this, review the relevant Official Go Team's blog post (1) The variable used by rules_go is following the `Cover_snake_case` naming convention, which is natural to starlark and python, but not Golang. For this reason, some static analysis could get triggered by the usage of snake case naming variable while running over coverage source code. The correct solution for this problem should be to make rules_go's static analysis framework `nogo` to NOT run on the generated source code and instead only run on the original source code. However, replacing underscore separators with character `Z` is a relatively cheap fix that would let us unblock static analysis run during `bazel coverage` for now. See discussion in (2) for more details. (1): https://go.dev/blog/cover (2): #2984
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
whichis natural to starlark and python, however this will strip up some
golang static analyzer for not following the
CamelCase
variable namingconvention. In particular, if the generated source for coverage is run
against
staticcheck
's ST1003 installed as part ofnogo
, it willfail.
Apply snake_case to CamelCase conversion on all coverage variables.
(1): https://go.dev/blog/cover
What type of PR is this?
Bug fix
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.