Discussion on named return values

[rootulp]

Golang has named return values so a function can be written with unnamed return values:

func parseMsgShares(shares [][]byte) ([]Message, error) {

or named return values

func parseMsgShares(shares [][]byte) (messages []Message, err error) {

celestia-core currently uses both forms which leads to code style nits in PR comments. If we reach a decision on which form is preferred, we can enforce it via linters and avoid this type of code style inconsistency.

⚠️ named return parameters enable bare returns which reduce readability so if we do enforce named return values, we should also enforce no bare returns.

Ref:

• https://golangci-lint.run/usage/linters/#varnamelen
• https://golangci-lint.run/usage/linters/#revive
• https://yourbasic.org/golang/named-return-values-parameters/

Original discussion: #820

[rootulp]

@adlerjohn points out two drawbacks to using named return values:

Bare returns are one big issue. The other is un-obvious side effects. Assigning to a named return value is a side-effect to an otherwise pure function, and there's nothing in the syntax to indicate this explicitly to the reader. Named return values are basically global variables.

I don't understand the second drawback (re: global variables). To my knowledge, a named return value is declared inside the scope of the function so a function that uses named return values can still be pure.

[evan-forbes]

...which reduce readability so if we do enforce named return values, we should also enforce no bare returns.

I almost never use them for bare returns, but sometimes I like adding the names as documentation and saving me a line or two of variable declarations.

func() (has bool) {}
func(t []int) (s []int) {
    for _, i := range t {
        if i == 4 {
            s = append(s, i)
        }
    }
    return s
}

tbh though I realllllllyyyy don't care. if someone has a strong preference we can do that as long as we do not change tendermint code

[evan-forbes]

sorry, hit control + enter at the wrong time and it closed the issue...

[rootulp]

as long as we do not change tendermint code

That's a really good point. Any decision we reach here would only be productive if enforced through linters. I think minimizing the diff between celestia-core and tendermint/tendermint is a more important goal than codebase consistency with regard to this minor code style rule.

My preference would be to remain consistent with whichever linters are enforced by https://github.com/tendermint/tendermint/blob/main/.golangci.yml