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
cmd/vet: PackageVetx in vet.cfg lists full transitive closure of package dependencies #30296
Comments
/cc @mvdan @alandonovan |
As with type information, analysis facts also include information about methods and struct fields or imported types, even though they might come from indirectly imported packages. But it should still be much less than the complete transitive closure in most cases. What command did you run, exactly? |
I'm not an expert in go compilation process, so forgive me if i'm wrong. But it seems that in the case of type information all indirect typeinfo is "pulled-along" into .a files. So that only direct dependencies are needed for compilation. Rob Pipe specifically mentions that fact in his talk https://youtu.be/rKnDgT73v8s?t=880 For example, see https://github.com/slon/vet/blob/master/inner/b.go . This package implicitly uses io.Writer interface. But go compiler is able to build this package without
|
@alandonovan what do you think? |
Thanks. It does appear that the set of .vetx files equals the transitive closure:
I didn't write this code (see https://tip.golang.org/src/cmd/go/internal/work/exec.go#L1009); it's possible that it was necessary in the original version of vetx but I would expect the set of keys of PackageFile is all that is needed. (Even the "fmt' entry, which is artificially inserted, need no longer be treated specially for vet.) |
@dmitshur is there any way I can help with investigation? |
@slon Sure. Take a look at https://golang.org/wiki/HandlingIssues to become familiar with what's needed to transition to the next state of NeedsDecision or NeedsFix. Alan has started investigating this and pinged two other people who are familiar with cmd/vet. You should see if there's some information or understanding they need that you can help provide. You might have to wait for them to have a chance to look into this first. |
I am not actively investigating this. My advice would be to try to change the logic in cmd/go so that it provides the exact same set of .vetx files to vet as it provides .a files to the compiler, then run all the vet tests. |
The fix turned out very small.
After rebuilding toolchain with
All tests in master branch of Tried running
|
Change https://golang.org/cl/164459 mentions this issue: |
@alandonovan would you please assist me with getting this change merged? Looks like my CL was not done properly, since it is still open without reply :( |
@slon, thanks for the fix. Is there anything remaining to do here, or should we close this issue? |
Updates #30296 Change-Id: Ifea1a4c82c1c5b31fdc2e96fdbb1274748c8f50e Reviewed-on: https://go-review.googlesource.com/c/go/+/164459 Reviewed-by: Bryan C. Mills <bcmills@google.com>
We're done. Thank you. |
We are building custom analysis driver for golang.org/x/tools/go/analysis.
Seems like current behaviour of go vet command contradicts documentations.
This paragraph states, that each pass has access only to facts from directly imported packages.
Each pass (a, p) starts with the set of facts produced by the same analyzer a applied to the packages directly imported by p.
I would expect, that
PackageVetx
section ofvetx.cfg
should contain the same number of entries as thePackageFile
section.But
go vet
driver lists.vetx
files from full transitive closure of package dependencies. That seems less efficient and might cause problems with analysis scalability on large projects or in distributed build setting.Could you please clarify, what the intended behaviour should be?
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes.
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
go vet -n bytes
What did you expect to see?
PackageVetx section of vet.cfg should list only directly imported packages.
What did you see instead?
PackageVetx contains full transitive closure of package dependencies.
The text was updated successfully, but these errors were encountered: