Skip to content
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

x/tools/go/analysis/internal: Add an option to emit diagnostics for external packages #38177

Open
Matts966 opened this issue Mar 31, 2020 · 4 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@Matts966
Copy link

What version of golang/tools are you using?

~/tools $ git rev-parse HEAD
a30bf2db82d4f645ded9030efbbf6d4fbe5e70eb

Proposal

Currently the analysis driver in golang/tools prints diagnostics only for root packages. We can't change this behavior without changing the golang/tools repository or developing a driver from scratch because this behavior is written in the internal package.

Some analyses could be more beneficial if they could emit diagnostics for dependencies because problems in dependencies can also be problems in the root packages.

So I would like to implement an option to emit diagnostics for external packages by adding an argument to golang.org/x/tools/go/analysis/internal/checker.Run, and adding a command-line argument to golang.org/x/tools/go/analysis/singlechecker.Run and golang.org/x/tools/go/analysis/multichecker.Run.

Is it OK, or are there any reasons why the option is not in the packages?

Thank you.

@gopherbot gopherbot added this to the Unreleased milestone Mar 31, 2020
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Mar 31, 2020
@andybons
Copy link
Member

@matloob @alandonovan

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 31, 2020
@alandonovan
Copy link
Contributor

(By "external packages", I assume you mean dependencies.)

The driver runs the requested analyzers (plus any indirectly required due to Requires) on dependencies only if facts are needed from analyzing lower-level packages; most analyzers don't actually need to look at packages other than the root.

Doesn't some variant of this command do what you need?

$ go vet $(go list -deps myproj/...)

@Matts966
Copy link
Author

Matts966 commented Apr 3, 2020

@matloob, @alandonovan
I tested the command & understand the workaround can what I need (linting all dependencies recursively). Thank you for the suggestion 🙇

However, I found some differences.
I compared that workaround with my copied and modified driver like below.

# with modified driver
~/knil$ time knil golang.org/x/tools/... 2>&1 | wc -l
   15800

real    0m18.837s
user    0m47.938s
sys     0m4.163s

# with `go list -deps`
~/knil$ time knil $(go list -deps golang.org/x/tools/...) 2>&1 | wc -l
   15159

real    0m45.245s
user    1m41.810s
sys     0m14.848s

I don't know the cause of the difference of line count returned by wc (maybe the version differences of dependencies?), however, the performance difference is probably because the current driver double-checks the dependencies both as roots and dependencies.

This performance difference probably will not be resolved by the workaround, and we should prepare the option to optimize the analysis?

@alandonovan
Copy link
Contributor

alandonovan commented Apr 3, 2020

Thanks for the report. I'm not surprised it's slower. The differences are that:

  1. the default command runs all analyses on the root packages, and modular analyses (the few that use Facts) on all dependencies too. It then reports diagnostics just for the root packages.
  2. your fullchecker does the same, but reports diagnostics for the dependencies. These will necessarily include only the diagnostics produced by modular analyses.
  3. the 'go list' workaround runs all analyses on all packages and prints all diagnostics.

#1 and #3 are easy to explain. #2 is not. That's why I don't want to add a flag.

If you're feeling motivated, it would be good to profile #3 and see if there are easy performs gains to be made.

@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

5 participants