indent-error-flow incorrectly identifying conditional attribution as redundant

[sonalys]

Describe the bug
When we declare variables inside if statements, and returns inside the if block, on the else statement, we can no longer use other declared variables because revive detects indent-error-flow redundancy.

To Reproduce
Steps to reproduce the behavior:

1. I updated revive go get -u github.com/mgechev/revive
2. I run it with the following flags & configuration file:

if a, b := 1, 2; a != 1 {
  return a
} else {
 return b
}

Expected behavior
This should not be redundant, because we are only declaring those variables inside the if statement, and it should not be possible to access b without the else statement.

[chavacava]

Hi @sonalys, thanks for filling the issue.

For the example code you provided, revive should generate a message like:

if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

As the message says, if necessary, you'll need to move the variable declaration to its own line. Something like:

a, b := 1, 2
if a != 1 {
  return a
} 
return b

The current implementation of the rule in revive is from the original one from golint and is based the on a simple structural analysis (in the lines of "if the then block ends with a structured jump (return, break, continue) and there is an else branch then warn")
There was an discussion on this subject in the golint repo but the GO team closed the subject without modifying golint behavior.

One common critic to the refactoring proposed by golint is that now the scope of the variables is not constrained to the if branches. To alleviate that one could refactor to:

{
  a, b := 1, 2
   if a != 1 {
    return a
  }  
  return b
}

Note: avoiding warnings in cases like those of your example will require to add a data flow analysis ("any variable defined in the if is used in the then branch?")

[sonalys]

yes, improving the message would be great, because it doesn't seem to justify the behaviour it wants with the actual message
if it considered the declaration of variables inside the if scope it would be great, because sometimes it's really bothersome to already have an error declared above in the code, and you need a new variable like in the code below:

err := returnsError()
if err != nil {
  return err
}

myNewVar, err = myFuncThatReturnsVarAndError()
if err != nil {
 return err
}

because we will need to either declare myNewVar outside the if-statement, or make the err variable to be scoped.
if I want to do something like

if myNewVar, err := myFuncThatReturnsVarAndError(); err != nil {
   return err
} else {
  useMyNewVarIntoSomethingBecauseItsStillInScope(myNewVar)
}

it will not work

I think pre-declaring this stuff outside if-statements can make the code more difficult to read and bigger, an example of the first case

var err error
err = returnsError()
if err != nil {
  return err
}

var myNewVar MyType
myNewVar, err = myFuncThatReturnsVarAndError()
if err != nil {
...
}

when it could be

if err := returnsError(); err != nil {
 return err
}

if myNewVar, err := myFuncThatReturnsVarAndError(); err != nil {
  return err
} else {
  useMyNewVarIntoSomethingBecauseItsStillInScope(myNewVar)
}

[chavacava]

As I've said, for the example you provided, revive already generates a message in the lines of:

if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

If in your case is not generating such a message, please provide the actual code you are linting.

There are many subjects that are a matter of personal taste, thus which is more easy to read

if myNewVar, err := myFuncThatReturnsVarAndError(); err != nil {
   return err
} else {
  useMyNewVarIntoSomethingBecauseItsStillInScope(myNewVar)
}

or

myNewVar, err := myFuncThatReturnsVarAndError()
if err != nil {
   return err
} else {
  useMyNewVarIntoSomethingBecauseItsStillInScope(myNewVar)
}

It is not something we will discuss :)
Again, you can fix the scoping problem by creating a new scope by surrounding the declaration and the if statement with { and }... whether this fix is ugly, nice, ... is up to you.

[chavacava]

Not a bug. revives behaves identically to golint.
The subject was already discussed in various golint issues and the GO team always decided to stick to the current behavior.