esimonov / ifshort Goto Github PK
View Code? Open in Web Editor NEWGo linter for checking that your code uses short syntax for if-statements whenever possible.
License: MIT License
Go linter for checking that your code uses short syntax for if-statements whenever possible.
License: MIT License
I'm using this via golangci-lint v1.37.1:
All the code can be found in kopia/kopia@113670e
I'm listing 4 cases here but there a few more there - I'd appreciate if you could take a look.
internal/parallelwork/parallel_work_queue.go:127:2: variable 'cb' is only used in the if-statement (internal/parallelwork/parallel_work_queue.go:128:2); consider using short syntax (ifshort)
cb := v.ProgressCallback
^
Notice how cb
is later invoked:
func (v *Queue) maybeReportProgress(ctx context.Context) {
cb := v.ProgressCallback
if cb == nil {
return
}
if clock.Now().Before(v.nextReportTime) {
return
}
v.nextReportTime = clock.Now().Add(1 * time.Second)
cb(ctx, v.enqueuedWork, v.activeWorkerCount, v.completedWork)
}
fs/entry.go:141:2: variable 'name' is only used in the if-statement (fs/entry.go:151:2); consider using short syntax (ifshort)
name := newEntry.Name()
Notice how name
is used in a closure passed to sort.Search()
and in one more if statement:
func (e Entries) Update(newEntry Entry) Entries {
name := newEntry.Name()
pos := sort.Search(len(e), func(i int) bool {
return e[i].Name() >= name
})
// append at the end
if pos >= len(e) {
return append(append(Entries(nil), e...), newEntry)
}
if e[pos].Name() == name {
if pos > 0 {
return append(append(append(Entries(nil), e[0:pos]...), newEntry), e[pos+1:]...)
}
return append(append(Entries(nil), newEntry), e[pos+1:]...)
}
if pos > 0 {
return append(append(append(Entries(nil), e[0:pos]...), newEntry), e[pos:]...)
}
return append(append(Entries(nil), newEntry), e[pos:]...)
}
tests/testenv/cli_test_env.go:73:2: variable 'exe' is only used in the if-statement (tests/testenv/cli_test_env.go:74:2); consider using short syntax (ifshort)
exe := os.Getenv("KOPIA_EXE")
Notice how exe
is used in the return statement:
// NewCLITest creates a new instance of *CLITest.
func NewCLITest(t *testing.T) *CLITest {
t.Helper()
exe := os.Getenv("KOPIA_EXE")
if exe == "" {
// exe = "kopia"
t.Skip()
}
/* a bunch of lines removed */
return &CLITest{
startTime: clock.Now(),
RepoDir: testutil.TempDirectory(t),
ConfigDir: configDir,
Exe: filepath.FromSlash(exe),
fixedArgs: fixedArgs,
DefaultRepositoryCreateFlags: formatFlags,
LogsDir: logsDir,
Environment: []string{
"KOPIA_PASSWORD=" + TestRepoPassword,
"KOPIA_ADVANCED_COMMANDS=enabled",
},
}
}
cli/command_diff.go:35:5: variable 'isDir1' is only used in the if-statement (cli/command_diff.go:38:2); consider using short syntax (ifshort)
_, isDir1 := ent1.(fs.Directory)
^
cli/command_diff.go:36:5: variable 'isDir2' is only used in the if-statement (cli/command_diff.go:38:2); consider using short syntax (ifshort)
_, isDir2 := ent2.(fs.Directory)
^
Notice how isDir1 is used close to the end of the function:
func runDiffCommand(ctx context.Context, rep repo.Repository) error {
ent1, err := snapshotfs.FilesystemEntryFromIDWithPath(ctx, rep, *diffFirstObjectPath, false)
if err != nil {
return errors.Wrapf(err, "error getting filesystem entry for %v", *diffFirstObjectPath)
}
ent2, err := snapshotfs.FilesystemEntryFromIDWithPath(ctx, rep, *diffSecondObjectPath, false)
if err != nil {
return errors.Wrapf(err, "error getting filesystem entry for %v", *diffSecondObjectPath)
}
_, isDir1 := ent1.(fs.Directory)
_, isDir2 := ent2.(fs.Directory)
if isDir1 != isDir2 {
return errors.New("arguments do diff must both be directories or both non-directories")
}
d, err := diff.NewComparer(os.Stdout)
if err != nil {
return errors.Wrap(err, "error creating comparer")
}
defer d.Close() //nolint:errcheck
if *diffCompareFiles {
parts := strings.Split(*diffCommandCommand, " ")
d.DiffCommand = parts[0]
d.DiffArguments = parts[1:]
}
if isDir1 {
return d.Compare(ctx, ent1, ent2)
}
return errors.New("comparing files not implemented yet")
}
I like to use the build pattern approach where function calls are 'just' concatenated without storing intermediate results to configure something and then generate the result.
In case enough calls are concatenated (~30) then the ifshort linter performance degrades drastically with each added call.
package main
func main() {
t := thing{}
t.add().add().add().add().add().add().add().add().add().add().add().add().add().
add().add().add().add().add().add().add().add().add().add().add().add().add()
}
type thing struct {
}
func (t thing) add() thing {
return t
}
When I then call the linter
./ifshort -debug vfpst .
The run takes ~ 900ms but when I add more calls the performance degrades drastically:
We had a builder with enough calls that lead to a execution of ifshort time of > 20m resulting in our pipeline to timeout.
And it took a while to figure out what is the actual problem.
go version go1.17.7 linux/amd64
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/m/.cache/go-build"
GOENV="/home/m/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/m/work/gocode/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/m/work/gocode"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17.7"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/m/work/n/ifshort-bug/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1585209923=/tmp/go-build -gno-record-gcc-switches"
10:12:40.122061 load [.]
10:12:40.193156 building graph of analysis passes
1m43.554850346s ifshort@ifshort-bug
package main
func main() {
t := thing{}
t.add().add().add().add().add().add().add().add().add().add().add().add().add().
add().add().add().add().add().add().add().add().add().add().add().add().add()
}
type thing struct {
}
func (t thing) add() thing {
return t
}
considering this:
func (r *Request) Parameter(name RequestParameter) (string, bool) {
idx := r.FindParameter(name)
if idx < 0 {
return "", false
}
return r.ParamsMembers[idx].Value, true
}
report this:
request.go:48:2: variable 'idx' is only used in the if-statement (request.go:50:2); consider using short syntax (ifshort)
Hi,
linter returns false negative on a following code:
package main
import "fmt"
func main() {
i := 2
if int(i) != 2 {
fmt.Println("i != 2")
}
}
This code fails with transaction/query.go:35:2: variable 'zero' is only used in the if-statement (transaction/query.go:36:2); consider using short syntax
linter error:
type Query struct {
LastUpdatedAt time.Time
}
func (q Query) appendToSQL(sqlBuilder *strings.Builder, sqlParams []interface{}) []interface{} {
zero := time.Time{}
if q.LastUpdatedAt != zero {
}
return sqlParams
}
If I implement linter recommendation - compilation will fail:
type Query struct {
LastUpdatedAt time.Time
}
func (q Query) appendToSQL(sqlBuilder *strings.Builder, sqlParams []interface{}) []interface{} {
if zero := time.Time{}; q.LastUpdatedAt != zero {
}
return sqlParams
}
Compile time error:
transaction/query.go:36:10: syntax error: cannot use zero := time.Time as value
transaction/query.go:37:3: syntax error: unexpected if, expecting expression
transaction/query.go:45:2: syntax error: non-declaration statement outside function body
Environment:
❯ go version
go version go1.17.6 linux/amd64
❯ golangci-lint --version
golangci-lint has version 1.44.0 built from 617470fa on 2022-01-25T11:31:17Z
False positive using labeled breaks
package main
func main() {
foo := true
BREAKOUT:
for i := range []int{0, 1, 2} {
for j := range []int{0, 1, 2} {
if j == 2 && i == 2 {
foo = false
break BREAKOUT
}
}
}
if foo {
return
}
}
$ ifshort main.go
/tmp/main.go:4:2: variable 'foo' is only used in the if-statement (/tmp/main.go:14:2); consider using short syntax
The following (oversimplified) example produces a false positive:
type output struct {
err error
str string
}
func getValue(ch chan output) ([]string, error) {
out := <-ch
if out.err != nil {
return []string{}, out.err
}
return []string{out.str}, nil
}
In our code there are more actions happening between the two returns, but the function signature is required to be like this to satisfy interface requirements
Using the ifshort v1.36.0 packaged with golangci-lint on the following code
package test
import "fmt"
func test() {
p := map[string]int{}
q := "bar"
if _, ok := p[q]; ok {
fmt.Println(q)
}
}
elicits this warning
variable 'q' is only used in the if-statement (test.go:9:2); consider using short syntax (ifshort)
which is all good, but I do not off-hand see how that would work since the if
statement already is using the short form, and there is dependency so that the q
needs to be set first.
I can see some horrible ways of constructing small helper functions just for this one spot, but as I said, horrible.
While whittling this down from the original code, I noticed that using the q
inside the if
is important. If the q
is not used there, the ifshort
warning goes away.
This appears similar to #4, but in this case the rewrite is far from obvious:
line := <-out
if got := line[strings.Index(line, " INFO")+1:]; got != want {
// Only validate the message: other tests cover timestamp
t.Fatalf("got %v, want %v", got, want)
}
as defining two dependent variables using multiple assignments is a challenge (barring refactoring the logic that produces "got" into a separate function, anonymous or otherwise).
The code below generates this warning:
internal/rbd/rbd_util.go:1619:2: variable 'removeOldKey' is only used in the if-statement (internal/rbd/rbd_util.go:1639:2); consider using short syntax (ifshort)
removeOldKey := false
^
removeOldKey
is initialized to false
, but only set to true
in one case inside the switch statement. The warning does not seem to be correct to me (or I fail to see how to rewrite it to a shorter syntax).
// MigrateMetadata reads the metadata contents from oldKey and stores it in
// newKey. In case oldKey was not set, the defaultValue is stored in newKey.
// Once done, oldKey will be removed as well.
func (ri *rbdImage) MigrateMetadata(oldKey, newKey, defaultValue string) (string, error) {
value, err := ri.GetMetadata(newKey)
if err == nil {
return value, nil
} else if !errors.Is(err, librbd.ErrNotFound) {
return "", err
}
// migrate contents from oldKey to newKey
removeOldKey := false
value, err = ri.GetMetadata(oldKey)
switch {
case err == nil:
// oldKey was fetched without error, remove it afterwards
removeOldKey = true
case errors.Is(err, librbd.ErrNotFound):
// in case oldKey was not set, set newKey to defaultValue
value = defaultValue
default:
return "", err
}
// newKey was not set, set it now to prevent regular error cases for missing metadata
err = ri.SetMetadata(newKey, value)
if err != nil {
return "", err
}
// the newKey was set with data from oldKey, oldKey is not needed anymore
if removeOldKey {
err = ri.RemoveMetadata(oldKey)
if err != nil {
return "", err
}
}
return value, nil
}
False positive warning: variable 'v' is only used in the if-statement (main.go:17:2); consider using short syntax
when variable is used in the inner struct:
package main
import "log"
type (
T1 struct {
T2 []T2
}
T2 struct {
V string
}
)
func main() {
v := "value"
if v == "" {
v = "-"
}
t := T1{
T2: []T2{
{
V: v,
},
},
}
log.Println(t)
}
Hello!
package main
import "fmt"
func main() {
flag := false
for i := 0; i < 10; i++ {
switch {
case false:
continue
case true:
flag = true
}
}
if flag {
fmt.Println("hello")
}
}
golangci-lint run main.go
main.go:6:2: variable 'flag' is only used in the if-statement (main.go:16:2); consider using short syntax (ifshort)
flag := false
^
err := c.s.Send(msg)
c.sendM.Unlock()
if err != nil {
c.close(errors.Wrap(err, "failed to send message"))
return
}
In general, I think ifshort should skip cases when there is some code between assignment and if
package main
func main() {
_ = bug(value("nil"))
}
type value string
func (m value) String() string {
return string(m)
}
type myType struct {
value *string
}
func bug(v value) *myType {
testVar := v.String()
if testVar == "nil" {
return nil
}
return &myType{value: &testVar}
}
The use as a pointer inside the struct is not detected.
variable 'testVar' is only used in the if-statement (ifshortbug.go:19:2); consider using short syntax (ifshort)
ifshort complains about a waitgroup beeing used only once but in fact it is used multiple times afterwards:
lmd/listener.go:208:2: variable 'wg' is only used in the if-statement (lmd/listener.go:222:2); consider using short syntax (ifshort)
wg := &sync.WaitGroup{}
func SendCommands(commandsByPeer map[string][]string) (code int, msg string) {
code = 200
msg = "OK"
resultChan := make(chan error, len(commandsByPeer))
wg := &sync.WaitGroup{}
for pID := range commandsByPeer {
PeerMapLock.RLock()
p := PeerMap[pID]
PeerMapLock.RUnlock()
wg.Add(1)
go func(peer *Peer) {
defer logPanicExitPeer(peer)
defer wg.Done()
resultChan <- peer.SendCommandsWithRetry(commandsByPeer[peer.ID])
}(p)
}
if waitTimeout(wg, PeerCommandTimeout) {
code = 202
msg = "sending command timed out but will continue in background"
return
}
...
}
Original code can be found here:
https://github.com/sni/lmd/blob/f6eefbe1af8012d29998e122441c3c705d648a3d/lmd/listener.go#L204
package main
type test struct {
}
func (t test) Test() test {
return t
}
func main() {
_ = test{}.
Test().Test().Test().Test().Test().Test().Test().Test().Test().Test().
Test().Test().Test().Test().Test().Test().Test().Test().Test().Test().
Test().Test().Test().Test().Test().Test().Test().Test().Test().Test().
Test().Test()
}
For the code above linter runs for 40s, when adding couple more calls to Test() it eventually timeouts with error:
golangci-lint run --disable-all -E ifshort
ERRO Timeout exceeded: try increasing it by passing --timeout option
in reffer to golangci/golangci-lint#2662 :
the following code causes false-positive report:
package main
import (
"fmt"
)
type state struct {
x int32
}
type other struct {
int32
}
func getState() *state {
return &state{}
}
func main() {
state := getState()
if state.x == 1 {
fmt.Println("it is 1")
}
_ = other{
int32(state.x),
}
}
I've included the full listing at the end, but it's essentially this:
func Example1() error {
cfg := &Config{}
cfg, err := cfg.Update()
if err != nil {
fmt.Println(err)
}
fmt.Println(cfg)
return nil
}
$ ifshort ifshortbug.go
/tmp/ifshortbug.go:19:7: variable 'err' is only used in the if-statement (/tmp/ifshortbug.go:20:2); consider using short syntax
/tmp/ifshortbug.go:33:7: variable 'err' is only used in the if-statement (/tmp/ifshortbug.go:34:2); consider using short syntax
Note that if I change the Update
func to return a value instead of a pointer, the false positive is not seen.
Full listing:
package main
import (
"fmt"
)
type Config struct {
V int
}
func (c *Config) Update() (*Config, error) {
r := *c
r.V++
return &r, nil
}
func Example1() error {
cfg := &Config{}
cfg, err := cfg.Update()
if err != nil {
fmt.Println(err)
}
fmt.Println(cfg)
return nil
}
func UpdateConfig(c *Config) (*Config, error) {
return c.Update()
}
func Example2() error {
cfg := &Config{}
cfg, err := UpdateConfig(cfg)
if err != nil {
fmt.Println(err)
}
fmt.Println(cfg)
return nil
}
func main() {
Example1()
Example2()
}
Consider following diff:
diff --git internal/util/util_test.go internal/util/util_test.go
index 3e9eb1e..b47a3da 100644
--- internal/util/util_test.go
+++ internal/util/util_test.go
@@ -64,8 +64,7 @@ func TestPickIntFirst(t *testing.T) {
func TestIndent(t *testing.T) {
t.Parallel()
- expected := " foo"
- if a := Indent("foo", " "); a != expected {
+ if a, expected := Indent("foo", " "), " foo"; a != expected {
t.Fatalf("expected '%s', got '%s'", expected, a)
}
}
While it is true that expected
is only used in if
block, I think moving it's declaration into if
statement makes it very confusing, harder to read and generally uncommon.
Is this behavior intentional? I have doubts that this suggestion should be promoted.
func Run(tasks []Task, n, m int) error {
chTasks := make(chan Task)
chErrSignal := make(chan struct{}, len(tasks))
done := make(chan struct{})
careAboutErrors := m > 0
var errCounter int
go func() {
defer close(chTasks)
for _, task := range tasks {
select {
case <-chErrSignal:
if careAboutErrors {
errCounter++
}
default:
}
if careAboutErrors && errCounter >= m {
close(done)
return
}
chTasks <- task
}
}()
var wg sync.WaitGroup
wg.Add(n)
for i := 0; i < n; i++ {
go worker(done, chTasks, chErrSignal, &wg)
}
wg.Wait()
close(chErrSignal)
if careAboutErrors && errCounter >= m {
return ErrErrorsLimitExceeded
}
return nil
}
check output:
variable 'careAboutErrors' is only used in the if-statement (run.go:48:2); consider using short syntax (ifshort)
careAboutErrors := m > 0
Not sure if this case is covered already:
func examples() []string {
field := os.Getenv("FIELD")
if field == "" {
field = "foo"
}
return []string{field}
}
results in:
variable 'field' is only used in the if-statement (example.go:2:2); consider using short syntax (ifshort)
field := t.Field
^
This is the shortest example I could boil down to, but found the issue using field
as a map key, and in other ways in the return. Not sure if these are all the same issue, happy to provide more examples.
False positive:
package main
import (
"flag"
"fmt"
"time"
)
func main() {
delayPtr := flag.Duration("delay", 0, "")
flag.Parse()
delay := *delayPtr
if delay == 0 {
delay = time.Second
}
select {
case <-time.After(delay):
fmt.Println("hello")
}
}
Linter error:
example/main.go:12:2: variable 'delay' is only used in the if-statement (example/main.go:13:2); consider using short syntax (ifshort)
delay := *delayPtr
^
In a larger example, you can imaging a for-select loop where there is another case like checking if a context is cancelled.
This is possibly related to #17 except instead of assigning to the variable in the select statement, it is read.
I'm not sure if it's intentional but the following code produces a finding:
isDeleteNodeAllowed := err == nil
if isDeleteNodeAllowed {
fmt.Println("abc")
}
if isDeleteNodeAllowed {
fmt.Println("def")
}
controllers/machine_controller.go:278:2: variable 'isDeleteNodeAllowed' is only used in the if-statement (controllers/machine_controller.go:280:2); consider using short syntax (ifshort)
isDeleteNodeAllowed := err == nil
^
(full original code can be seen here, but the short version above also produces the finding)
In case it's not intentional, I think the problem is that getNamedOccurrenceMap=>addFromCondition
only adds one occurrence for the first if. The second if is not added as additional occurrence because addFromIdent=>occ.isComplete
returns true.
Interestingly this issue is only reported in ~90-95% of the cases when ifshort is run.
Context: We are running ifshort via golangci-lint and we are also running the nolint linter.
For some reason that finding (on the original code) is not reported on every run. We've added a nolint statement, but in those cases where ifshort does not report the finding we'll get a finding that the nolint statement is not required.
the following code
package main
import (
"context"
"fmt"
)
func main() {
ctx := context.Background()
value := ctx.Value("key")
if value == nil {
panic("")
}
fmt.Println(value.(string))
}
produces this error (when running through golangci-lint
version 1.36.0);
test.go:11:2: variable 'value' is only used in the if-statement (test.go:12:2); consider using short syntax (ifshort)
value := ctx.Value("key")
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.