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

go/types: LookupFieldOrMethod no longer accepts a nil type (aka "should go/types accept nil types?") #50620

Closed
dominikh opened this issue Jan 14, 2022 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dominikh
Copy link
Member

In previous versions of Go, types.LookupFieldOrMethod would accept a nil type and return a nil object. In 1.18, it panics instead. The function was never documented to accept nil types and this probably doesn't constitute a breaking change. I'm filing this issue primarily to document the change, but also so we can consider supporting the old behavior explicitly.

Intuitively, the old behavior makes sense to me. Unfortunately, go/types isn't very consistent with handling nil types. Some functions, such as types.Comparable and types.IsInterface never supported nil, whereas other functions, such as types.Identical explicitly handle nil. And other functions, like previously types.LookupFieldOrMethod support nil by accident. This prompts the question: should we strive for consistent handling of nil types, or should we leave things alone?

Example program:

package main

import (
	"fmt"
	"go/types"
)

func main() {
	fmt.Println(types.LookupFieldOrMethod(nil, false, nil, "Foo"))
}

Go 1.17.6 output:

<nil> [] false

Go 3b5eec9 output:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x50c78f]

goroutine 1 [running]:
go/types.under({0x0?, 0x0?})
	/home/dominikh/prj/go/src/go/types/type.go:27 +0x4f
go/types.lookupFieldOrMethod({0x0?, 0x0?}, 0x0, 0x0?, {0x551a33, 0x3})
	/home/dominikh/prj/go/src/go/types/lookup.go:154 +0x673
go/types.LookupFieldOrMethod({0x0?, 0x0?}, 0x0?, 0x0?, {0x551a33, 0x3})
	/home/dominikh/prj/go/src/go/types/lookup.go:62 +0xc5
main.main()
	/home/dominikh/prj/src/example.com/foo.go:9 +0x32

/cc @findleyr @griesemer @mvdan

@dominikh dominikh added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 14, 2022
@findleyr findleyr added this to the Go1.18 milestone Jan 14, 2022
@findleyr
Copy link
Contributor

Thanks for filing. Adding the 1.18 milestone but not marking as a release blocker.

I don't think it should be a problem for us to preserve (and document) this behavior. I wonder if we should also try to normalize our APIs by always accepting nil types (i.e. implementing and documenting a non-panicking behavior with respect to nil types in Comparable and IsInterface, etc.). The latter needn't be done for 1.18, but it would be nice to have a plan. Need to think about it a bit more.

@findleyr findleyr self-assigned this Jan 14, 2022
@griesemer
Copy link
Contributor

griesemer commented Jan 14, 2022

The old behavior for nil types was likely accidental. Historically, I've tried to avoid nil types being accepted for type operations (and panic instead) unless documented otherwise, because of the chance of hiding bugs. That said, I just recently had a use of LookupFieldOrMethod where I wished it would handle the nil case. Of course it's trivial to do the nil check before the call.

In some places (Identical if I remember correctly) nil types are accepted for "fault tolerance", so as not to panic even in the presence of some other (unrelated) bug.

@griesemer griesemer self-assigned this Jan 14, 2022
@dominikh
Copy link
Member Author

I think I agree with griesemer. For Identical, aside from fault tolerance, it makes sense to support nil if you think of the function as a "smarter ==". For the other functions, not hiding bugs seems to be more important and more in line with usual Go behavior if we posit that nil isn't a valid Type.

I'm leaving this issue open so you can make the final decision, but I'd be inclined towards leaving things as they are, and fixing code that passes nils to LookupFieldOrMethod.

@gopherbot
Copy link

Change https://golang.org/cl/379374 mentions this issue: go/types, types2: explicitly check for non-nil type in LookupFieldOrMethod

@griesemer
Copy link
Contributor

Per the above discussion, leaving behavior of LookupFieldOrMethod for a nil type as before, but now testing for it explicitly, with an explicit runtime panic. The CL also adds a test so that we don't change this behavior in the future.

@griesemer griesemer added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 19, 2022
jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
…ethod

Document and enforce API expectation. Add a test so we don't
inadvertently change the function behavior with respect to nil
type arguments.

Fixes golang#50620.

Change-Id: Ic000bff7504a03006bd248a319c7a2d49dcf09c8
Reviewed-on: https://go-review.googlesource.com/c/go/+/379374
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Jun 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants