proposal: cmd/vet, x/tools/go/analysis/passes/errorsastype: detect misuse of `AsType` in `else if`

[mateusz834]

Proposal:

• Expose the internal errorsastype analyzer from gopls as x/tools/go/analysis/passes/errorsastype
• Make use of it in go vet analyzers suite.

#78889 (comment)

Rationale

So recently in a code review I have suggested:

var err error

if err, ok := errors.AsType[*FooErr](err); ok {
    doSth(err)
} else if err, ok := errors.AsType[*BarErr](err); ok {
    doSth(err)
}

And at first glance it looks like a completely valid code, but it is not.

The err passed to second errors.AsType is not the err declared in the first line, but it is the err declared in the first if stmt (which is always going to be a concrete-type nil (zero-val to be precise)).

I also don't think that any "shadowing" analyzer would help here, for example consider the output of #75368.

I understand this output as err in if and else if are shadowing the global err directly, bot not that the err in else if is shadowing the err in if and so on.

Since errors.AsType is a new thing I don't think we can have data to back this up with, but might be worth detecting since it seems like a easy thing to do by mistake.

CC @adonovan

[mateusz834]

What is worse, the first AsType can return ok == False with a nil err (concrete type, not interface), thus causing the next AsType to panic if the err type declared say Unwrap which accesses a field in the struct.

So there are two cases here either a unreachable condition or a runtime panic (I think), but all these unreachable condition cases can easily lead to a runtime panic, say after adding Unwrap.

[gabyhelp]

Related Issues

• x/tools/go/analysis/passes/errorsas: support errors.As provided by other packages #44784
• proposal: cmd/vet: missing error assignment check #19727
• cmd/vet: warn when errors.As target has type *error #47528 (closed)
• x/tools/go/analysis/passes/printf: report Errorf("%w", *E) where E implements error (defeats errors.AsType[E] etc) #61342 (closed)
• proposal: x/tools/.../shadow: specific cases that are unlikely to have false positives #58917
• x/tools/go/analysis/passes/shadow/cmd/shadow: fixing shadowed errs results in code more likely to cause errors #40113
• errors: AsType (As with type parameters) #51945 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[cagedmantis]

cc @golang/tools-team

[abhay1999]

I'd like to work on this.

Proposed approach: keep this in the existing errorsas analyzer, but make the new diagnostic deliberately narrow so it does not become a general shadowing check.

Specifically, detect an else if whose init statement is an errors.AsType short declaration, and report when the first argument to that AsType resolves to the result variable declared by the immediately preceding if/else if init statement. For example:

if err, ok := errors.AsType[*FooErr](err); ok {
	...
} else if err, ok := errors.AsType[*BarErr](err); ok {
	...
}

In the second call, the input err is the typed-zero *FooErr result from the first arm, not the outer error value. That makes later arms unreachable at best, and can panic if the typed nil has methods such as Unwrap that dereference the receiver.

Implementation-wise, I think the analyzer can:

1. walk *ast.IfStmt chains in each file;
2. recognize := init statements whose RHS is a call to errors.AsType;
3. record the first result identifier from each arm;
4. for each following else if, check whether the first argument to errors.AsType is an identifier whose types.Object is the previous arm's result object;
5. add tests for both the bad chain and non-diagnostics for cases that pass the original outer error under a different name.

This should avoid warning on ordinary shadowing and only catch the surprising AsType/else if interaction described here.

[mateusz834]

I already have a draft of this change, will send it soon.

[abhay1999]

Thanks for the heads up. I'll wait for your CL then, and I'm happy to help review/test it once it's up.

[gopherbot]

Change https://go.dev/cl/770360 mentions this issue: go/analysis/passes/errorsas: detect misuse of AsType in else if

[mateusz834]

My plan is to give it more thought during the weekend, but you can see what i have done up to this point here CL 770360

[abhay1999]

Thanks, I'll take a look at CL 770360 and leave any feedback there rather than duplicating discussion here.