Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
raghvenders opened this issue Oct 29, 2023 · 12 comments · May be fixed by #63816
Open

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

raghvenders opened this issue Oct 29, 2023 · 12 comments · May be fixed by #63816
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@raghvenders
Copy link
Contributor

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

@seankhliao seankhliao changed the title builtin: PanicNilError : 1.21 - Runtime Error message like Runtime Errors - runtime error: panic called with nil argument runtime: PanicNilError error string should have "runtime error: " prefix Oct 29, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Oct 29, 2023
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 29, 2023
@mauri870
Copy link
Member

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
Copy link
Member

@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
Copy link

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
Copy link
Member

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

mauri870 added a commit to mauri870/go that referenced this issue Oct 29, 2023
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.

This CL changes the error from panic(nil) from:

panic called with nil argument

to

runtime error: panic called with nil argument

Fixes golang#63813
@mauri870 mauri870 self-assigned this Oct 29, 2023
@mauri870 mauri870 added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 29, 2023
@gopherbot
Copy link

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

@randall77
Copy link
Contributor

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:?

@dmitshur dmitshur added this to the Go1.22 milestone Nov 1, 2023
@cherrymui cherrymui added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 1, 2023
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Nov 1, 2023
@odeke-em
Copy link
Member

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()
                })
        }
}

@odeke-em odeke-em modified the milestones: Go1.22, Go1.23 Dec 12, 2023
@joedian
Copy link

joedian commented Apr 24, 2024

I see a referenced issue and fix for #63816 . @randall77 how can we move Gerrit issue forward?

@randall77
Copy link
Contributor

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
Copy link
Contributor

rsc commented Apr 24, 2024

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

@odeke-em
Copy link
Member

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

10 participants