golang/go

x/tools/gopls: nil Signature (?) deref in Completion

adonovan opened this issue · 9 comments

adonovan commented 
#!stacks
"runtime.sigpanic" && "go/types.(*Signature).Results:=94" && "expectedReturnStmtType"

Issue created by stacks.

// expectedReturnStmtType returns the expected type of a return statement.
// Returns nil if enclosingSig is nil.
func expectedReturnStmtType(enclosingSig *types.Signature, node *ast.ReturnStmt, pos token.Pos) types.Type {
	if enclosingSig != nil {
		if resultIdx := exprAtPos(pos, node.Results); resultIdx < len(node.Results) {
			return enclosingSig.Results().At(resultIdx).Type()
		}
	}
	return nil
}

This stack sZBuvA was reported by telemetry:

golang.org/x/tools/gopls@v0.17.0-pre.3 go1.23.3 darwin/arm64 other,vscode (10)

Dups: mCLZHw

adonovan commented

This is a strange one: the nil deref is dominated by a non-nil check.

adonovan commented

Closely related to #70636.

findleyr commented

There's definitely a bug here though. The condition checked is against len(node.Results), but in the presence of ill typed code, the signature length need not match the length of the return statement.

if resultIdx := exprAtPos(pos, node.Results); resultIdx < len(node.Results) {
    return enclosingSig.Results().At(resultIdx).Type()
}

My guess is that the compiler is somehow reporting the panic from the wrong call.

findleyr commented

Aha, well the bug I spotted is exactly #70636.

Therefore, I'm not sure what this bug is. If it were a bug in the compiler's pclntab, I'd have expected it to always report the wrong function.

findleyr commented

For triaging: we expect that this is really a dupe of #70636, somehow obscured by a misreported stack from the compiler. I'll leave this in the v0.17.0 milestone, but if it does not recur in the next prerelease, where #70636 is fixed, I think we can safely transfer this to a compiler/runtime bug.

findleyr commented

FWIW, I can reproduce this exact crash with gopls@v0.17.0-pre.3, and the repro from my fix for #70636. Therefore, this is a dupe.

adonovan commented

https://go.dev/cl/634038 is a reproducer for the problem that doesn't depend on telemetry.