nogo: use original source files instead of coverage-instrumented #3770

emmaxy · 2023-11-28T23:57:22Z

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Nogo static analysis should not run against coverage instrumented source files, but the original ones.

The current mechanism with nogoSrcsOrigin map doesn't work when cgo is enabled, because source files paths change after cgo compilation, resulting in no match to the nogoSrcsOrigin map, hence nogo checks against modified source files, reporting unexpected errors.

This PR allows nogo to run static analysis against the original source files, instead of coverage-instrumented ones, by making a hard copy of the source file slice if coverage mode is set and before they are modified.

Which issues(s) does this PR fix?

Fixes #3769

Other notes for review

…ge instrumented

fmeum · 2023-11-29T07:43:48Z

CC @sluongng

sluongng

Oh hey that’s my TODO 😂
Great job on this PR!

Could you help provide us with a test that demonstrate the broken behavior before this change and it’s now fixed? There has been several discussions about reworking nogo to enable it with bzlmod and validation action. A test could help us avoid repeating the same mistake.

sluongng · 2023-11-29T14:20:10Z

On second thought, I am not quite sure if this is the right thing to do since we do have analyzers that could operate on cgo code. https://cs.opensource.google/go/x/tools/+/master:go/analysis/passes/cgocall/cgocall.go

It would be nice if we have a test to demonstrate how this PR would affect such an analyzer.

go/tools/builders/compilepkg.go

sluongng

The current mechanism with nogoSrcsOrigin map doesn't work when cgo is enabled, because source files paths change after cgo compilation, resulting in no match to the nogoSrcsOrigin map, hence nogo checks against modified source files, reporting unexpected errors.

Perhaps we could include the original cgoSrcs path inside nogoSrcsOrigin for a simpler fix.

If that's not desirable vs the current approach, then I would expect the nogoSrcsOrigin mechanism cleaned up within this PR as well.

Would love to hear what you think about this.

go/tools/builders/compilepkg.go

emmaxy · 2023-11-30T22:25:11Z

Perhaps we could include the original cgoSrcs path inside nogoSrcsOrigin for a simpler fix.

If that's not desirable vs the current approach, then I would expect the nogoSrcsOrigin mechanism cleaned up within this PR as well.

Would love to hear what you think about this.

As explained above, the nogoSrcsOrigin mechanism cannot protect goSrcs nor cgoSrcs from coverage modification when cgo is enabled, and source file do need to be cgo compiled for nogo to run static analysis. So I feel the nogoSrcsOrigin mechanism should be abondoned here. I'll update this PR to further cleanup nogoSrcsOrigin.

sluongng

Great job. The PR looks much cleaner now.

Is it possible to add some tests demonstrating the new behavior?

go/tools/builders/compilepkg.go

sluongng · 2023-12-05T09:25:06Z

This goes without saying, but might be worth highlighting in the changelog if we were to merge this in the future:

This change will double the cgo/cc part in a compilepkg action when coverage is enabled, leading to slower action, depending on how much cgo/cc is in your package.

emmaxy · 2023-12-05T09:36:34Z

Is it possible to add some tests demonstrating the new behavior?

Could you help provide some example/reference for such tests? I'd really appreciate it. I'm not very familiar with how tests are done in rules_go, as this is my first time working in this repo.

sluongng · 2023-12-05T09:39:32Z

https://github.com/bazelbuild/rules_go/blob/master/tests/core/nogo/coverage/README.rst should be a good starting point for this case since it tests for a combination of nogo+coverage.

emmaxy · 2023-12-11T04:56:14Z

Unit test added! Sorry for the delay.
This unit test passes with the fix in this PR, and fails on current master, with error message:

compilepkg: nogo: errors found by nogo during build-time code analysis:
foo.go:22:1: don't use init function (gochecknoinits)
Target //:foo_test failed to build

This shows that the nogo was running static analysis on coverage instrumented source files, resulting in error non-existing in the original source file.

But I'm not sure why the build is failing for windows (OpenJDK 11, VS2022), any suggestions?

sluongng

Very nice!
Having the test would let us prevent future regression.

Could you please add a few lines describing the test in tests/core/nogo/coverage/README.rst?

Aside from that and a few nits, this PR LGTM.

tests/core/nogo/coverage/coverage_cgo_test.go

