cmd/vet enhancement: Detect tautological integer comparisons

[Todd Neal]

I was interested in the vet tool, and added a feature that checks for tautological comparisons for the integer types. Running it across HEAD produces the following six warnings:

runtime/sigqueue.go:52: comparison of identifier s < 0 is always false
runtime/proc1.go:1519: comparison of identifier nmspinning < 0 is always false

cmd/asm/internal/asm/parse.go:817: comparison of identifier shift < 0 is always false
cmd/compile/internal/big/natconv.go:304: comparison of identifier nbits >= 0 is always true
runtime/signal1_unix.go:192: comparison of identifier b >= 0 is always true
runtime/signal1_unix.go:196: comparison of identifier b >= 0 is always true


As an example of the type of issue it finds, from runtime/sigqueue.go:

// Called from sighandler to send a signal back out of the signal handling thread.
// Reports whether the signal was sent. If not, the caller typically crashes the program.
func sigsend(s uint32) bool {
        bit := uint32(1) << uint(s&31)
        if !sig.inuse || s < 0 || int(s) >= 32*len(sig.wanted) || sig.wanted[s/32]&bit == 0 {
                return false
}

And from runtime/proc1.go:

       var nmspinning uint32

        if _g_.m.spinning {

                _g_.m.spinning = false

                nmspinning = xadd(&sched.nmspinning, -1)

                if nmspinning < 0 {

                        throw("findrunnable: negative nmspinning")

                }

It didn't find any major issues, just unreachable code.  If it's something of interest, I can put it up in a CL.  I'm new to go, so it may require some modifications to be more idiomatic.

- Todd

[Josh Bleecher Snyder]

> I was interested in the vet tool, and added a feature that checks for
> tautological comparisons for the integer types. Running it across HEAD
> produces the following six warnings:
>
> runtime/sigqueue.go:52: comparison of identifier s < 0 is always false
> runtime/proc1.go:1519: comparison of identifier nmspinning < 0 is always
> false
> cmd/asm/internal/asm/parse.go:817: comparison of identifier shift < 0 is
> always false
> cmd/compile/internal/big/natconv.go:304: comparison of identifier nbits >= 0
> is always true
> runtime/signal1_unix.go:192: comparison of identifier b >= 0 is always true
> runtime/signal1_unix.go:196: comparison of identifier b >= 0 is always true

Good finds. I'd say that each of these deserves a careful look and a
fix (although perhaps not for Go 1.5). Would you mind filing an issue
with these? Thanks.

I wonder why the check didn't also flag math/big/natconv.go, since the
code there is the same as command/compile/internal/big/natconv.go.

> It didn't find any major issues, just unreachable code. If it's something
> of interest, I can put it up in a CL. I'm new to go, so it may require some
> modifications to be more idiomatic.

Please do mail it as a CL. I can't promise it'll go in (the bar for
vet is quite high), but I do promise to review it carefully. And I'll
run it over a larger corpus than the standard library, to see what it
turns up. I'd also be happy to help you shape it into idiomatic code
as needed, if it looks like it is appropriate for inclusion in vet (or
even if you'd just like the feedback).

-josh

[Todd Neal]

On Thursday, June 18, 2015 at 7:12:53 PM UTC-5, Josh Bleecher Snyder wrote:

> I was interested in the vet tool, and added a feature that checks for
> tautological comparisons for the integer types. Running it across HEAD
> produces the following six warnings:
>
> runtime/sigqueue.go:52: comparison of identifier s < 0 is always false
> runtime/proc1.go:1519: comparison of identifier nmspinning < 0 is always
> false
> cmd/asm/internal/asm/parse.go:817: comparison of identifier shift < 0 is
> always false
> cmd/compile/internal/big/natconv.go:304: comparison of identifier nbits >= 0
> is always true
> runtime/signal1_unix.go:192: comparison of identifier b >= 0 is always true
> runtime/signal1_unix.go:196: comparison of identifier b >= 0 is always true

Good finds. I'd say that each of these deserves a careful look and a
fix (although perhaps not for Go 1.5). Would you mind filing an issue
with these? Thanks.

Sure thing, I'll file those issues.

 

I wonder why the check didn't also flag math/big/natconv.go, since the
code there is the same as command/compile/internal/big/natconv.go.

I checked, and it found both cases but I was manually running vet across the source code and missed it.  Is there any recommended way to "vet" the entire src/ directory?

 

> It didn't find any major issues, just unreachable code.  If it's something
> of interest, I can put it up in a CL.  I'm new to go, so it may require some
> modifications to be more idiomatic.

Please do mail it as a CL. I can't promise it'll go in (the bar for
vet is quite high), but I do promise to review it carefully. And I'll
run it over a larger corpus than the standard library, to see what it
turns up. I'd also be happy to help you shape it into idiomatic code
as needed, if it looks like it is appropriate for inclusion in vet (or
even if you'd just like the feedback).

-josh

No problem  I'll appreciate the feedback regardless of its included.  I'll submit a CL and add you as a reviewer.

- Todd

[Josh Bleecher Snyder]

> I checked, and it found both cases but I was manually running vet across the
> source code and missed it. Is there any recommended way to "vet" the entire
> src/ directory?

go vet ./...

and

go tool vet .

should both work (from within the src directory)

>> > It didn't find any major issues, just unreachable code. If it's
>> > something
>> > of interest, I can put it up in a CL. I'm new to go, so it may require
>> > some
>> > modifications to be more idiomatic.
>>
>> Please do mail it as a CL. I can't promise it'll go in (the bar for
>> vet is quite high), but I do promise to review it carefully. And I'll
>> run it over a larger corpus than the standard library, to see what it
>> turns up. I'd also be happy to help you shape it into idiomatic code
>> as needed, if it looks like it is appropriate for inclusion in vet (or
>> even if you'd just like the feedback).
>>
>> -josh
>
>
> No problem I'll appreciate the feedback regardless of its included. I'll
> submit a CL and add you as a reviewer.

Great. I look forward to it.

-josh