Consider relaxing redundant assignment if it is part of a sequence

[ijc]

After #319 (motivated by #312) a function of the form:

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

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

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

return nil

Now complains about the doAThirdThing block and suggests transforming it into return doAThirdThing() making the function:

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

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

return doAThirdThing()

Which I think is a regression in terms of code readability, it makes it non-uniform and implies that doAThirdThing is somehow different or special to what came earlier. It also means that adding more steps results in needing to transform this line (for similar reasons Go insists that the last line in a struct literal still has a trailing , so it doesn't need to be touched if the struct is extended).

Perhaps the heuristic could be tweaked to only emit the warning if the block is not proceeded by a similar block?

[sjpotter]

yep, just ran into this as well, honestly, it makes the code a pain. Its a bad check. If we expect to add doAFourthThing or more, every time we'd have to do a significant edit on the code, is this really what's desired?

[matryer]

I completely agree, but I don't think this rule ever really serves much of a purpose.

I personally find this whole rule to go against the philosophy of Go to some extent. I get that it's about redundant code which we don't like, but let's explore the point from @ijc regarding the Go comma.

Consider this code:

[]string{
  "one",
  "two",
  "three",
}

The true value of allowing the comma on the final item is:

• you can add items without touching the previous line,
• you can remove the final item and don't have to remember to go and remove the comma,
• you can reorder the lines without having to interfere with the commas,
• diffs look nicer,

The same should apply to bocks of code like this:

if err := doSomething(); err != nil {
    return err
}
if err := doSomethingElse(); err != nil {
    return err
}
if err := doAThirdThing(); err != nil {
    return err
}
return nil

Even if this is the only item in a function, I still think across a typical codebase it will force us to be unnecessarily different and will feel unfamiliar.

[andybons]

I can’t find any note of this in Effective Go or CodeReviewComments. Per the scope of golint, if it’s not in those documents recommended as a stylistic convention then we will accept a PR removing it.

[matryer]

Great - is it the lintIfError method? Does it check anything else or only that? Mat

[andybons]

Perhaps. I don’t know much about the code. I just own it now :)

Unit tests should help you narrow it down I think.

[matryer]

@andybons I think I got it, please have a look.

[andybons]

Whoops. Got the wrong issue number in the commit message. Closing since #388 was merged.

[abbot]

Hello. I would like to get some broader discussion about this check, because I believe it's removal is a regression.

I understand that in the specific example @ijc shown this lint might seem to be hindering, however that example is not a good example of Go error handling (explicitly called out in CodeReviewComments): as it doesn't actually handle the errors, it only passes them through, unmodified. I would argue that good code should actually handle the errors (e.g. explain what it was trying to do), and instead should look more like:

if err := doSomething(); err != nil {
    return fmt.Errorf("foobar: failed to do something: %v", err)
}

Note that this immediately removes any linter warnings. Another very common pattern which was caught by this check was

if err != nil {
  return err
}
return nil

which is just plain redundant.

I would like to have more discussion about this issue, and get this check back, probably with improvements/suggestions.

Also +@alandonovan who reviewed the initial implementation of this

[andybons]

@abbot could you point to where in CodeReviewComments the style this check enforced is mentioned?

[abbot]

@andybons, style guide doesn't ask for this, as is doesn't say that you should use if foo instead of if foo == true. The purpose of the style guide is not to enumerate all examples of bad style, but to give examples of good style. Not being mentioned in style guide doesn't mean it's good or bad style.

Style guide asks to handle errors though (with fall back solution to return errors), and this lint is not triggered if errors are actually handled, not simply returned.

[andybons]

@abbot I agree that the style you mention above is better, however the check for redundancy was removed because it doesn't fall in the scope of the tool. In all examples, the errors are handled in the way that neither CodeReviewComments nor Effective Go forbids.

I am open to suggestions.

[ijc]

This construct is in no way comparable to something like if foo ==true because returning an error without wrapping is a reasonable and correct way of handling an error under some (but, I agree, certainly not all) circumstances.

Wrapping an err when you know the function you have called always returns a reasonable error (which it is entirely possible you do in lots of cases) is just noise and adding such noise just to satisfy the kind of check we are talking about here is a cure worse than the disease IMHO.

I also think you are reading far more into that handle errors section than is actually there. It says:

See https://golang.org/doc/effective_go.html#errors. Do not discard errors using _ variables. If a function returns an error, check it to make sure the function succeeded. Handle the error, return it, or, in truly exceptional situations, panic.

I think claiming that this says that "return errors" is "as a fallback" is way overstating it. In fact I would rather say it explicitly condones just returning the error as a valid possible way of "handling" an error.

The real point of that whole section is against throwing errors away, which I think most people would agree with. Returning an error is not throwing it away.