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 · 6 comments · May be fixed by #63816
Open

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

raghvenders opened this issue Oct 29, 2023 · 6 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

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
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.

7 participants