runtime: PanicNilError error string should have "runtime error: " prefix

[Goclipse27]

What version of Go are you using (go version)?

$ go version
1.21.3

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
Windows, amd64, 1.21.3

What did you do?

package main

import (
    "fmt"
    "runtime"
)

type I interface {
    M()
}

func main() {

    defer func() {
        if r := recover(); r != nil {
            if e, ok := r.(runtime.Error); ok {
                fmt.Println(e.Error())
            }
        }
    }()

    defer func() {
        if r := recover(); r != nil {
            if e, ok := r.(runtime.Error); ok {
                fmt.Println(e.Error())
                panic(nil)
            }
        }
    }()

    defer func() {
        if r := recover(); r != nil {
            if e, ok := r.(runtime.Error); ok {
                fmt.Println(e.Error())
                a := 1
                b := 0

                fmt.Println(a / b)
            }
        }
    }()

    var i I
    i.M()

    //panic(nil)

    //fmt.Println("hello")

}

What did you expect to see?

Since Panic error is a different runtime error as mentioned in the documentation - https://pkg.go.dev/builtin#panic and it has implementation of method - RuntimeError // RuntimeError is a no-op function but // serves to distinguish types that are run time // errors from ordinary errors: a type is a // run time error if it has a RuntimeError method. RuntimeError()

I would like to see the error message like other runtime errors - "runtime error:" as mentioned in the above example

What did you see instead?

I rather the see the error message as - panic called with nil argument

[mauri870]

This is also confirmed by the Go specification:

The return value of recover is nil when the goroutine is not panicking or recover was not called directly by a deferred function. Conversely, if a goroutine is panicking and recover was called directly by a deferred function, the return value of recover is guaranteed not to be nil. To ensure this, calling panic with a nil interface value (or an untyped nil) causes a run-time panic.

This behavior was introduced in CL 461956

[mauri870]

@gopherbot please open a backport issue for Go 1.21.

Errors that implement runtime.Error should have a "runtime error: " prefix, with the solo exception of runtime.plainError on purpose. Calling panic(nil) results in a PanicNilError that violates this constraint.

[gopherbot]

Backport issue(s) opened: #63815 (for 1.21).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

[mauri870]

Looks like runtime.TypeAssertionError is also a runtime.Error that does not follow this convention.

[gopherbot]

Change https://go.dev/cl/538496 mentions this issue: runtime: add missing runtime error prefix to PanicNilError

[randall77]

I think it is past time to update runtime.TypeAssertionError. That's error has been around a long time and is not worth fixing. (We grandfathered a bunch of errors without the runtime error: prefix using plainError back as part of #14965. This one wasn't so marked, and maybe we can do that, but it would not be a user-visible change I think).

@rsc Do we want to fix PanicNilError to add a runtime error:?

[odeke-em]

Thanks everyone for chiming in and for the discourse. So my interpretation from the quoted spec links and docs is that the error prefix does not matter but the interface that's implemented which is runtime.Error does matter which is why I vote to say that this issue isn't a release blocker because as @randall77 mentioned, we grandfathered a bunch of runtime errors without that prefix and I am the one who implemented runtime.plainError. @randall77 also mentions a concern I'd have to that this behavior has been in for a very long time where some runtime errors don't have the prefix "runtime error:" and indeed I fear that could cause behavioral changes and moreover this very late in the cycle, thus I shall move this to the next milestone.

@raghvenders @mauri870 perhaps this discussion can come up in the next cycle Go1.23 but also please add this test to panic_test.go

func TestPanicNilErrorPrefix(t *testing.T) {
        tests := []struct {
                name      string
                wantPanic string
                fn        func()
        }{
                {
                        name:      "panic(nil)",
                        wantPanic: "runtime error: panic with nil argument",
                        fn: func() {
                                panic(nil)
                        },
                },
                {
                        name:      "panic((any)(nil))",
                        wantPanic: "runtime error: panic with nil argument",
                        fn: func() {
                                var foo any = nil
                                panic(foo)
                        },
                },
                {
                        name:      "panic((error)(nil))",
                        wantPanic: "runtime error: panic with nil argument",
                        fn: func() {
                                var err error
                                panic(err)
                        },
                },
        }

        for _, tt := range tests {
                tt := tt
                t.Run(tt.name, func(t *testing.T) {
                        defer func() {
                                r := recover()
                                if r == nil {
                                        t.Fatal("expected a panic")
                                }

                                re, ok := r.(runtime.Error)
                                if !ok {
                                        t.Fatalf("wrong panic type: got %T, want runtime.Error", r)
                                }
                                if !strings.Contains(re.Error(), "runtime error: panic called with nil argument") {
                                        t.Fatalf("mismatched message, missing `runtime error: panic with nil`, : got:\n%s", re.Error())
                                }
                        }()

                        tt.fn()
                })
        }
}

[joedian]

I see a referenced issue and fix for https://github.com/golang/go/pull/63816 . @randall77 how can we move Gerrit issue forward?

[randall77]

We just need the CL submittable. It was languishing with a trybot failure. I kicked off trybots again to see if that was a flake.

[rsc]

I'd be fine with leaving this alone but I'm also fine with changing this if people feel strongly.

[odeke-em]

Despite my advocacy to keep it as is due to grandfathering, we firstly don't have enough data on who really relies on the whole string "panic with nil argument", secondly it would seem consistent that runtime.PanicNilError which already implements runtime.Error should be prefixed with "runtime error:", thirdly that oversight looks like rolling forward it should be corrected for the future of Go. Therefore, I humbly suggest that we go forward with the fix.

[seankhliao]

a cursory search would indicate nobody relies on the actual string 0 results: https://github.com/search?type=code&q=+%22panic+with+nil+argument%22 only on the exported error https://github.com/search?type=code&q=runtime.PanicNilError+language%3AGo&l=Go