bombsimon / wsl Goto Github PK

I got a linter hit:

thelper.go:184:2: declarations should never be cuddled (wsl)
        var log string
        ^

for code block:

func (h *THelper) Conclude() {                                                 
        var err string                                                         
        var log string                                                         
                                                                               
        if h.Controller == nil {                                               
                goto endConclusion                                             
        }                                                                      
                                                                               
        // process error messages                                              
        err = strings.Join(h.failed, "\n")                                     
                                                                               
        // process log messages                                                
        log = strings.Join(h.log, "\n")                                        
                                                                               
endConclusion:                                                                 
        switch {                                                               
        case h.QuietMode && err == "":                                         
        case h.QuietMode && err != "":                                         
                fallthrough                                                    
        case log != "" && err != "":                                           
                h.Controller.Errorf("\n%v\n%v\n\n\n", err, log)                
        case log != "":                                                        
                h.Controller.Logf("\n%v\n\n\n", log)                           
        case err != "":                                                        
                h.Controller.Errorf("\n%v\n\n\n", err)                         
        }                                                                      
        h.Flush(AllType)                                                       
}

Looks fine to me though, as in, group all var declaration. What was the initial preference?

In this PR https://github.com/bombsimon/wsl/pull/37/files#diff-676fb336aa3ff093ebaa37c4293f1cd1R417 the issue was addressed just for Close method, but in my opinion it applies to any method that is used in a similar way.

Here is an example usage of https://github.com/uber-go/automaxprocs package:

undoMaxProcs, err := maxprocs.Set()
if err != nil {
    return fmt.Errorf("failed to set GOMAXPROCS, err: %w", err)
}
defer undoMaxProcs()

// The rest of the code goes here.

I think this code aligns nicely with Go-best-practices, but wsl still complains about it:

main.go:72:2: only one cuddle assignment allowed before defer statement (wsl)
	defer undoMaxProcs()
	^

and wants to see it formatted this way:

undoMaxProcs, err := maxprocs.Set()
if err != nil {
    return fmt.Errorf("failed to set GOMAXPROCS, err: %w", err)
}

defer undoMaxProcs()

// The rest of the code goes here.

which is less ideomatic in my opinion.

Blocks cannot start or end with a whitespace. I think this is a very reasonable rule except for maybe the possibility to end a case block with a whitespace. I'm open to the idea to at least have this configurable and maybe even as the default since this can actually improve readability and mark where the next case starts. For this to be reasonable it's important to ensure this does not apply to the last case since that would actually mena the ending of the switch block and not the case block.

[token] {
    // Can't be a whitespace here
    fmt.Println("...")
    // Can't be a whitespace here
}

switch {
// Can't be a whitespace here
case 1:
    // Can't be a whitespace here
    fmt.Println("...")
    // Can't be a whitespace here - should it be possible?
case 2:
    // Can't be a whitespace here
    fmt.Println("...")
    // Can't be a whitespace here - should it be possible?
case 3:
    // Can't be a whitespace here
    fmt.Println("...")
// Can't be a whitespace here
}

... I think. I am mighty confused by go modules.

In golangci-lint I am getting the following problem with their v1.21.0 tag:

# github.com/golangci/golangci-lint/pkg/golinters
/go/pkg/mod/github.com/golangci/[email protected]/pkg/golinters/wsl.go:43:6: unknown field 'AllowCaseTrailingWhitespace' in struct literal of type wsl.Configuration

This build worked before on the same commit of my code. I noticed my builds are using different versions of WSL even though they grab the same version of golintci-lint.. The build had worked with wsl v1.2.5. However, now by build is get v1.2.7 of wsl.

I suspect that go modules believes this is fine, because v1.2.7 should be backwards compatible to v1.2.5 if you are not using any new fields according to semantic versioning.

However, when I git diff v1.2.6 v1.2.7 wsl.go I see that the exported configuration property AllowCaseTrailingWhitespace was removed/renamed to CaseForceTrailingWhitespaceLimit, which is a breaking a change:

