lint: enable govet shadow linter and resolve linter warnings

[cce]

Summary

This enables the govet shadow analysis pass, implemented in https://github.com/golang/tools/blob/master/go/analysis/passes/shadow/shadow.go and enabled via golangci-lint.

The shadow linter prevents some cases of shadowed variables being used, which especially impacts the re-use of common variable names like err and ok. In this PR I attempted to resolve these issues mostly be just renaming names like err to more specific and unused names like err2 or osErr.

In a few other cases I updated

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

to

err = X()
if err != nil {
    return err
}

if the check-and-return came right away. I also removed a couple of named error return variables or unnecessary declarations of names earlier in a function that were shadowed later.

The shadow linter strict: false setting is used which comes with the following explanation:

A span stores the minimum range of byte positions in the file in which a given variable (types.Object) is mentioned. It is lexically defined: it spans from the beginning of its first mention to the end of its last mention. A variable is considered shadowed (if strict is off) only if the shadowing variable is declared within the span of the shadowed variable. In other words, if a variable is shadowed but not used after the shadowed variable is declared, it is inconsequential and not worth complaining about. This simple check dramatically reduces the nuisance rate for the shadowing
check, at least until something cleverer comes along.

One wrinkle: A "naked return" is a silent use of a variable that the Span will not capture, but the compilers catch naked returns of shadowed variables so we don't need to.

Cases this gets wrong (TODO):

• If a for loop's continuation statement mentions a variable redeclared in the block, we should complain about it but don't.
• A variable declared inside a function literal can falsely be identified as shadowing a variable in the outer function.

Test Plan

Existing tests should pass — in most cases I did not change the logic at all, but I did make some bigger changes to resolve some of the warnings.

[codecov]

Codecov Report

Merging #5261 (08f7542) into master (56b7e82) will decrease coverage by 0.02%.
The diff coverage is 29.35%.

@@            Coverage Diff             @@
##           master    #5261      +/-   ##
==========================================
- Coverage   55.43%   55.41%   -0.02%     
==========================================
  Files         452      452              
  Lines       63872    63875       +3     
==========================================
- Hits        35405    35394      -11     
- Misses      26053    26073      +20     
+ Partials     2414     2408       -6     

Impacted Files	Coverage Δ
agreement/demux.go	91.20% <0.00%> (ø)
cmd/algocfg/profileCommand.go	29.68% <0.00%> (ø)
cmd/goal/account.go	13.68% <0.00%> (ø)
cmd/goal/application.go	18.19% <0.00%> (ø)
cmd/goal/asset.go	15.80% <0.00%> (ø)
cmd/goal/interact.go	3.42% <0.00%> (ø)
cmd/goal/network.go	14.81% <0.00%> (ø)
cmd/goal/tealsign.go	11.70% <0.00%> (ø)
daemon/algod/api/server/v2/handlers.go	0.82% <0.00%> (ø)
daemon/algod/server.go	4.30% <0.00%> (ø)
... and 25 more

... and 13 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[gmalouf]

Talked this through with @cce , basically we are mostly okay with the inconsistent handling in some cases (see cmd/goal/asset.go for an example) as long as the linter (which in ways is stricter than needed) is happy with both of them.

For example, we are fine re-using an err variable directly, as long as it is not shadowing (and there is a quick nil check) as in https://github.com/algorand/go-algorand/pull/5261/files#diff-43408fd8ee96ef81b41df4152675473a9d2b76358da39757022f1e2d6ee00872R764 whereas in other situation err2, err3, etc is more appropriate for readability.

[gmalouf]

Falls in the bucket of "this function is too long for naming return variables"

[gmalouf]

I wish we could enforce a rule around not naming return variables when functions are too long strictly - but doubt linter picks it up.

[jannotti]

I don't thinking naming them is the problem, naked returns are the problem. You might name return values if you are trying to distinguish two returned ints - you want to explain which one is which, in the signature.

But naked returns are confusing. It's so easy to not realize what you're returning, especially in the presence of shadowing. So I would much rather ban naked returns in long functions. I very often return things explicitly, even if a naked return would do the same thing. In a long function, it's much easier to understand without a moment's hesitation.

[gmalouf]

Agree, naked returns are the primary culprit. The return value naming is useful for readability, but if we can't stop naked returns in those cases seems not worth it.

[cce]

We can ban naked returns for functions longer than a certain number of lines. What should we pick? 50 lines?

[tzaffi]

It depends on how many failures currently but ideally even less (enough for very simple if / else).

[gmalouf]

I was thinking 15-20 lines max - not sure it's a blocker for this PR but a quick follow-up.

[cce]

Will do in future PR to add the nakedret linter maybe with perhaps 50 lines to start

[gmalouf]

These are safe without naming the error variable in function signature, frankly more readable too.

[gmalouf]

Why didn't we rename/remove the err error declaration in the signature of this function (its pretty long)?

[cce]

Taking out named return variables seemed like a whole can of worms that could make this PR get pretty big — I was going for the minimal set to make the linter happy

[tzaffi]

I agree with the vast majority of the changes. I left a couple of questions/suggestions in places that I disagreed or could spot an alternative.

[tzaffi]

LGTM

[jannotti]

I'll try to look at more later, but this looks mergable to me.