emmaxy · 2023-12-15T05:32:00Z

Took a while before I realized the build failure was because a known issue #2518 that coverage not able to run on windows platform, so I updated the tests accordingly. Also changed it so that tests are skipped with target_compatible_with in BUILD.bazel instead of CI configuration.
Now that build is green, @sluongng could you help review again, and if looks good to you I could merge this PR

sluongng

Functionally LGTM. I have a few optional nits/questions from a second reading of the diff.

Will let the active maintainers discuss the style concerns of moving targets out of presubmit.yml. cc: @fmeum @linzhp

go/tools/builders/compilepkg.go

.bazelci/presubmit.yml

go/tools/builders/compilepkg.go

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [io_bazel_rules_go](https://togithub.com/bazelbuild/rules_go) | http_archive | minor | `v0.44.2` -> `v0.45.0` | --- ### Release Notes <details> <summary>bazelbuild/rules_go (io_bazel_rules_go)</summary> ### [`v0.45.0`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.45.0) [Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.44.2...v0.45.0) #### `WORKSPACE` code load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") http_archive( name = "io_bazel_rules_go", sha256 = "de7974538c31f76658e0d333086c69efdf6679dbc6a466ac29e65434bf47076d", urls = [ "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.45.0/rules_go-v0.45.0.zip", "https://github.com/bazelbuild/rules_go/releases/download/v0.45.0/rules_go-v0.45.0.zip", ], ) load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") go_rules_dependencies() go_register_toolchains(version = "1.21.6") #### What's Changed - nogo: use original source files instead of coverage-instrumented by [@emmaxy](https://togithub.com/emmaxy) in [bazelbuild/rules_go#3770 - Allow compilers to be overwritten in go_grpc_library() by [@mering](https://togithub.com/mering) in [bazelbuild/rules_go#3812 - update documentation for 0.44 by [@tyler-french](https://togithub.com/tyler-french) in [bazelbuild/rules_go#3813 - Temporarily undo deprecation of `//proto:go_grpc` by [@fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3818 - Rename bazel_features polyfill to prevent conflicts by [@fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3817 - Handle prerelease versions in polyfill by [@illicitonion](https://togithub.com/illicitonion) in [bazelbuild/rules_go#3821 - Extend gomock to allow passing an source_importpath instead of library when operating in source mode by [@therve](https://togithub.com/therve) in [bazelbuild/rules_go#3822 - Fix coverage collection with modified `COVERAGE_DIR` env variable by [@fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3820 - improving logging for subcommand failure by [@linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3824 - release rules go 0.45.0 by [@tyler-french](https://togithub.com/tyler-french) in [bazelbuild/rules_go#3825 #### New Contributors - [@emmaxy](https://togithub.com/emmaxy) made their first contribution in [bazelbuild/rules_go#3770 - [@therve](https://togithub.com/therve) made their first contribution in [bazelbuild/rules_go#3822 **Full Changelog**: bazelbuild/rules_go@v0.44.2...v0.45.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate).  Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [io_bazel_rules_go](https://togithub.com/bazelbuild/rules_go) | http_archive | minor | `v0.44.2` -> `v0.45.0` | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>bazelbuild/rules_go (io_bazel_rules_go)</summary> ### [`v0.45.0`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.45.0) [Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.44.2...v0.45.0) #### `WORKSPACE` code load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") http_archive( name = "io_bazel_rules_go", sha256 = "de7974538c31f76658e0d333086c69efdf6679dbc6a466ac29e65434bf47076d", urls = [ "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.45.0/rules_go-v0.45.0.zip", "https://github.com/bazelbuild/rules_go/releases/download/v0.45.0/rules_go-v0.45.0.zip", ], ) load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") go_rules_dependencies() go_register_toolchains(version = "1.21.6") #### What's Changed - nogo: use original source files instead of coverage-instrumented by [@emmaxy](https://togithub.com/emmaxy) in [bazelbuild/rules_go#3770 - Allow compilers to be overwritten in go_grpc_library() by [@mering](https://togithub.com/mering) in [bazelbuild/rules_go#3812 - update documentation for 0.44 by [@tyler-french](https://togithub.com/tyler-french) in [bazelbuild/rules_go#3813 - Temporarily undo deprecation of `//proto:go_grpc` by [@fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3818 - Rename bazel_features polyfill to prevent conflicts by [@fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3817 - Handle prerelease versions in polyfill by [@illicitonion](https://togithub.com/illicitonion) in [bazelbuild/rules_go#3821 - Extend gomock to allow passing an source_importpath instead of library when operating in source mode by [@therve](https://togithub.com/therve) in [bazelbuild/rules_go#3822 - Fix coverage collection with modified `COVERAGE_DIR` env variable by [@fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3820 - improving logging for subcommand failure by [@linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3824 - release rules go 0.45.0 by [@tyler-french](https://togithub.com/tyler-french) in [bazelbuild/rules_go#3825 #### New Contributors - [@emmaxy](https://togithub.com/emmaxy) made their first contribution in [bazelbuild/rules_go#3770 - [@therve](https://togithub.com/therve) made their first contribution in [bazelbuild/rules_go#3822 **Full Changelog**: bazelbuild/rules_go@v0.44.2...v0.45.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/kreempuff/rules_unreal_engine).

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [io_bazel_rules_go](https://togithub.com/bazelbuild/rules_go) | http_archive | minor | `v0.44.2` -> `v0.45.1` | --- ### Release Notes <details> <summary>bazelbuild/rules_go (io_bazel_rules_go)</summary> ### [`v0.45.1`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.45.1) [Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.45.0...v0.45.1) #### `WORKSPACE` code load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") http_archive( name = "io_bazel_rules_go", sha256 = "6734a719993b1ba4ebe9806e853864395a8d3968ad27f9dd759c196b3eb3abe8", urls = [ "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.45.1/rules_go-v0.45.1.zip", "https://github.com/bazelbuild/rules_go/releases/download/v0.45.1/rules_go-v0.45.1.zip", ], ) load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") go_rules_dependencies() go_register_toolchains(version = "1.21.6") #### What's Changed - Disable `layering_check` feature in CGo tests by [@fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3829 - Print stack traces without panicking on test timeout by [@fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3833 - Fix regression in empty `.go` file filtering for nogo by [@fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3832 - release patch version 0.45.1 by [@tyler-french](https://togithub.com/tyler-french) in [bazelbuild/rules_go#3836 **Full Changelog**: bazelbuild/rules_go@v0.45.0...v0.45.1 ### [`v0.45.0`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.45.0) [Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.44.2...v0.45.0) #### `WORKSPACE` code load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") http_archive( name = "io_bazel_rules_go", sha256 = "de7974538c31f76658e0d333086c69efdf6679dbc6a466ac29e65434bf47076d", urls = [ "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.45.0/rules_go-v0.45.0.zip", "https://github.com/bazelbuild/rules_go/releases/download/v0.45.0/rules_go-v0.45.0.zip", ], ) load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") go_rules_dependencies() go_register_toolchains(version = "1.21.6") #### What's Changed - nogo: use original source files instead of coverage-instrumented by [@emmaxy](https://togithub.com/emmaxy) in [bazelbuild/rules_go#3770 - Allow compilers to be overwritten in go_grpc_library() by [@mering](https://togithub.com/mering) in [bazelbuild/rules_go#3812 - update documentation for 0.44 by [@tyler-french](https://togithub.com/tyler-french) in [bazelbuild/rules_go#3813 - Temporarily undo deprecation of `//proto:go_grpc` by [@fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3818 - Rename bazel_features polyfill to prevent conflicts by [@fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3817 - Handle prerelease versions in polyfill by [@illicitonion](https://togithub.com/illicitonion) in [bazelbuild/rules_go#3821 - Extend gomock to allow passing an source_importpath instead of library when operating in source mode by [@therve](https://togithub.com/therve) in [bazelbuild/rules_go#3822 - Fix coverage collection with modified `COVERAGE_DIR` env variable by [@fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3820 - improving logging for subcommand failure by [@linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3824 - release rules go 0.45.0 by [@tyler-french](https://togithub.com/tyler-french) in [bazelbuild/rules_go#3825 #### New Contributors - [@emmaxy](https://togithub.com/emmaxy) made their first contribution in [bazelbuild/rules_go#3770 - [@therve](https://togithub.com/therve) made their first contribution in [bazelbuild/rules_go#3822 **Full Changelog**: bazelbuild/rules_go@v0.44.2...v0.45.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate).  Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>

nogo: use original source files for static analysis instead of covera…

1d808c2

…ge instrumented

sluongng reviewed Nov 29, 2023

View reviewed changes

emmaxy commented Nov 29, 2023

View reviewed changes

go/tools/builders/compilepkg.go Show resolved Hide resolved

sluongng mentioned this pull request Nov 30, 2023

cgo2: refactor for readability #3771

Open

sluongng reviewed Nov 30, 2023

View reviewed changes

go/tools/builders/compilepkg.go Show resolved Hide resolved

go/tools/builders/compilepkg.go Show resolved Hide resolved

nogo: cleanup nogoSrcsOrigin

ee8f5b2

sluongng reviewed Dec 1, 2023

View reviewed changes

go/tools/builders/compilepkg.go Outdated Show resolved Hide resolved

nogo: use goSrcsNogo directly to run nogo static analysis

d5d75ee

emmaxy added 2 commits December 10, 2023 19:50

Merge branch 'master' into master

45be653

nogo: add unit test to demonstrate how the issue is fixed

713b54a

sluongng reviewed Dec 11, 2023

View reviewed changes

tests/core/nogo/coverage/coverage_cgo_test.go Outdated Show resolved Hide resolved

tests/core/nogo/coverage/coverage_cgo_test.go Outdated Show resolved Hide resolved

sluongng reviewed Dec 11, 2023

View reviewed changes

tests/core/nogo/coverage/coverage_cgo_test.go Outdated Show resolved Hide resolved

emmaxy added 4 commits December 11, 2023 19:06

nogo: update per review comments

2e1aed6

Merge branch 'master' into master

7fd90fb

Merge branch 'master' into master

afc9cc8

Merge branch 'master' into master

5c82944

emmaxy force-pushed the master branch 2 times, most recently from 680bfbb to b43bffc Compare December 15, 2023 01:16

sluongng approved these changes Dec 15, 2023

View reviewed changes

go/tools/builders/compilepkg.go Show resolved Hide resolved

.bazelci/presubmit.yml Show resolved Hide resolved

go/tools/builders/compilepkg.go Show resolved Hide resolved

emmaxy force-pushed the master branch from b43bffc to e53c55d Compare December 16, 2023 02:49

fmeum reviewed Dec 16, 2023

View reviewed changes

go/tools/builders/compilepkg.go Show resolved Hide resolved

nogo: attempt to get test to pass on windows

504172a

emmaxy force-pushed the master branch from e53c55d to 504172a Compare January 4, 2024 08:56

Merge branch 'master' into master

731986e

fmeum approved these changes Jan 4, 2024

View reviewed changes

fmeum merged commit 6236dd8 into bazelbuild:master Jan 4, 2024
2 checks passed

sluongng mentioned this pull request Jan 8, 2024

Nogo: provide a way to ignore external cgo source #3619

Open

sluongng mentioned this pull request Mar 22, 2024

cgocall doesn't work #2396

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nogo: use original source files instead of coverage-instrumented #3770

nogo: use original source files instead of coverage-instrumented #3770

emmaxy commented Nov 28, 2023

fmeum commented Nov 29, 2023

sluongng left a comment

sluongng commented Nov 29, 2023

sluongng left a comment

emmaxy commented Nov 30, 2023

sluongng left a comment

sluongng commented Dec 5, 2023

emmaxy commented Dec 5, 2023

sluongng commented Dec 5, 2023

emmaxy commented Dec 11, 2023

sluongng left a comment

emmaxy commented Dec 15, 2023

sluongng left a comment

nogo: use original source files instead of coverage-instrumented #3770

nogo: use original source files instead of coverage-instrumented #3770

Conversation

emmaxy commented Nov 28, 2023

fmeum commented Nov 29, 2023

sluongng left a comment

Choose a reason for hiding this comment

sluongng commented Nov 29, 2023

sluongng left a comment

Choose a reason for hiding this comment

emmaxy commented Nov 30, 2023

sluongng left a comment

Choose a reason for hiding this comment

sluongng commented Dec 5, 2023

emmaxy commented Dec 5, 2023

sluongng commented Dec 5, 2023

emmaxy commented Dec 11, 2023

sluongng left a comment

Choose a reason for hiding this comment

emmaxy commented Dec 15, 2023

sluongng left a comment

Choose a reason for hiding this comment