diff --git a/wsl.go b/wsl.go
index 8a1f45b..ea8d4eb 100644
--- a/wsl.go
+++ b/wsl.go
@@ -50,21 +50,9 @@ type Configuration struct {
        //  }
        AllowMultiLineAssignCuddle bool
 ...
-       AllowCaseTrailingWhitespace bool
+       // If the number of lines in a case block is equal to or lager than this
+       // number, the case *must* end white a newline.
+       CaseForceTrailingWhitespaceLimit int

So I believe that for Go modules, that change would have need to have been v2.x.x and using a new a package folder with v2 at the end of the folder path (or module declaration) as per https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher

Should support the following formats:

• ./... - This directory and everything recursive
• *.go - All go files, recursive if --recursive is specified.
• ???

It is good practice to defer a close statement for a HTTP response body after the error check as discussed in https://stackoverflow.com/questions/33238518/what-could-happen-if-i-dont-close-response-body-in-golang#33238755

In my opinion, this defer statement should logically be part of the block where the body originates so I think we should allow the following:

resp, err := client.Do(req)
if err != nil {
    return err
}
defer resp.Body.Close()

instead of enforcing the following:

resp, err := client.Do(req)
if err != nil {
    return err
}

defer resp.Body.Close()

Must be more generous about when to enter a block and parse it.

Gives no error but should

var foo string

func() {
    var err error
    foo = "1"
}

Gives error if assigned to variable

var foo string

x := func() {
    var err error
    foo = "1"
}

assignments should only be cuddled with other assignments

Also this gives no error, assignign or not:

var err error
foo, err = func() { return "", nil }

comments like this

// here should a white space
/* here should a white space
/***** here should a white space *****/

could you add a rule? thank you.

Hi!

I see that there exists a allow-trailing-comment flag to allow blocks to end with a comment. We would like to have a similar flag that would allow blocks to start with comments.

Here is our use case: we are working on an API and we use https://github.com/go-swagger/go-swagger to generate swagger documentation for our API. The syntax that all routes are defined for the doc is a comment starting with swagger: route and we usually place the handler description right at the start of our handler functions. This does trigger an error (we are using golangci-lint) and we would like to allow such comments instead of applying nolint: wsl to the whole handler function.

Just a suggestion, I can also see it as intentional, since it would allow too complicated blocks maybe. I stumble a lot into that in my code, so I have some examples and can tackle down if and how we can improve this. I'm also very critic to myself that this suggestion is wrong and I have to overthink my code :)

what this rule wants:

	r := map[string]string{}
	for k, v := range m {
		r[k] = v
	}

I often have a range with a little if if !thisorthat()

	r := map[string]string{}
	for k, v := range m {
		if !c.IsReserved(k) {
			r[k] = v
		}
	}

Or a switch-case:

	c.Environment = make(map[string]string)
	for _, env := range req.GetConfig().GetEnvs() {
		switch env.GetKey() {
		case "user-data":
			c.CloudInitUserData = env.GetValue()
		default:
			c.Environment[env.GetKey()] = env.GetValue()
		}
	}

See if Go modules is more suitable in favor of dep.

Can we produce a "hit" list for explaining each hits and how to amend them? E.g. https://go-critic.github.io/overview

I do not mind contribute back. I see a lot of good potential for this linter. =)

EDIT: added mind

I see this less often than my other issue and am also fine if this stays the same. But thought it is worth giving in my thoughts. The following issue has one popular case throughout my code in common, a function call expected to return a list. I'll try to elaborate.

If a function is expected to return a single element, but there is no such element, it usually returns an error indicating it is not found:

e, err := findElement()
if err != nil { // contains a `xyz not found` error
  return err
}

If a function is expected to return a list, but there are no elements matching, it doesn't return an error, since an empty list is expected. Only the caller knows if it wants tho have an empty result or not.

e, err := findElements()
if err != nil { // any lower level error, eg. timeout etc.
  return err
}
if len(e) == 0 { // cuddling allowance here for discussion
  return fmt.Errorf("not found")
}

