Go CheckLocks Analyzer | Hacker News Reader

Go CheckLocks Analyzer

github.com

94 pointslima19 comments

I like Go, and it’s nice to see more safety being added, but it definitely feels bolted on in a less than ideal way. Soon enough we’ll have a similar tool that asserts that nilability is checked on certain fields, whereas we could have just had a language with null safety to begin with. I understand there are trade offs (for example non-nillability is add odds with useful zero defaults in some way), but I wonder what was missed that should have been built into the language and type system directly.

Tools like this are often a sign that the language was under-designed.

Yes, but it is also nice that Go is simple enough (simplistic enough, if you want) that these tools can be added on top, without requiring deep integration.

I don't know of any language where building a static analyzer requires "deep integration".

The closest I can think of is Java, where you can optionally do something during compilation with e.g. an annotation processor, which has a number of rather significant reliability and performance benefits. But that's an optional feature provided by an absurdly mature set of tools, definitely not a requirement - there are tons of java analysis tools that don't plug into javac.

If anything, this one serves as a bad example of Go's simplicity being a benefit. It's using the SSA analysis pass to do much of what a compiler does, because the compiler doesn't give you enough data to do it otherwise. It's an extremely complex workaround with only one implementation which is also built by the language developers, though it does plug into a decent framework (x/tools/go/analysis).

There’s a totally valid complaint that the ssa analysis pass and compiler’s ssa representation are different and should be merged (I have the same one, and I’m pretty sure the Go team knows that), but what’s the problem with exposing this to user analyzer tools?

The language is simple enough to allow you to write analyzers in the first place. I’m totally fine with people disliking the tool (it’s purpose built for one codebase!), but I sincerely fail to see how it serves as a bad example of Go’s simplicity.

I like the tool and it's great that it exists - I may try to attach it to a couple projects, small gaps in using mutexes and atomics have been a continual pain in projects handed between teams. The Analysis framework is broadly very successful too, and it's part of why the SSA analysis pass is fairly widely used.

It's more that 1) I don't see how it's a positive thing that much of this logic had to be duplicated externally, and 2) it's definitely not a positive thing that the one and only implementation of this (AFAIK) was built by the same organization that builds the compiler.

For 2, they're certainly in a good position to do so and I'm glad they did (it's very useful), but there being no third party equivalent seems to me like a fairly strong mark against "simplicity" - outsiders didn't figure out how to do this. If it was simple, they probably would have. And done so multiple times by now. Instead, much of the field seems to be waiting on the core team to provide things. That feels more like Rust to me (which I doubt many would claim is "simple"), except they've built an enormous amount of control into the language itself so many tools don't need to be external.

I think you’re missing context: I wrote most of this tool as part of another open source project (gVisor, which is the repository) and am not on or related to the core Go team. Google is a huge company with tens of thousands of engineers, and for all intents and purposes this is a third-party tool.

It is also not duplicating any logic that I’m aware of, beyond the tangentially-related ssa issue? (Which is valid, but I would assume a low-priority problem for the Go team.)

The tool is very much tailored to our individual project needs, so TBH I’m not sure how consumable it is. This is why I would surprised to see it here, and being used as the basis for any kind of statement about Go itself. I’m certainly open to making it consumable if there’s interest, but I don’t think it’s what folks are assuming from the title? Hope that clarifies.

the "internally developed" part was only referring to the SSA pass, yeah. sorry, that was ambiguous.

re consumable: in-struct is fine for the vast majority of cases I can think of, but my initial plans on anything like this are always "read, understand, then decide". so I'm plenty happy it's open source, it might give me ideas how to build things like this :) I'd have to dig in a lot deeper to see if it's immediately-applicable or not, but at a glance it sounds "good enough" + helps plug a very real, very dangerous gap. you may get some use if it's spread wider :)

though at this point I might just wait for generics. building an ad-hoc guarded-value will be FAR easier then. I suspect the source will help me figure out how to build a couple more specialized tools though, I haven't dug into the SSA pass much yet.

C++ [1], Java [2], Python [3], and even Rust [4] all have static analyzers. There will always be room for analysis that is beyond the compiler's purview.

[1] https://clang.llvm.org/extra/clang-tidy/checks/list.html [2] https://spotbugs.github.io/ [3] http://mypy-lang.org/ [4] https://github.com/rust-lang/rust-clippy

I think the reasonable alternative here is Clojure's metadata tagging, which would allow for more "principled" execution.

Saying "it all has to be in the typesystem" can _also_ become a cludge (see languages that have built-in null handling vs languages that just require you to use Option and the like)

This kind of checking has a long history, I've seen it first referenced in the 2006 book Java Concurrency in Practice as class annotations and field and method annotations. Clang had similar annotations for years.

I’m one of the authors of this analyzer, and it’s surprising to see it here.

To clear one misconception: some of the comments seem to be taking this as some kind of official Go tool, or using it a data point regarding the language itself. I don’t think this is justified; the analyzer is only used within the gVisor codebase, and is not an outgrowth of Go itself.

On the points raised: I’m not a languages or compiler person; I think it’s pretty neat that I can write a decently powerful static analyzer using the same standardized packages used by vet and other tools. To me, this is the strength of Go: the core is simple. I agree with the idea that it would be cool if Go had some more advanced features in this area, but I also understand why these changes are made slowly, to ensure there’s experience and evidence. In the meantime, we’re free to write tooling that meets our specific needs (and lets one scratch a fun itch).

One last thing: the README is incomplete and out of date (globals are supported, for example), but it was never really for public consumption.

Maybe this tool could be extracted into its own repo and made ready for public consumption, if it's generally useful?

Clang supports similar annotations for C++. That's being developed by Google too:

https://clang.llvm.org/docs/ThreadSafetyAnalysis.html

I like it. Allows to check some potential race conditions statically and not rely on "go test -race" or running "-race" binaries in prod for a small percent of the traffic (does anyone even do that really?)

At least for atomic access, I still believe that using wrapper types that prevent non-atomic access are the better idea: e.g. https://github.com/julienschmidt/atom (or uber/atomic).

That already ensures safe access at dev / compile-time. One explicitly states on declaration that the variable is accessed atomically by using the wrapper type. A linter could then simply check that sync/atomic isn't used directly anywhere.

Note that this also exists in the gVisor code base, the atomics support here is mostly for mixed-mode operation (read atomic, write locked) which is also common. It’s pretty new, so I still need to annotate a lot of the code.

i wonder if it might be helpful to go a step further and define an alias type that has the tag applied and accesses examined?

    // +checkatomic
    type atomicint int64

    type mystruct struct {
        count atomicint
    }

What’s nice about the current approach is that it is explicitly stated at the point of the variable declaration, and not hidden away in some far off place.

Also, it doesn’t require you to import a package, it’s completely orthogonal to the consumer of the code. They can opt in if they want, others aren’t forced to import anything.

Reply on news.ycombinator.com