Go-tools: staticcheck: warn on duplicate (identical) build constraints

[dmitshur]

(It appears vet doesn't already catch this.)

Duplicate (identical) build constraints can be harmless (have no effect), but they're certainly never right, and likely indicate a user mistake. It'd be nice to catch and report them.

Real world example: https://github.com/golang/sys/commit/14ac33bf8474b62c65cae263af2e4d3b543cc699.

Note that multiple build constraints are okay. It's only a problem if any of the build constraints are identical.

This is ok:

// +build linux darwin
// +build 386

// means (linux OR darwin) AND 386

This is not good:

// +build linux darwin
// +build darwin linux

// means (linux OR darwin) AND (darwin OR linux)

However, a tricky case to handle is something like this:

// +build linux darwin
// +build darwin 386

// means (linux OR darwin) AND (darwin OR 386)

Even though it repeats darwin twice, it's a valid set of constraints. It _may_ be simplified to:

// +build darwin linux,386

// means darwin OR (linux AND 386)

Edit: Removed an incorrect alternative simplification. I originally accidentally also wrote the following, which does not apply:

// +build darwin
// +build linux,386

// means darwin AND (linux AND 386)

But that's a pretty separate matter. (Maybe gosimple material?)

So maybe the staticcheck check can be the simple case when a build constraint can be entirely removed without changing behavior.

Reference: https://godoc.org/go/build.

[dominikh]

Checking for identical build constraints is straightforward (split on spaces, sort, compare) and a good start.

While we're already verifying build tags, we could go a step further, and flag constraints that are not satisfiable. For example:

+build linux
+build !linux

would be impossible to satisfy. This is probably not a common issue in commited code, but may be common while writing code, especially when people incorrectly assume that multiple +build lines are ORed together. Exhibit A would be @shurcooL's suggested "simplification"

// +build darwin
// +build linux,386

which is not satisfiable, as it means darwin AND linux AND 386, which is impossible.

[dominikh]

A fun challenge in flagging impossible build constraints, however, is our build system: currently, staticcheck strictly operates on packages, and packages only include files that have their build constraints satisfied.

We would need to introduce a separate pipeline that operates purely on files (or ASTs), ignoring build constraints, and not relying on type information – which aren't required for this kind of check.

[dmitshur]

Thanks for catching my mistake in the original post. I've fixed it (but kept a reference in my edit note).

it means darwin AND linux AND 386, which is impossible.

Strictly speaking, it's not impossible. It can be satisfied by specifying additional build tags. E.g.:

GOOS=linux GOARCH=386 go list -tags=darwin

Or:

go list -tags="darwin linux 386"

Of course, doing so is highly unusual, and such a program is very unlikely to build successfully due to build errors it would cause in the standard library when both linux and darwin tags are satisfied (normally, they are expected to be mutually exclusive).

[dominikh]

Impossible for all but the wicked :smiling_imp:

[tamird]

should this also flag _windows.go files with explicit // +build windows tags?

[dominikh]

@tamird It doesn't currently, and I'm not sure that it should – though there are multiple possible constellations, that'd need to be considered individually. Do you have any real-world examples of this happening?

[tamird]

It's definitely happened in CockroachDB a few times, specifically the
windows case. I think it's worthwhile.

On Sep 25, 2017 07:35, "Dominik Honnef" [e-mail] wrote:

Reopened #137 https://github.com/dominikh/go-tools/issues/137.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/dominikh/go-tools/issues/137#event-1263513705, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABdsPMLaeKf-X7MKwuRJGzCTAHlp1Jszks5sl5AKgaJpZM4Oi14l
.

[dominikh]

To expand on the different constellations: two that come to mind immediately are

1. a build tag in the file name and only the identical build tag in +build
2. a build tag in the file name and multiple +build in the file

The first case can probably be flagged (I'll have to check my corpus). I'd argue that the second case shouldn't be flagged.

[dmitshur]

I agree the second case is not worth a rigid error report (but could be okay in an editor suggestion mode where false positives are much more acceptable).

[dominikh]

Loosely related, pointed out by @mvdan: https://github.com/golang/go/commit/45c57e59072526b27673b51613f2438d35c48de2

i.e. GOOS/GOARCH checks that are always true or false due to build tags

[dominikh]

Note that the usefulness of a check like this will change once Go switches to the new syntax.