Support to cuddle assignments for a whole block

[bombsimon]

Right now you have to use the variable, type or field in the first statement in a block if you want to use it like this:

variable := "assigned"
[token] {
    // Must use 'variable' here
}

I think this is reasonable for most of the time but I kind of like to be able to use this pattern, especially if it's spawning a go routine like this:

done := make(chan struct{})
go func() {
    fmt.Println("Let's spawn this go routine")
    <-somethingBlocking

    close(done)
}()

// Wait for done
<-done

This might be useful in more scenarios like ranges, if-statements etcetera and therefore could possible be configured. Although as of now the only place I really felt that this was needed was a very few go routines.

Since this linter isn't really embrarcing any kind of official best practices like pythons black I think it's ok to have the linter heavily configurable like e.g. perltidy.

[hollowaykeanho]

Don't quite get you.. can you derive an example based on the former, using the latter's contents?

[bombsimon]

The idea is to be able to use the cuddled variable wherever you want in the block. Instead of just this:

counter := 0
if somethingTrue {
    counter++
}

I also think it should be possible to configure it like this:

counter := 0
if somethingTrue {
    checker := getAChecker()
    if !checker {
        return
    }

    counter++ // Cuddled variable used in block, but not as first statement
}

[hollowaykeanho]

counter := 0
if somethingTrue {
   counter++
}

+1. The rule between the assignment and definition cuddling (related) is very strict at the moment.

counter := 0
if somethingTrue {
checker := getAChecker()
if !checker {
return
}

counter++ // Cuddled variable used in block, but not as first statement
}

I don't see any difference at the moment but this one is visually and cognitively acceptable.

[bombsimon]

I'm not really sure what the best way to attack this issue would be. Since this should be valid for every block I need to store all variables from all blocks in parseBlockStatements and return them to the caller of the function. However, right now I'm not parsing every line in a block but only if it's cuddled with something and not the first block.

If I'm going to store all assigned/used variables from all blocks I think I want to re-think the current layout first.

[mitar]

Yes, cuddling of variable assignments with a followup block should be possible if they are then used somewhere (anywhere) inside the block. To me this is the main obstacle with wsl and it makes code less readable by requiring a newline in there.

[treuherz]

If it isn't likely this improvement will be made soon, would you be open to PR making it possible to toggle the assignments-cuddling-loops lint off?

[bombsimon]

If it isn't likely this improvement will be made soon, would you be open to PR making it possible to toggle the assignments-cuddling-loops lint off?

Yeah I don't see a problem adding an opt-in to disable this, we already added (perhaps with too little consideration) other exclude rules.

If we do this however I think it should be consistent and disabel the check for

• if (code)
• for (code)
• range (code)
• switch (code)
• type switch (code)
• defer (code, special FuncLit code)

While looking at this I saw that all of these allow cuddling with variable that's used first in block except switch for some reason.

[treuherz]

Is there a distinction here between variables used in the opening statement or within the block itself? Does wsl currently treat these differently?

[bombsimon]

Is there a distinction here between variables used in the opening statement or within the block itself? Does wsl currently treat these differently?

Yes it is, that's what mentioned in the ticket. F.ex. these are allowed:

// Used in if/for/range
x := "x"
if x != "" {
    // ...
}

// Used in first block statement
x := "x"
if y {
    x = z
}

// Used as argument in first statement
x := "x"
if y {
    pass(x)
}

When parsing block statements (statements that has a body) we start by grabbing the first statement at

wsl/wsl.go

Line 253 in 4740984

firstBodyStatement := p.firstBodyStatement(i, statements)

and then using that to create a list of usages at

wsl/wsl.go

Lines 302 to 308 in 4740984

	// We could potentially have a block which require us to check the first
	// argument before ruling out an allowed cuddle.
	var calledOrAssignedFirstInBlock []string
	if firstBodyStatement != nil {
	calledOrAssignedFirstInBlock = append(p.findLHS(firstBodyStatement), p.findRHS(firstBodyStatement)...)
	}

.

I think for the long term solution we probably want to return all variables used (not shadowed) in parseBlockBody and use those at all the places we currently use calledOrAssignedFirstInBlock. Thinking of this now, it might be possible to solve without too much work, I'll see if I can find some time to look into this.

[treuherz]

I'm sorry, I should have been clearer with my question. What I was trying to ask was whether you want whatever toggle(s) I add to cover all three of the cases you've described, or separately toggle case 1 (used in opening statement) and cases 2 and 3 (used in block).

[bombsimon]

Ah, I see! I think it would make sense to disable it completely, so meaning either you have to use it in the block or first in statement like now, or the lint is off and you can use it wherever.

[arvenil]

It's very annoying that I have to add new line, just because I've added a condition.

var out []string
for i := range in {
    out = append(out, in[i])
}

var out []string // NOT OK, new line is required

for i := range in {
  if in[i] == "whatever" {
    out = append(out, in[i])
  }
}

[bombsimon]

Another one of those super old issues finally getting resolved, this time thanks to @jacobdrury! Thank you so much!! 🎉 😍

If you want this now, you can install wsl from master: go install github.com/bombsimon/wsl/v4/cmd/wsl@master.

However I'll release a new version and once dependabot picks it up add the new option to golangci-lint. Keeping this issue open until that's done.