But you can also have a function expecting to return nil even if no error happens:

e, err := findElement()
if err != nil { // any lower level error, eg. timeout etc
  return err
}
if e == nil { // cuddling allowance here for discussion
  return fmt.Errorf("...")
}

One could think, "yeah allow one cuddling once per return value, if the return value is used". If we think further, what do we do if we have many return values... that's gonna look awful probably.

I also have a small case - which looks like happening only once - is a needed type conversion after marshalling

yml, err := yaml.Marshal(s.NetworkConfig) // we can't convert types directly on multiple returns
if err != nil {
  return err
}
config[cfgNetwork] = string(yml) // config is map[string]string so it forced me to convert the type

And if we go further:

obj, err := someCall()
if err != nil {
  return err
}
if obj != nil {
  some.Foo = obj.r // to me this could be seen as a bad example, because I wouldn't allow cuddling for a usage of `obj` properties. (if you start to access properties, probably more assignments are here clogging the block)
  some.Foo = obj // instead this could be allowed
  return obj // or we could define strictly only allow if we use returns
}

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".

Hi, I have the following issue while using the wsl tool

When two "if" blocks are together, I get: "if statements should only be cuddled with assignments"

func validateErr(err error) {
	if err == nil {
         ...
	}
	if errors.Is(err, ErrSomething) {
         ...
	}
...
}

But if I insert a space between them, i get : "if statements that check an error must be cuddled with the statement that assigned the error"

func validateErr(err error) {
	if err == nil {
         ...
	}

	if errors.Is(err, ErrSomething) {
         ...
	}
...
}

So, getting block should not end with a whitespace (or comment)

logging/logcore.go:59:21: block should not end with a whitespace (or comment) (wsl)
                zapcore.FatalLevel:

func (c *svcLoggingCore) Write(ent zapcore.Entry, fields []zapcore.Field) error {
	buf, err := c.enc.EncodeEntry(ent, fields)
	if err != nil {
		return err
	}

	switch ent.Level {
	case zapcore.InfoLevel:
		return c.logger.Info(buf.String())
	case zapcore.WarnLevel:
		return c.logger.Warning(buf.String())
	case zapcore.ErrorLevel,
		zapcore.DPanicLevel,
		zapcore.PanicLevel,
		zapcore.FatalLevel:
		return c.logger.Error(buf.String())
	}

	return nil
}

near as I can see gofmt made that code that format so, should be ok? or am I missing something

Sometimes assignments (function call results) may be split into multiple lines in order to avoid too long lines. In case such statements are used, linter does not allow an if statement to follow. Since the if statement is closely related to the operation, it should still be allowed for the if to follow an assignment that is split into multiple lines.

Allowed:

err := functionCall(arg1, arg2)
if err != nil {
    return err
}

Not allowed:

err := functionCall(
    arg1, arg2,
)
if err != nil {
    return err
}

I'd love to be able to enforce that error checking is cuddled with the error assignment.

For example, right now this doesn't trigger the linter:

thing, err := db.GetThing()

if err != nil {
  return err
}

But I'd like it to :)

Thanks for the awesome linter!

Support for a flag which can specify ignore patterns in glob format, i.e. --ignore *_test.go or --ignore examples/

Example functions must end with comments, but wsl doesn't like that very much:

Here's a snippet that's failing for me in go-which:

func ExampleWhich() {
        path := Which("sh")
        fmt.Printf("Found sh at: %s", path)

        // Output: Found sh at: /bin/sh
}

$ wsl which_test.go 
which_test.go:124: block should not end with a whitespace (or comment)

I'm thinking wsl should probably ignore this particular check in Example tests.

Should be allowed to cuddle things like this:

idx := i
if i > 0 {
	idx = i - 1
}

Or

vals := map[int]structr{}{}
for i := range someRange {
    vals[i] = struct{}
}

Support to create a fixed version like go fmt to resolve the reported issues.

According to https://blog.golang.org/examples ExampleXxx tests should end with:

<empty line>
    // Output:
    // <expected output>
}

