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

proposal: go/types: change Interface.Embedded to panic instead of returning nil for non-defined types #46423

Closed
mdempsky opened this issue May 27, 2021 · 10 comments

Comments

@mdempsky
Copy link
Member

mdempsky commented May 27, 2021

Last night I ran into an issue where I'd called Embedded(i) instead of EmbeddedType(i), and it took me a little while to realize the mistake. The issue instead manifested as nil-pointer panic elsewhere from trying to use the Type((*Named)(nil)) value I had inadvertently constructed. I think the mistake would have been easier and quicker to identify if Embedded(i) had simply panicked instead.

I think this is relevant to Go 1.18 because the introduction of generics and union types will probably increase the need for using EmbeddedType instead of Embedded. (Today, EmbeddedType is only needed when a type alias is used to embed an anonymous interface.)

/cc @griesemer @findleyr

Edit: For historical context, Interface.Embedded was originally provided and returned a *Named (i.e., defined type). This was sufficient because you could only embed defined types into interfaces. However, with Go 1.9, we added type aliases, so we needed a more relaxed variant of the function that could return arbitrary types, thus Interface.EmbeddedType. At the same time, we specified Interface.Embedded to return (*Named)(nil) if the i'th embedded type wasn't a defined type. I think this was a mistake, and seemingly no real world code relies on this behavior. (Instead, real world programs end up with a nil pointer dereference far away from the deprecated call site.)

@mdempsky mdempsky added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 27, 2021
@mdempsky mdempsky added this to the Go1.18 milestone May 27, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/340913 mentions this issue: [dev.typeparams] go/types: Interface.Embedded to panic if embedded type is not a defined type

@findleyr
Copy link
Contributor

Sorry for the late response on this, but shouldn't this be considered a breaking change, and not subtly so?

Existing code could be using Embedded and checking for nil return, as is currently documented. This code will now panic. I don't see how we can make this change.

@mdempsky
Copy link
Member Author

FWIW, there are only 6 uses of Interface.Embedded in Google's internal code base. None of them check for a nil result; they would all ultimately panic when Named.Obj is called on the nil *Named pointer.

I agree this is technically a backwards incompatible change, but I don't think it's a change that actually breaks any real world programs. It seems like all of the programs are already broken, and this will make it easier to identify why/where. (My 2c at least.)

@findleyr
Copy link
Contributor

If there's consensus that this is OK to do, I certainly won't object. Maybe this should go through the proposal process, since it's technically an API change?

@mdempsky mdempsky changed the title go/types: change Interface.Embedded to panic instead of returning nil for non-defined types proposal: go/types: change Interface.Embedded to panic instead of returning nil for non-defined types Aug 11, 2021
@gopherbot gopherbot added Proposal and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Aug 11, 2021
@mdempsky mdempsky added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Aug 11, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Aug 25, 2021
@rsc
Copy link
Contributor

rsc commented Aug 25, 2021

If people are checking for nil it seems like something we should probably not change?

@mdempsky
Copy link
Member Author

@rsc Agreed, but as I reported above, "[n]one of [the uses that I found] check for a nil result; they would all ultimately panic when Named.Obj is called on the nil *Named pointer."

@rsc rsc moved this from Incoming to Active in Proposals (old) Aug 25, 2021
@rsc
Copy link
Contributor

rsc commented Aug 25, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Sep 1, 2021

Especially since this is deprecated, it seems like we should just freeze it and instead work on flagging deprecated things more obviously? It doesn't seem worth making breaking changes to clean up old deprecated things.

@rsc
Copy link
Contributor

rsc commented Sep 1, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Sep 1, 2021
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Sep 8, 2021
@rsc
Copy link
Contributor

rsc commented Sep 8, 2021

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Sep 8, 2021
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

5 participants