Investigate porting to go/analysis #376
Comments
|
Conflicts with: #340 |
|
Just curious, what is the status here? You've written in other issues that you plan to do it. What are the perks that we can expect? |
|
Status: In progress. There are still some issues in go/analysis (primarily golang/go#29616) that need resolving. I have a local branch that ports a good chunk of checks to the new framework. Perks: Primarily interoperability. You will be able to use your favourite check runner (vet, staticcheck, golangci-lint, ...) and compile a version of it that incorporates all the various checks. For something like golangci-lint it means that it'll have to do less (hopefully none) patching of tools like staticcheck to make them reuse work. Furthermore, go/analysis forces analyses into a specific way of thinking (modular analyses), which enables some optimizations, such as memorizing build facts on disk, as well as incremental checking. This should drastically lower CPU time for repeated runs, and lower peak memory usage since we don't need to maintain the entire type graph and AST in memory all at once. |
|
When this is done it will integrate with Bazel's |
|
Status update: the majority of the port is done. All checks (except U1000) use the go/analysis framework. We have our own go/analysis runner (which will be used by staticcheck) which greatly reduces memory usage (where master needs 4.7 GB and 22s to check all of std, the port needs 450 MB and
The current hope is to finalize the port in June, give it intense internal testing, then release a version for public testing, before finally merging it into master, some time in July or August. Raw numbers for people who are into that:
Edit: More improvements, new numbers. |
|
@ianthehat has a version of gopls that integrates staticcheck at https://github.com/ianthehat/tools/tree/staticcheck – it uses the go/analysis rewrite. |
|
The majority of the work has been completed as of c7b3dbf – I encourage everyone to run the |
|
Holy Commit Message, Batman! I see you removed Edit: Or, you know, I could actually read the message, heh.
|
|
No need for forking; quoting the holy commit message:
|
|
(Edit by dominikh: fixed.) I ran |
|
@RaduBerinde Can you answer the following questions?
118% CPU utilization seem suspiciously low, making me wonder if this is a very slow or single-cored CPU, or if the system was already under load. |
Bonus: on master against cockroach, takes 50 seconds: |
|
I'd love to test this myself, but as is tradition at this point, cockroachdb doesn't actually build for me (https://play.golang.org/p/tos6sXux_V4) – are there any other projects with similarly bad performance characteristics? Edit: apparently the thing that fails to compile is not actually important and staticcheck happily processes |
|
If you run into any other problems, this is a guide on what is needed: https://github.com/cockroachdb/cockroach/blob/master/CONTRIBUTING.md#getting-and-building |
|
@RaduBerinde I've pushed a fix, please try again. The concrete issue was that one package ( |
|
Nice, you rock. It now runs in 50s and uses 2.4GB, which is great! |
|
(Edit by dominikh: fixed.) @dominikh It mostly spins really well! Except then I give it a file that doesn't exist, it panics: |
|
(Edit by dominikh: fixed.) I ran 00c4802 with |
|
@ainar-g Fixed in 6f1f542 Thank you for your extensive testing! |
|
Awesome! Tried it again, much better. Still see some false positives: Similar: |
|
@dominikh Confirming, the panic is gone, and the whole program mode still works on my small projects. Will check on bigger projects next week. |
|
@dominikh On a bigger project I, too, have issues with interfaces. I have the following code in tests: type errTimeout struct{}
func (errTimeout) Error() string { return "timeout" }
func (errTimeout) Timeout() bool { return true }
// …
err := &url.Error{Err: errTimeout{}}
// …
switch err := err.(type) {
case nil:
return false
case *url.Error:
return err != nil && err.Timeout()
default:
return false
}
type timeout interface {
Timeout() bool
}The new Whole Program Mode seems to not dive deep enough to notice that. It's a minor annoyance, to be fair, and I am actually surprised that the old |
|
@ainar-g I don't think we can handle this case with the new framework, not unless we load all dependencies from source code (which would defeat a lot of the performance gains). For dependencies that we don't need to load from source, we collect interfaces from export data. An unexported type may not show up in export data. A larger concern is that the check may yield different results depending on if the dependency was loaded from source or not, i.e. if we have cached them before. I'll definitely have to investigate that. |
|
As I've said, it's not a big deal for me. Just don't forget to mention that in the release notes, so that you have something to point people to when the issues start coming in :-) RaduBerinde's case seems different though. |
|
@RaduBerinde Your issues should be fixed as of a577b01 (the fix itself being 2f699b8 but subsequent commits fixing other problems.) Please let me know if that addresses all your false positives. |
|
I will merge |
|
Oops, I somehow missed your previous message. I'm going to try it ASAP. I tried to |
|
@RaduBerinde you'll need to update your version of golang.org/x/tools |
|
Ah, thanks, sorry I didn't dig far enough. Still seeing a couple of issues: This is used in the same file: A similar one: And these are part of a |
|
Also, a random question, would you say that this new version of staticcheck includes everything that |
|
The last ones went away after I added |
|
The ToSSTable issue is caused by a flaw in Haven't looked into the pflag.Value issue yet; the functions shouldn't get flagged if *localityList is ever passed to a function accepting a pflag.Value as an argument. As for golint: a subset of golint's checks is implemented in staticcheck. I think the vast majority of checks in golint are too noisy, so they're not part of staticcheck. You can find the full list at http://staticcheck.dev/docs/checks – they're in the ST category of checks. |
|
@dominikh If Go Modules are still out of the question, I think there need to be some kind of instructions to properly build HEAD or other another branch, to exclude this form of “Oh, your |
|
@ainar-g you'd think that years after Go's release people would know about |
|
@dominikh IIRC, [1]: Ironically, unless working in module mode, where you can do |
|
@ainar-g I'd be happy to explain my point of view, but this issue isn't the right place for that. Feel free to hit me up on IRC or Slack if you want to hear more. |
|
@RaduBerinde I have pushed more fixes for When I run I hope I've managed to unbreak all of unused now. For reference, the latest commit as of writing is 4187a5f. |
|
Awesome, thank you so much! Yeah, we already have whitelists for the ones you mentioned. The reflect case we have I don't think can be solved - the function names that are called through reflection come from external testfiles. |
|
|
Check if it is a) feasible b) worthwhile to migrate to the go/analysis framework.
The text was updated successfully, but these errors were encountered: