Add option to allow cuddling of var declaration and an expression that uses that variable's reference

[grongor]

Sorry, I'm not sure how to describe it better, so here goes an example:

var config Configuration
conf.Load(&config)

Another that comes to my mind:

var config Configuration
err = json.Unmarshal(body, &config)

When there is an empty line in between these two statements seem really disconnected, while I consider them more or less a single statement, which isn't possible due Go "limitations".

[bombsimon]

I think I get your concern perfectly fine! 😄

This is interesting, I guess I'm not used ot see var definitions like this for single variables in the middle of a block. Although I don't know what conf.Load does both of these seem to manipulate the default empty struct Configuration. So I'm more used to see this:

config := Configuration{}
err := json.Unmarshal(body, &config)

As you might have seen in the other issue, the rule is described here. It's configurable to allow cuddling of var declarations like this:

var foo = 1
var bar = 2

Maybe an alternative would be to extend the usage of the allow-cuddle-declarations to just imply "treat the declaration like an assignment". If so, the same rules would apply for any var declaration as for an := assignment (and thus allow both your examples in this issue).

The only thing I think would be hard would be to handle these cases, but maybe that's not needed?

// Actually only one declaration
var (
    config Configuration
)
err := json.Unmarshal(body, &config)

// "Separated" declarations
var (
    err error

    config Configuration
)
err = json.Unmarshal(body, &config)

// Although this should be seen as
// err := errors.New()
// config := Configuration{}
// json.Unmarshal(...)
// Which isn't allowed either
var (
    err    error
    config Configuration
)
err = json.Unmarshal(body, &config)

[grongor]

Hah, I never tried this with := syntax, somehow var seems more right to me there :D And yeah, with := the linter doesn't complain :)

Anyway, it's more complicated...with this code:

config := Configuration{}
err := json.Unmarshal(body, &config)
if err != nil {
...

we get only one cuddle assignment allowed before if statement, which makes sense from the point of the if, but doesn't from the point of var declaration/unmarshal :D

On the other hand, this is valid:

var (
	config Configuration
	err    = json.Unmarshal(body, &config)
)
if err != nil {
...

And I get why it's valid, but...it doesn't seem right that one is and the other is not :D Oh man I don't envy you managing this tool ... 😂

But yeah, as you wrote

Maybe an alternative would be to extend the usage of the allow-cuddle-declarations to just imply "treat the declaration like an assignment".

I think that makes sense, I personally don't differentiate between these two as they are basically different syntax for the same thing. I would be cautious with throwing in a plain assignment = (I see three types of operations here: declare, declare+assign [either var or :=], assign), but I guess then the complexity would be just out of the roof.

[bombsimon]

The reason for := working is because it's one of the main mantras of grouping usages. It's meant to be used like this:

someResult := getResult()
if someResult > 0 {
    // ...
}

someList := []int{4, 8, 15, 16, 23, 42}
for _, v := range someList {
    // ...
}

Anyway, it's more complicated...with this code:

config := Configuration{}
err := json.Unmarshal(body, &config)
if err != nil {
...

This to me is one of the most obvious places to use the ; and early returns:

config := Configuration{}
if err := json.Unmarshal(body, &config); err != nil {
    return nil, err
}

return &config, nil

On the other hand, this is valid:

var (
	config Configuration
	err    = json.Unmarshal(body, &config)
)
if err != nil {
...

If that's valid that is what I would call a feature (a.k.a. a bug I won't admit is there by accident!). I guess it's just a coincidence that the last assignment is err and that err is used in the if statement. This would not be valid if the last assignment wasn't used in the condition.

Regarding managing this, I don't plan to support all end every combination of every feature. I want this to help me and others to write code that looks the same while making it readable by grouping code by usage. I want to find a way where I can be strict with some basic ideas (such as never cuddle blocks, just don't...) but avoid false positives. I don't mind if something I wouldn't write is allowed, however I don't want to give warnings on things that doesn't work 100% consistent.

Since bundling this in golangci-lint I've made a few things configurable to avoid having to write patterns to ignore errors but I'm not sure how far I can take this. If this was a standalone linter I wouldn't mind ending up with something like perltidy but as long as I want it bundled with golangci-lint I think we start to reach the maximum number of configurables while still following the linter mantra.

I think that makes sense, I personally don't differentiate between these two as they are basically different syntax for the same thing.

Well, yes and no. As long as = is used it's an assignment and as long as it's not it's a declaration. To me it makes more sense to see var x = 1 and x := 1 as the same rather than var x int and var x = 1 as the same even though the both use the var keyword.

But to sum it up, a lot of times the var keyword is used when assigning and it looks waay better to just declare an error with var err error than err := error(nil).

I don't see any major issues having the current allow-cuddle-declarations making all var and assignments treated the same. Although to avoid confusion it could be renamed to something like treat-var-as-assigned or something descriptive. When toggled on the linter should make any difference between

var x = 1
var y = 1

And

var x = 1
if x > 0 {
    // ...
}

And essentially just making it equivalent with the := like it is for you!

[grongor]

Except for the name treat-var-as-assigned (I find it confusing :D, actually out of context I wouldn't probably guess what it does) I totally agree with everything :)

[grongor]

I think allow-cuddle-declarations is a good name, and the rule simply fails to recognize that := and var are both declarations...

[bombsimon]

Yeah we can work on the name, I also see I had a type, was trying to write treat-var-as-assign but I guess the name isn't that important if there are proper docs.

You are right about them all being declarations, they're just no all explicit. They are however not all assignments. At least this is what I've been taught, but I guess it's possible that I'm wrong here.

Code	Description
var x Type	Declare x to be of type Type
var x Type = val	Declare x to be of type Type and assign the value val
x := val	Assign x the value of val (no explicit declaration)

Although I found this interesting so I did some testing and compiled these three programs with go tool compile -S main.go.

package pkg

func x() int {
	x := 0

	return x
}

package pkg

func x() int {
	var x int = 0

	return x
}

package pkg

func x() int {
	var x int

	return x
}

And yeah, it's obvious that since the declaration only version (the last one) isn't a pointer and thus cannot be nil, this will all result in the same behaviour:

PCDATA  $0, $0
PCDATA  $1, $0
MOVQ    $0, "".~r0+8(SP)
RET

So I guess this shows that it would be even more reasonable to treat var and := the same on a technical level, although keep in mind that most of what this linter does is for the eye only and not for computers. :)

[grongor]

I think you are kinda wrong about := being Assign x the value of val (no explicit declaration). From what I know, := is simple syntax sugar which lets you omit the type, but other than that is no different from Declare x to be of type Type and assign the value val (the type is inferred). This is why I find it strange that it's treated differently and why I would expect just the assignment = to be treated differently (as there is no declaration, the variable must be declared beforehand).

And it looks like you confirmed that with the compiler :) And yeah, of course, linter is for us, humans :) But in that case, I think it's pretty agreeable that both syntaxes are the same, on both the compiler level and on the eyes :)

