search

dgryski / semgrep-go

Go rules for semgrep and go-ruleguard
MIT License
460 stars 37 forks source link

False positive in ruleguard's unconvert #22

Open ainar-g opened 3 years ago

ainar-g commented 3 years ago

Consider this code:

mask := uint64(1 << n)

Currently, ruleguard complains:

[REDACTED]/main.go:9:10: unconvert: unnecessary conversion (ruleguard.rules.go:28)
9       mask := uint64(1 << n)

But the conversion is necessary here, because mask is a new variable, which the developer wants to be a uint64. I think such rules should consider if the left-hand-side variable is new or not.

quasilyte commented 3 years ago

I wonder why go/types would report 1 << n type as uint64 in that context. I would expect it to be untyped int since the left operand is an untyped constant 1.

It should be fixed on the ruleguard side. The rule itself looks fine. It might be some corner case that needs to be worked around to get the desired result. (Although I need to read spec a little more on that to be sure.)

quasilyte commented 3 years ago

It might be related to this: https://github.com/golang/go/issues/13061 See also: https://github.com/mdempsky/unconvert/blob/95ecdbfc0b5f3e65790c43c77874ee5357ad8a8f/unconvert.go#L570

ainar-g commented 3 years ago

@quasilyte, I think it's consistent with the Go Spec:

The right operand in a shift expression must have integer type or be an untyped constant representable by a value of type uint. If the left operand of a non-constant shift expression is an untyped constant, it is first implicitly converted to the type it would assume if the shift expression were replaced by its left operand alone.

In fact, the Spec even gives an example of this exact behaviour:

 var s uint = 33
 // …
 var k = uint64(1<<s)          // 1 has type uint64; k == 1<<33