proposal: go/types: change Interface.Embedded to panic instead of returning nil for non-defined types #46423
Comments
mdempsky
added
the
NeedsDecision
|
Change https://golang.org/cl/340913 mentions this issue: |
|
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. |
|
FWIW, there are only 6 uses of Interface.Embedded in Google's internal code base. None of them check for a 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.) |
|
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
changed the title
gopherbot
added
Proposal
and removed
NeedsDecision
mdempsky
added
NeedsDecision
|
If people are checking for nil it seems like something we should probably not change? |
|
@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." |
|
This proposal has been added to the active column of the proposals project |
|
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. |
|
Based on the discussion above, this proposal seems like a likely decline. |
|
No change in consensus, so declined. |
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.)The text was updated successfully, but these errors were encountered: