ir: understand the effects of testing.TB.Fatal et al on control flow

[dominikh]

testing.TB is an interface, so we can't deduce any information about its methods. However, only the standard library can implement testing.TB and we can special case its methods.

This does, however, raise the question what we'll do about user-defined interfaces that affect control flow. This primarily affects SA5011, in that it may produce false positives when nil pointer checks abort control flow through an interface method.

[tomislavmitic2012]

This potentially will cause problems for my team when running static check with our CI tool. Are there any workarounds for this until it is addressed.

[dominikh]

@tomislavmitic2012 several:

1. run a released version of staticcheck instead of the master branch
2. disable SA5011
3. follow calls to testing.TB.Fatal with explicit returns

I would strongly suggest option 1, followed by option 2.

[tomislavmitic2012]

thanks, will do

[yasushi-saito]

Can we support a directive such as

func Panic() { // linter:noreturn
  panic("hello")
}

the same problem happens with any function that panics or terminates the process by design.

[dominikh]

@yasushi-saito Any static function call, i.e. one not involving interfaces, already takes into consideration whether the called function panics, calls os.Exit or runtime.Goexit – and it does so for deeply nested calls, too. Interfaces should be the only instances in which this detection fails.

If you have counter-examples, please file a separate issue with a minimal reproducible piece of code.

[tamird]

@tomislavmitic2012 several:

1. run a released version of staticcheck instead of the master branch
2. disable SA5011
3. follow calls to testing.TB.Fatal with explicit returns

I would strongly suggest option 1, followed by option 2.

Option 1 is no longer viable, right? This now affects v0.1.1, for instance, which you can see in this CI run (https://buildkite.com/gvisor/pipeline/builds/2402, see nogo targets).