[bombsimon]

Hehe, I think we are saying the same thing with different words and I agree with you! All of them are partly declaration but not all of them are assignment. You can omit the type with var too, just like your example var x = 12345 so the sugar to leave out the type isn't specific for :=.

They are two separate types in the AST from Go's own standard library. See DeclStmt which is the node after the var keyword and AssignStmt which is the node when using the :=.

You are right about the latter also being a declaration (which is required to assign/initiate the the value). From the documentation:

An AssignStmt node represents an assignment or a short variable declaration.
A DeclStmt node represents a declaration in a statement list.

So an assign statement is partly a declaration but a declaration does not always include an assignment. This is the reason for the different behaviour, combined with the fact that the AST which the linter is based on returns different types for them. And also why I avoid to use them interchangeably.

TL;DR: We agree with each other on what the syntaxes do and if they look the same to the eye or not boils down to opinions. We also agree on that it would be a good feature for the linter to be able to treat var the same way as :=. It will however not be the default case since it's not a setting me personally would enable.

Oh, and PRs are welcome since I don't know when I will find the time to implement this!

[grongor]

I never looked at Go AST before, but from what you wrote it seems they just named them wrong :D But yeah, we agree to agree 😂 I will definitely give it a look, but since I have no experience with Go linters/Go AST, I can't promise anything :) Although I wrote huge number of PHP sniff rules for phpcs, so I guess that should be pretty similar ... we'll see :)

[thaney071]

Not sure if this applies here but another similar case is when you need to declare an error variable because the return from a function is going to an exising variable, but error has not been defined

func someFunc(in *someThing) error {
    var err error
    in, err = someOtherFunc()
    if err != nil {
        return err
     }
}

The assignment cannot use := in this case because in is already defined by the function parameters. So err needs to be defined before being assigned by the function call.

That is just a very basic example. The issue with that is either the var err error and the call to the function can be cuddled or the function call and the error handling, but not both.

[bombsimon]

Merged in golangci/golangci-lint#1750, will be available in next release.

[mitar]

So how can one allow the code example from above:

var config Configuration
err := json.Unmarshal(body, &config)
if err != nil {
...

I still get "only one cuddle assignment allowed before if statement" error.

[bombsimon]

You can't, but that's not a part of how cuddling and usage of var works, that's the rule of only cuddle one assignment/declaration with if statements.

Alternative allowed ways could be f.ex.

var config Configuration

err := json.Unmarshal(body, &config)
if err != nil {
...

or

var config Configuration
if err := json.Unmarshal(body, &config); err != nil {
...