Skip to content

Allow to exclude certain target from nogo facts collection #4353

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

linzhp
Copy link
Contributor

@linzhp linzhp commented May 20, 2025

What type of PR is this?
Feature

What does this PR do? Why is it needed?
After #4268, nogo collects facts from all targets. So go_register_nogo's includes and excludes now overlap with the JSON config's only_files and exclude_files, which is to surppress nogo errors in certain files/targets. Meanwhile, collecting facts can be slow for very large Go packages for some analyzers, slower than compilation. To address the performance issue, this PR revert excludes to its previous behavior: stop nogo from collecting facts from certain packages, while making includes of go_register_nogo a no-op with a warning message, asking people to use the JSON config instead. So nogo will continue to collect facts from all targets, which is the safe default, except those in excludes.

Other notes for review
With this PR, users need to add a config like this to surppress issues in external repositories:

{
  "_base": {
    "exclude_files": {
      "^external/": "external repositories"
    }
  }
}

Or if all Go code they care about are under a specific directory, they can have:

{
  "_base": {
    "only_files": {
      "^src/": "All Go files in the current repo"
    }
  }
}

Alternatively, to be backward-compatible, we can add a flag or attribute like exclude_collect_facts, but adding that on top of excludes and exclude_files makes hard to remember which is which.

@@ -542,6 +544,10 @@ func checkAnalysisResults(actions []*action, pkg *goPackage) ([]diagnosticEntry,
if baseConfig, ok := configs[nogoBaseConfigName]; ok {
currentConfig = baseConfig
}
if currentConfig.excludeFiles == nil {
// TODO(linzhp): add a test to verify the case of [].
currentConfig.excludeFiles = []*regexp.Regexp{_externalFilePattern}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this, most repos no longer need the JSON config unless they want to exclude certain paths in the repo. @fmeum

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant