New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

golint asks you to drop else block containing return, even when local vars are being used #17

Closed
deepakjois opened this Issue Jul 8, 2013 · 10 comments

Comments

Projects
None yet
9 participants
@deepakjois

deepakjois commented Jul 8, 2013

package main
import (
    "fmt"
)

func main() {
    res := get(3)
    fmt.Println(res)
}

func get(i int) int {
    if f := i * 2; f == 6 {
        return f
    } else {
        return f * 3
    }
}

Running golint displays:

main.go:15:9: if block ends with a return statement, so drop this else and outdent its block
@dsymonds

This comment has been minimized.

Member

dsymonds commented Jul 8, 2013

I think it's fine. The usual Go style for that is

func get(i int) int {
    f := i * 2
    if f == 6 {
        return f
    }
    return f * 3
}

@dsymonds dsymonds closed this Jul 8, 2013

@attilaolah

This comment has been minimized.

attilaolah commented Jul 19, 2013

@dsymonds you are referring to the third code example in this section? Looks like I misinterpreted it then. I was just about to open a duplicate issue.

Thanks for pointing that out.

@dsymonds

This comment has been minimized.

Member

dsymonds commented Jul 20, 2013

Yes, it's that one.

@jgroeneveld

This comment has been minimized.

jgroeneveld commented Jun 4, 2015

This is actually a nice pattern to retrieve a value, evaluate the error, return on error or assign the value if there is no error while protecting the namespace.

I see why this is rule exists in the usual cases but when using a local var I was surprised that golint is reporting it.

[...] some code
    if loc, err := store.Location(); err != nil {
        return nil, false
    } else {
        doc.Location = loc.ToArray()
    }

// at this point there is no var loc anymore, its only used in the else case
[...] some code
@jriquelme

This comment has been minimized.

jriquelme commented Jun 12, 2015

Same opinion as @jgroeneveld here, I have many complains of golink for things like:

if nid, err := att(node, "nid"); err != nil {
    return nil, err
} else {
    deliveryMsg.Nid = parseNid(nid)
}

I prefer this form for this particular case (doesn't pollute the scope with "temporary" variables).

@dsymonds

This comment has been minimized.

Member

dsymonds commented Jun 12, 2015

The point of a common style is that it overrides personal preference.

@yurishkuro

This comment has been minimized.

yurishkuro commented Dec 26, 2015

@dsymonds However, golint readme also says:

If you find an established style that is frequently violated, and which you think golint could statically check, file an issue.

@gordallott

This comment has been minimized.

gordallott commented Sep 6, 2016

for anyone coming to this via google search and finding this dead end, i forked and removed this poor decision. just go get that version instead and enjoy less scope leaking

https://github.com/gordallott/lint

@delsvr

This comment has been minimized.

delsvr commented Nov 13, 2017

The point of a common style is that it overrides personal preference.

This isn't just a question of style, though. These examples are functionally different. The snippets by @jgroeneveld and @jriquelme limit the scope of variables to the if block. The linter enforces polluting the parent scope.

At any rate, there is not even any indication that your example is the "usual Go style". Just see Go's own tutorial on flow control: https://tour.golang.org/flowcontrol/7.

func pow(x, n, lim float64) float64 {
	if v := math.Pow(x, n); v < lim {
		return v
	} else {
		fmt.Printf("%g >= %g\n", v, lim)
	}
	// can't use v here, though
	return lim
}

Please reconsider.

@l0k1verloren

This comment has been minimized.

l0k1verloren commented Aug 1, 2018

I have discovered that variables declared inside the first part of an if keyword's block, the if <statement>; <boolean> { ... }that these variables fall out of scope after the if block, but remain in scope inside subsequent else blocks attached to the main if block. For these cases, you have two options, to pre-declare the variable in the parent scope or you have to perform all the operations on the variables within following else blocks.

I am in two minds about it, I kinda came to it with the expectation that these variables were bound to the parent scope but they are not. Conforming to what golint says requires considerably more predeclarations of variables, so I just ignore the green squiggle indicating golint's warning. I don't know whether it would be better if the scoping of declarations inside the if statement parameters were bound to the parent, or if it is better to leave it as is and perhaps have golint not give these warnings if the declared variables are used within the direct or deeper descendant, but if it were changed it would break a lot of the code I am working on at the moment.

It's a very trivial issue and taking the conservative, idiomatic way of looking at it, that the grammar should not be changed as it doesn't provide significant expressivity, and could potentially break a lot of code (in a relatively trivial way), it would only reduce some of the wordiness of these constructions, and for me having to put else blocks isn't a terribly onerous burden compared to not having to predeclare.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment