Skip to content
This repository has been archived by the owner on Jul 31, 2022. It is now read-only.

Potential false positive: variable used in multiple if statements #23

Closed
sbueringer opened this issue Nov 12, 2021 · 5 comments
Closed

Comments

@sbueringer
Copy link

sbueringer commented Nov 12, 2021

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.

@esimonov
Copy link
Owner

Could you please advise what version of golangci-lint are you using? I ran v1.42.1 on the following function ten times in a row and it didn't report any findings.

func notUsed_IfStmt_MultipleIfStmt_OK() {
	err := getValue()
	isDeleteNodeAllowed := err == nil

	if isDeleteNodeAllowed {
		fmt.Println("abc")
	}

	if isDeleteNodeAllowed {
		fmt.Println("def")
	}
}

@sbueringer
Copy link
Author

I'm using v1.43.0

@sbueringer
Copy link
Author

sbueringer commented Nov 17, 2021

Sorry as a full example:

main.go:

package main

import (
	"fmt"
)

func main() {
	err := fmt.Errorf("test")
	isDeleteNodeAllowed := err == nil

	if isDeleteNodeAllowed {
		fmt.Println("abc")
	}

	if isDeleteNodeAllowed {
		fmt.Println("def")
	}
}

.golangci.yml:

linters:
  disable-all: true
  enable:
  - ifshort

linters-settings:
  ifshort:
    # Maximum length of variable declaration measured in number of characters, after which linter won't suggest using short syntax.
    max-decl-chars: 50

output:

main.go:11:2: variable 'isDeleteNodeAllowed' is only used in the if-statement (main.go:9:2); consider using short syntax (ifshort)
        isDeleteNodeAllowed := err == nil

@Walz
Copy link

Walz commented Dec 6, 2021

I encountered a similar issue, I've been able to reproduce it with the following code:
main.go

package main

func main() {
	x := 0

	for i := 0; i < 10; i++ {
		if i%2 == 0 {
			continue
		} else {
			x++
		}
	}

	if x > 0 {
		println(x)
	}
}

And the output of golangci-lint:

main.go:5:2: variable 'x' is only used in the if-statement (cmd/main.go:15:2); consider using short syntax (ifshort)
	x := 0
	^

x is used in the else and the second if.
I'm using golangci-lint 1.43.0.

@korya
Copy link

korya commented Jan 9, 2022

Another case where a similar false positive occurs:

err := doSomething()

doSomethingElse()

if err != nil {
    fmr.Printf("Failed: %s\n", err)
}

I am using golangci-lint v1.43.0 as well.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants