Allow `defer` after error-handling part

[LasTshaMAN]

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.

[bombsimon]

Thanks for the report!

Yeah this seems like a 50/50 to me, I don't have a strong opinion and would be fine accepting both ways. It might be a bit tricky to figure out in the current state of wsl though. The logic should be something like if the defer statement uses a token that was assigned in the statement before the block I'm cuddled with, it should be ok. This includes for and range statements as well I guess?

list, doneFunc := callF()
for _, x := range list {
    // ...
}
defer doneFunc()

Also, I think this should be isolated to defer because otherwise the whole don't cuddle blocks idea would fail. I don't think this should be valid, what's your opinion?

someF, err := getF()
if err != nil {
    return err
}
someF()

I'm just curios though, you mention Go-best-practices and say that the current way is less ideomatic. I cant recall reading about this kind of details in eihter Effective Go or any other article. Is this just a feeling you have after reading a lot of code or is this stated anywhere?

I know the http example which lead to the PR you mention shows this as an example but I wouldn't assume that it qualifies as best practices just because of that. I don't think a single file in standard library passes wsl without warnings so if the Go authors uses the proper and idiomatic way to write code I think this linter does a terrible job trying to push that code style. 😃

[LasTshaMAN]

Thanks for the detailed reply.

The logic should be something like if the defer statement uses a token that was assigned in the statement before the block I'm cuddled with, it should be ok.

This sounds good to me.

This includes for and range statements as well I guess?

I would say yes.

Also, I think this should be isolated to defer

Agree.

I'm just curios though, you mention Go-best-practices and say that the current way is less ideomatic. Is this just a feeling you have after reading a lot of code or is this stated anywhere?

Yes indeed. Maybe ideomatic isn't the best word to use here, it is simply "my opinion" based on all the examples of Go code I've seen out there so far. I can sum it up as follows:

• "func call + relevant err handling + relevant defer statements" are usually considered to be a part of the same "block of code", I want to be able to keep such a block of code separated from another similar blocks of code by using "empty lines" (or move this block of code around as a single entity, e.g. move it to a separate function as a part of refactoring)
• currently wsl thinks that defers don't belong in that "block of code" I'm describing here

I don't think this should be valid, what's your opinion?

My reasoning applies only to defer statements, in the example with someF you've outlined above someF is no different from any other variable a func might return, and (in a more readable version we are striving for) this variable usage below should be separated by a blank line from the block of code where it was obtained.

if the Go authors uses the proper and idiomatic way to write code

Having read some code from Go standard library (or Go implementation itself), I would say Go authors don't particulary care about making their code readable. Although it could have been worse =)

[ansel1]

I've run into a similar case in tests when using assertion frameworks:

db, err := OpenDB()
require.NoError(t, err)
defer db.Close()

Currently, wsl wants a line before defer. Unfortunately, I'm not sure what heuristic would work to recognize this case. Maybe "allow defer to cuddle with expressions that reference an error value from the previous assignment".

For now, I'm excluding test code from wsl, which is fine. Our test code is littered with assertion functions, which wsl has no special handling for, and it's not clear to me yet what would be desirable formatting for them.

[ansel1]

Another defer variation:

tx := BeginTx(db)
defer func() {
  EndTx(tx, err)
}()

wsl wants a line before the defer statement. In this case, EndTx is being wrapped in an anonymous function because we don't want to evaluate the err variable until the end of the function. It would be nice if wls recognized that the function is referencing variables assigned in the prior line, but I'm not sure how tricky that would be. The function could be long.

[bombsimon]

Thanks for the feedback @ansel1!

I agree that the first example might be hard to identify, there's not really a common pattern to look for. I think the best would be to com up with a different pattern for this but I won't suggest any without the full context. If you come up with something, please let me know.

The second example I thought would work actually. The linter is supposed to allow usages of identifiers assigned directly above the block if it's used in the first statement. This is valid:

n := 1
if bool {
    n = 1
}

So I think this should be as well

n := 1
if bool {
   fn(n)
}

I will try to look into the second issue this weekend!

[ansel1]

Re: the second case...

n := 1
if bool {
    fn(n)
}

probably works, but I don't think...

n := 1
defer fn() { 
    fn2(n) 
}()

...will work, where n is referenced inside an anon function.

Regardling the first case, could the rule be "if the next line is references the error, it's ok?" So

db, err := OpenDB()
require.NoError(t, err)
defer db.Close()

...would be equivalent to...

db, err := OpenDB()
if err != nil {
    return err
}
defer db.Close()

...because both if err {} and fn(err) refer to the error returned by the prior call?

[bombsimon]

I added a PR to fix the defer block. This does not solve the anonymous function call like this:

n := 1
func() { 
    fn2(n) 
}()

But... Does that make sense? When would you write that instead of just

n := 1
fn2(n)

If it's for later use, wouldn't it be

n := 1
x := func() {
    fn2(n)
}

Which would not yield an error.

[ansel1]

Sorry, that was a type. Declaring and calling an anonymous function doesn't make sense, unless you are deferring the call:

n := 1
defer func() {
  fn2(n)
}()

The usefulness of that construct is when you need to defer evaluation of the n variable. In our case, n is an error, and we want to do some error processing at the end of the method:

tx := BeginTx(db)
defer func() {
  EndTx(tx, err)
}()

We only want to evaluation err at the end of the method. If err != nil, we rollback the transaction.

[bombsimon]

Both of those examples will work when #101 is merged.

I will try to get back to the initial question and other ways of cuddle defer but that won't be as quick as this one. Sorry for not being to quick to fix these feature requests quicker.

[ansel1]

That seemed pretty quick to me! Thanks for the help!

[Davincible]

@bombsimon hey hey, absolutely love the progress on https://github.com/bombsimon/wsl/pull/72.

My only other issue with the linter has been this one. From a quick glance at the discussion above defer should be allowed now? But I do get wsl notices for my defer statements, and the issue is still open.

Do you know about what's going on with this issue now?

[bombsimon]

Thank you!

My plan now is to get the fixer working with as few changes as possible. Realising I'll have to bump major version for that release I want to take a pass on all the open issues to see if I can find an somewhat easy solution for them. If I can fit them I'll do it in the same release, if not I hope I can get back to them with a bigger rewrite like I mentioned in #72.