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
Assignees
Labels
Milestone

Comments

@dominikh
Copy link
Member

@dominikh dominikh commented Jan 14, 2022

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

@findleyr findleyr added this to the Go1.18 milestone Jan 14, 2022
@findleyr
Copy link
Contributor

@findleyr findleyr commented Jan 14, 2022

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

@dominikh dominikh commented Jan 18, 2022

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

@gopherbot gopherbot commented Jan 19, 2022

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

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jan 19, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants