Panic case

[Antonboom]

Hi!

Please look at this simple example:

func didPanic(fn func()) (result bool) {
	result = true // Handle `panic(nil)` case.
	defer func() { recover() }()
	fn()
	return false
}

assigned to result, but reassigned without using the value (wastedassign)
        result = true
        ^

[sanposhiho]

Not bug. Your case should be written like below to avoid the warn from wastedassign.

func didPanic(fn func()) (result bool) {
	defer func() {
		result = true 
		recover()
	}()
	fn()
	return false
}

[Antonboom]

@sanposhiho it's absolutely different code :) it doesn't work properly for not panic case: https://goplay.tools/snippet/mrRqJbl_pTP

package main

import "fmt"

func main() {
	fmt.Println(didPanic(func() {}))                 // Must be false.
	fmt.Println(didPanic(func() { panic("boom!") })) // Must be true.
	fmt.Println(didPanic(func() { panic(nil) }))     // Must be true.
}

func didPanic(fn func()) (result bool) {
	defer func() {
		result = true
		recover()
	}()
	fn()
	return false
}

/*
true
true
true
*/

[nickajacks1]

@Antonboom recover() returns the value passed to panic(), so you can check that to conditionally set result to true. https://go.dev/blog/defer-panic-and-recover

	defer func() {
		if r := recover(); r != nil {
			result = true
		}
	}()

[Antonboom]

@nickajacks1 hi!

Thank you for your attention, but your code is not valid for Go < 1.21, because I can do panic(nil).

Anyway the question is not "how to rewrite the code", but "how to improve the linter".

[nickajacks1]

I think I understand now. If fn panics, the assignment in the original code is technically used. I'm not really familiar with Go's AST, but this sounds complex to statically determine. Some thoughts:

• since fn is not first nil-checked, calling it can panic, so we can statically determine that it could panic.
  • Otherwise, I assume that it is impossible to know if fn panics without looking at every single usage of didPanic
• wastedassign could theoretically check for calls to recover within a defer function and assume that any called function object can panic. This could lead to false-negatives.
  • I noticed that ineffassign has opted for this route, though it disclaims in the readme that it opts for false-negatives over false-positives.
• You could tell wastedassign that a function could panic with some annotation comment, but that's no better than using a comment to disable the lint warning on the variable assignment.
• panic(nil) is probably dangerous anyway (sorry to pick on the example again...)

So I'm not really sure what the best move here is. I've noticed other times where wastedassign seems to opt for false-positives over false-negatives:

// wastedassign gives a "false positive" here (or maybe it's a bug, I haven't really looked deeper)
thing := 0
switch arg {
case 1:
	thing = 3
case 2:
	thing = 39
default:
	thing = 100
}