ifelse: option to preserve variable scope

[mdelah]

The idea is to give users a option to suppress prompts of the kind indicated in #816, where the benefit of refactoring may not obviously outweigh the cost of increasing variable scope.

The logic as it stands, is that the early-return / indent-error-flow / superfluous-else rule will be skipped if preserveScope is given as a configuration argument and:

• At least one if the if blocks in the chain has an if-initializer (i.e. we bring variables defined in the initializer into the surrounding scope), or
• The block to be de-indented contains at least one declaration (i.e. we bring variables defined at the top level of the "if" or "else" branch body into the surrounding scope), and
• The if-else chain is not located at the end of its own surrounding block (since in that case the relocated variables would go out of scope at the same point either way)

This is a slightly refinement on what I initially had in mind (ignore any chain with an if-initializer), but hopefully it makes sense. Tests have been added to cover the new behaviour. If not enabled, the rules work the same as before. Open to suggestions on the naming/wording.

Closes #816.

[chavacava]

Hi @mdelah, thanks for the PR!

I'm testing the new code and I have a case that seems not to be working OK: Config:

ignoreGeneratedHeader = false
severity = "warning"
confidence = 0.8
errorCode = 0
warningCode = 0

[rule.early-return]
    arguments = ["preserveScope"]

Input code (testdata/early-return.go):

// Test of empty-blocks.

package fixtures

func earlyRet() bool {
    if ok := call(); ok {
        println()
    } else {
        return nil
    }
}

Output:

testdata/early-return.go:6:2: if c { ... } else { ... return } can be simplified to if !c { ... return } ... (move short variable declaration to its own line if necessary)

My understanding of your proposal is that with the above configuration (preserveScope) the rule must not spot the problem. I'm right?

[mdelah]

Hi @chavacava, indeed, that code would fall under the third bullet point above ("The if-else chain is not located at the end of its own surrounding block..."). The enclosing function ends afterwards. I figured it would still be helpful to show the suggestion there, because ok will fall out of scope anyway. I think if you add more code at the end of the function, it will suppress the message.

What do you think, is it reasonable or does it muddy the water too much? Should the docs articulate this more clearly?

[chavacava]

Thanks @mdelah !

[gbjk]

This fix is great and solves false positives for us when using the linter.

Is there a schedule for when this will be released though, please?