It would be nice to update the Block should not end with a whitespace (or comment) rule to honor this style.

For example:

	result, err := doSomeThing()

	if err != nil {
		return err
	}

In the above code, there is a blank line before the if statement, and I really want to remind developers to remove it. Is it possible to add a rule to give such a warning?

Thank you in advance.

Hi, I found a part of my code which wsl should have reported linting errors, but hasn't because I found out that the comments are also considered as "block separators". Consider this arbitary file:

package root

import "fmt"

func init() {
	var foo string
	// some comment
	var bar string
	// if something
	err := fmt.Errorf("foo")
	if err != nil {
		fmt.Println("do-something")
		// bla
		return
	}
	// something else
	var baz string
	// and
	return
}

I would've expected that different wsl rules would be reported, but aren't.

$ go run github.com/bombsimon/wsl/cmd/wsl -w test.go
<no output>
$ go list -json github.com/bombsimon/wsl/cmd/wsl | jq -r '.Module.Version'
v1.2.8

I would've expected that the comments are part of that block, and a newline before the comment fulfills the separation.

for comparison this would satisfy wsl if there were no comments

package root

import "fmt"

func init() {
	var foo string

	// some comment
	var bar string

	// if something
	err := fmt.Errorf("foo")
	if err != nil {
		fmt.Println("do-something")

		// bla
		return
	}

	// something else
	var baz string

	// and
	return
}

var err error
x := 12345

I consider these two basically the same, and it's annoying that I have to split them. It makes the code look weird at some places. It's also weird when you realize that this is a perfectly fine way to do the same thing, just more verbose:

var (
    err error
    x	= 12345
)

Simplified version of my code:

for _, x := range a {
	if x == y {
		goto SKIP // do not append
	}

	clean = append(clean, x)
SKIP:
}       // block should not end with a whitespace (or comment)

I think wsl considers the label SKIP as a whitespace (or comment)...

A lot of things are allowed in multiple ways but it would be nice to check consistency.

e1 := Some()
if e1! != nil {
    return
}

// Should not be allowed, we didn't use a blank line on line 2
e2 := Some()

if e2 != nil {
    return
}

Consider the following code which WSL currently demands under the rule of ranges should only be cuddled with assignments used in the iteration:

clauses := make([]string, len(b.clauses))
i := 0

for k, v := range b.clauses {
	clauses[i] = k + " " + strings.Join(v, ", ")
	i++
}

It seems to me that since the sole purpose of i is for the iteration, and it's being incremented inside the block, that the following would be better:

clauses := make([]string, len(b.clauses))

i := 0
for k, v := range b.clauses {
	clauses[i] = k + " " + strings.Join(v, ", ")
	i++
}

Do you think that variables initialized for the purposes of iterating over maps and incremented inside the for loop should be considered an "assignment used in the iteration"?

Hello!

For example, I have a huge project with thousands lines of source and now I'd like to automate application of the strict spacing rules provided by wsl, instead of hours of manual routine work.

Are there any tips for rewriting input files? If currently not, it could be a great idea because some other alike tools provide such functionality, e.g. gofumpt, goimports.

Regards

Will be triggered by the comment in

func NewABBProducer() Producer {
	/***
	 * http://datenblatt.stark-elektronik.de/Energiezaehler_B-Serie_Handbuch.pdf
	 */
	ops := Opcodes{
...

Not sure this is expected?

For example:

begin := getBegin()
end := getEnd()
if begin <= end {
    // do something
}

Now I have to add a blank line before if, otherwise I will get an error.

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.

It's not possible to break cases into multiple lines but it really should!

switch {
case true, // Does work for *first* statement.
	false:
	fmt.Println("x")
case true, // Doesn't work here
	false:
	fmt.Println("x")
case true || // Same with || operator
	false:
	fmt.Println("x")
case true, false: // Ok
	fmt.Println("x")
case true || false: // Ok
	fmt.Println("x")
}

Currently ForceCuddleErrCheckAndAssign enforces errors to be cuddled with the assigning statement no matter how many lines it spans over. While my idea was to enforce this:

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

This is now also enforced with ForceCuddleErrCheckAndAssign set to true:

err := db.
    Where(&SomeType{
        SomeColumn: true,
    }).
    Preload("AnotherTable").
    Find(&data).Error
if err != nil {
    panic(err)
}

Since the err assignment spans over so many lines I think it should be optional or configurable to leave a space between the assignment an if statement even when ForceCuddleErrCheckAndAssign is set to true.

Ranges isn't handled at all, should probably be handled similar to if-statements. That means:

Ok:

b := true

r := make([]int, 5)
for i := range r {
    // This is OK
}

b := true
r := make([]int, 5)

for i := range r {
    // This is OK
}

Not ok:

b := true
r := make([]int, 5)
for i := range r {
    // This is OK
}

r := make([]int, 5)
b := true
for i := range r {
    // This is OK
}

r := make([]int, 5)

b := true
for i := range r {
    // This is OK
}

Is there any documentation on how to configure specific rules? On the golangci-lint example file, you can see things like "allow-cuddle-declarations" and "allow-assign-and-call". But the rules.md file doesn't specify the configuration option name to use.

My original function:

func sortedKeys(m map[string]interface{}) sort.StringSlice {
	keys := make(sort.StringSlice, len(m))

    i := 0
	for k := range m { // ranges should only be cuddled with assignments used in the iteration
		keys[i] = k
		i++
	}

	sort.Sort(keys)

	return keys
}

I have found three possible fixes:

1. Isolate i := 0 but this is the index of the for loop

   func sortedKeys(m map[string]interface{}) sort.StringSlice {
   	keys := make(sort.StringSlice, len(m))
   
       i := 0
   
   	for k := range m {
   		keys[i] = k
   		i++
   	}
   
   	sort.Sort(keys)
   
   	return keys
   }

2. Inverse both assignments i := 0 and keys := .. but this is wired :-/

   func sortedKeys(m map[string]interface{}) sort.StringSlice {
       i := 0
   
   	keys := make(sort.StringSlice, len(m))
   	for k := range m {
   		keys[i] = k
   		i++
   	}
   
   	sort.Sort(keys)
   
   	return keys
   }

3. Putting both assignments in one line, but this is the least readable :-(

   func sortedKeys(m map[string]interface{}) sort.StringSlice {
   	i, keys := 0, make(sort.StringSlice, len(m))
   	for k := range m {
   		keys[i] = k
   		i++
   	}
   
   	sort.Sort(keys)
   
   	return keys
   }

I thing wsl should consider i++ as an assignment used in the iteration.
@bombsimon What about your opinion?

Simplified version of my code:

for result.Next() {
    // some
    // code

    var a string
    var b interface{} // declarations should never be cuddled (wsl)

    for key, val := range result.Values() {
        [...]

I have found two options to respect the rule declarations should never be cuddled in that case.

1. Add a blank line between the two var statements:

   for result.Next() {
       // some
       // code
   
       var a string
   
       var b interface{}
   
       for key, val := range result.Values() {
           [...]

2. Group together the var statements:

   for result.Next() {
       // some
       // code
   
       var (
           a string
           b interface{}
       )
   
       for key, val := range result.Values() {
           [...]

But, in my opinion, the original code seems readable.
I suggest that consecutive var declarations can be cuddled.
@bombsimon What is your feeling?

I think it should be allowed to cuddle multiple go statements if they're on a single line like this:

t1 := CreateT()
t2 := CreateT()
t2 := CreateT()

go t1.Func()
go t2.Func()
go t3.Func()

Today the alternatives would be:

t1 := CreateT()
t2 := CreateT()
t2 := CreateT()

go t1.Func()

go t2.Func()

go t3.Func()

Or

t1 := CreateT()
go t1.Func()

t2 := CreateT()
go t2.Func()

t2 := CreateT()
go t3.Func()

But the last one is not reasonable if we have to do mor things with tn, e.g.

t1 := CreateT()
t1.SetX(1)
t1.SetY(2)

go t1.Func()

In the current version this is ok:

a := 1
err := foo()

if err != nil {
    return err
}

I think it'd be better if the err checking is ALWAYS close to the err assignation like:

a := 1

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

both examples are accepted by wsl but my suggestion is to add a rule to reject the first example. what do you think?

golangci-lint v1.21

rec.WaitCount--		 	
if rec.WaitCount == 0 {		 	
    return &delivering{}, nil		 			
}

resulted in a linter warning.

However an if statement following an assignment should be allowed.

Is it by design, or in anyway intended that the following cuddled statements inside the anonymous function are not hit by wsl ?

package main

import "fmt"

func main() {
        fmt.Println(func () error {
                foo:="bar"
                fmt.Println("Just a statement")
                if foo == "bar" {
                        return fmt.Errorf("bar")
                }
                return nil
        }())
}

It is warned by the wsl if I declare a new slice before the range loop.
I expect it's not warned.

only one cuddle assignment allowed before range statement (wsl)

https://play.golang.org/p/VEpWPSRTNfj

func evenFilter(a*A) []int{
	slice := a.slice
	newSlice := make([]int, 0, len(slice))
	for _, v := range slice { /* only one cuddle assignment allowed before range statement (wsl) */

		if v % 2 == 0 {
			newSlice = append(newSlice, v)
		}
	}

	return newSlice
}

The following codes are uncomfortable.

func evenFilter(a*A) []int{
	newSlice := make([]int, 0, len(a.slice))

	for _, v := range a.slice {

		if v % 2 == 0 {
			newSlice = append(newSlice, v)
		}
	}

	return newSlice
}

func evenFilter(a*A) []int{
	slice := a.slice
	newSlice := make([]int, 0, len(slice))

	for _, v := range slice {

		if v % 2 == 0 {
			newSlice = append(newSlice, v)
		}
	}

	return newSlice
}

I see some options like --no-test -allow-declaration, but I'm uncertain what they do.
I was looking for a way to disable some of the warnings about the:
block should not start/end with a whitespace
but I'm not sure if there is any such option

Cases in switch and select should force an empty newline between the cases unless the case block is X (2?) lines long.

Should have newlines:

switch foo {
case "a":
    ...
    ...
    ...
    ...
    ...
    ...
    
case "b":
    ...
    ...
    ...
    ...
    ...
    ...
    
case "c":
    ...
    ...
    ...
    ...
    ...
    ...
}

Should not have newlines

switch foo {
case "a":
    ...
case "b":
    ...
case "c":
    ...
}

Today only if err, if ok and if !ok is allowed without a whitespace.

This should be expanded to allow all if statements if the first condition is a variable assigned on the previous line.

Examples

foo, found := Get()
if !found {
    // Allowed
}

foo, errOne, errTwo := Do()
if errOne != nil || errTwo != nil {
    // Allowed
}

Unless the line above is missing a whitespace above!

foo := true
bar := false
biz := Some()
if !biz {
    // Not allowed - to cloggy!
}

// Use either
foo := true
bar := false
biz := Some()

if !biz {
    // Sweet!
}

// Or something like
foo := true
bar := false

biz := Some()
if !biz {
    // So much space!
}

I enabled this tool for my pipeline but now I'm seeing more spaces than needed like:

a := 1

if a == 1 {
...
}

I got another hit for:

        la := len(*a)                                                          
        lb := len(*b)                                                          
        if la != lb {                                                          
                h.Errorf("%s and %s has incorrect length: %v|%v",              
                        labelA,                                                
                        labelB,                                                
                        la,                                                    
                        lb,                                                    
                )                                                              
                                                                               
                return 3                                                       
        }

with the message:

thelper.go:449:2: only one cuddle assignment allowed before if statement (wsl)
        if la != lb {
        ^

False positive?

What exactly does this check want me to change, or is this a false-positive?

Repro:

foo := bar.Baz(foobar)
if t, ok := data[foo]; ok {
  data["hello."+foo] = t
  delete(data, foo)
}