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

spec: surprising rules for inheriting methods when declaring named types #41685

Open
mvdan opened this issue Sep 28, 2020 · 24 comments
Open

spec: surprising rules for inheriting methods when declaring named types #41685

mvdan opened this issue Sep 28, 2020 · 24 comments

Comments

@mvdan
Copy link
Member

@mvdan mvdan commented Sep 28, 2020

First, a piece of code that's minimized from a real bug we hit at work. What do you think this code does? https://play.golang.org/p/-FkcWvJzLZS

json.Unmarshal on Embedding uses the method we declared directly on that named type, which just does nothing, so we end up with a nil error. All as expected so far.

It gets trickier with the equivalent unmarshaling with type NewNamed Embedding. We get a nil pointer dereference panic, but that's not really important; what matters is that, as one can see in the panic stack trace, we call math/big.(*Int).UnmarshalJSON.

type Embedding is overriding the promoted UnmarshalJSON, so it's troublesome that one can sidestep that entirely by simply doing type T Embedding.

The other part about this behavior that surprises me is that, if we have a non-interface named type Bar, there's no way to tell if type Foo Bar will result in Foo having an empty method set or not. I always thought that the only way to define a type which inherited methods was via either type T SomeInterface or type T struct { Bar }. The following playground link further shows this inconsistency: https://play.golang.org/p/5uV8obYY8aR

The spec says:

A defined type may have methods associated with it. It does not inherit any methods bound to the given type, but the method set of an interface type or of elements of a composite type remains unchanged

From that text, I understand that of an interface type means our type T SomeInterface example above, and of a composite type means type T struct { Bar }.

I'm not sure whether to call this a spec bug or a request that it be clarified, but at the very least I find the behavior very unexpected, to the point that it can cause real bugs like the one I first showed in the issue. So I lean towards thinking that this is something we should fix in the implementation, not just clarifying the spec, because it feels like an unnecessary footgun.

I should note that I've checked both encoding/json and reflect for bugs, but haven't found any; they seem to both follow what the compiler does.

cc @griesemer @ianlancetaylor

@Merovius
Copy link

@Merovius Merovius commented Sep 28, 2020

I agree that the behavior is kind of surprising, but I don't think it can be changed. The spec says in the section about struct types

Given a struct type S and a defined type T, promoted methods are included in the method set of the struct as follows:

  • If S contains an embedded field T, the method sets of S and *S both include promoted methods with receiver T. The method set of *S also includes promoted methods with receiver *T.
    […]

And T1 in your example is IMO undoubtedly a struct type and T1 contains an embedded field WithMethod. So, IMO the spec pretty unambiguously agrees with the implementation. Changing it would IMO be breaking the compatibility promise.

One phrasing that would be unambiguous for the section you quoted could be

A defined type's method set contains all methods of its underlying type.

Because the underlying type is always either a type-literal or a predeclared type, this covers both the interface-case and the embedding-case and says that if neither is the case, there are no methods. This probably still wouldn't be much clearer, though. So for clarity, a "in particular, this means that methods of embedded fields and methods of interface types are inherited" could be added. And in any case, an appropriate example with embedding should probably be added to the block following your quote.

@mvdan
Copy link
Member Author

@mvdan mvdan commented Sep 28, 2020

I get that changing the spec and breaking programs is very unlikely, but I wanted to bring it up because I find the original JSON example to be a behavior bug from the point of view of the user.

I'm all for making the spec clearer or adding examples. It would have made my life easier trying to figure this out, at least. Having said that, I still find it concerning that overriding promoted methods in named types isn't final; they can be sidestepped pretty easily as I showed earlier. Which makes me worry about the scenario where the embedded type is unexported in an API, for example.

@mvdan
Copy link
Member Author

@mvdan mvdan commented Sep 28, 2020

One more thought before I head off: assuming that this is working as intended, beyond spec clarification, I would argue that a static analysis tool could also be nice. For example, any exposed type which embeds another type and overrides a method should trigger a warning, because any API user could sidestep that override by defining a new type. And I would imagine that the vast majority of API writers aren't realising that they are exposing two method sets that way.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 28, 2020

The language is working as I expect. I agree with the comments above that this would be hard to change.

I also don't think that it would be helpful to have a warning about overriding a promoted method of an embedded type. That is a standard technique, in which the outer type wants to promote some methods and override some other methods.

Clarifying the spec would of course be fine with me.

I think the lesson to take away here is: embedded types should be used with care. Promoted methods can bite you in various ways. I still like my example from a few years ago at https://talks.golang.org/2014/compiling.slide#17 (the "run" button on that slide should work).

@mvdan
Copy link
Member Author

@mvdan mvdan commented Sep 28, 2020

That is a standard technique, in which the outer type wants to promote some methods and override some other methods.

My point is that the overriding appears to be entirely optional, as any consumer can work around it by defining a new type, even if they are in a third party package or don't have direct access to the embedded type.

I think the lesson to take away here is: embedded types should be used with care.

I generally agree, but I don't think many Go developers will be aware to what extent it's a footgun. I hadn't realised that one could sidestep method overrides until today, for example, so I'm suddenly worried that multiple of the APIs I've written in the past with method overrides could have that issue.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 28, 2020

My point is that the overriding appears to be entirely optional, as any consumer can work around it by defining a new type, even if they are in a third party package or don't have direct access to the embedded type.

Understood. For cases where defensive programming is a concern, the fix is

type privateEmbeddor struct { embedee }
func (privateEmbeddor) OverridingMethod() {}
type Embeddor struct { privateEmbeddor }
@mvdan
Copy link
Member Author

@mvdan mvdan commented Sep 28, 2020

In hindsight, I should have raised this issue as more of an experience report about how overriding methods is much easier to mess up than most Go developers think. I'm still not sure what the best solution to that today could be, beyond trying to clarify the spec and warn users that struct embedding should be used with more care (and showing a few pitfall examples).

Maybe a blog post on golang.org, similar to how before there was a post covering the internals and common pitfalls around slices.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 28, 2020

A blog post sounds like a good idea.

@andybons andybons added this to the Unplanned milestone Sep 29, 2020
@mvdan
Copy link
Member Author

@mvdan mvdan commented Sep 29, 2020

Who should I talk to about that?

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Sep 29, 2020

FWIW here's an example where this behaviour would genuinely have tripped me up. I've used this noMethods idiom quite a lot to implement custom marshaling behaviour. I will be more careful from here on!

In the following example, I'd expect to see a JSON object (because the Bar value isn't zero), but instead we see null:

https://play.golang.org/p/j8HdbL3kfSA

For the record, here's one way of working around the problem: https://play.golang.org/p/TT1BXg1rljR

@jba
Copy link
Contributor

@jba jba commented Sep 30, 2020

I was bitten by promoted methods from embedding a while ago (https://golang.org/issue/18480), but the behavior of defined types that @mvdan describes is new to me, and I don't think the spec supports it.

In @mvdan's

type Embedding struct{ ... }
type T1 Embedding

we have a defined type, T1. Contrary to what @Merovius says, it is not a struct type, so the passage he quotes about what methods are in a struct type's method set should be irrelevant.

The spec is quite clear that a defined type "does not inherit any methods bound to the given type" and illustrates with an example with the same structure as Dan's:

type Mutex struct         { /* Mutex fields */ }
func (m *Mutex) Lock()    { /* Lock implementation */ }
func (m *Mutex) Unlock()  { /* Unlock implementation */ }

// NewMutex has the same composition as Mutex but its method set is empty.
type NewMutex Mutex

Here, Mutex corresponds to Embedding and NewMutex to T1. The elision of the Mutex fields is the spec's, not mine, so I see nothing to suggest that an embedded field of Mutex could change the situation.

The full sentence in the spec is:

It does not inherit any methods bound to the given type, but the method set of an interface type or of elements of a composite type remains unchanged.

The two exceptions are clearly illustrated by the examples following. The first is

type I interface {...}
type D I

and the second is the usual

type D struct {...}

Those are the only two cases where a defined type inherits methods from its "given" type. type T1 Embedding is not one of those cases; Embedding is itself a defined type, not an interface or composite type.

So I see no justification in the spec for T1 inheriting methods from Embedded.

@Merovius
Copy link

@Merovius Merovius commented Sep 30, 2020

Contrary to what @Merovius says, it is not a struct type

How do you come to that conclusion? IMO it is a type that is a struct, so it is a struct type. I had this discussion with someone on Twitter as well, so note that the spec in other places uses that interpretation of the term "struct type". For example

If the base type is a struct type, the non-blank method and field names must be distinct.

where "the base type" is the receiver type or what the receiver type points to - so it's always a defined type.

Arguing that T1 in the example is not a struct type is IMO akin to arguing that time.Duration is not an integer type.

The spec is quite clear […]

Well, given that we are having an argument about how to correctly interpret it, it obviously isn't that clear :)

FWIW, I'd argue that with something like embedding, if there is an ambiguity in the spec and all known implementations agree with one interpretation of it, then that interpretation should be considered correct. We are more than 10 years into go1, it's hard to argue that the language has been different all this time. If the issue was that implementations differed in behavior, then trying to lawyer around the spec might be useful. But the implementations agree, so we really should have a conversation around what the spec should be, to accurately describe the behavior as implemented.

[edit] And FWIW, I did try to make some suggestions on what the spec might say in my comment above :)

@myitcv
Copy link
Member

@myitcv myitcv commented Sep 30, 2020

Contrary to what @Merovius says, it is not a struct type

I actually agree with @Merovius because of the following line in the spec:

A type definition creates a new, distinct type with the same underlying type and operations as the given type, and binds an identifier to it.

That is to say, given:

type Embedding struct{ ... }
type T1 Embedding

T1 is a new, distinct type with the same underlying type and operations as the given type, Embedding. The underlying type of Embedding is struct{ ... }. Hence T1 "gains" the methods that type implies, but does not, per:

A defined type may have methods associated with it. It does not inherit any methods bound to the given type, but the method set of an interface type or of elements of a composite type remains unchanged:

inherit any of the methods of Embedding.

@jba
Copy link
Contributor

@jba jba commented Sep 30, 2020

The phrase "the method set of an interface type or of elements of a composite type remains unchanged" needs to be clarified to refer to underlying types. The example for interfaces that follows illustrates that, but not the one for composite types.

@bcmills
Copy link
Member

@bcmills bcmills commented Sep 30, 2020

I wonder: would it make sense to have a vet warning for the case when a defined type exposes an otherwise-shadowed method from an embedded field?

type Embedding struct {
	*big.Int  // ok
}

func (*Embedding) UnmarshalJSON(b []byte) error { return nil }  // ok

type NewNamed Embedding  // vet: method (*NewNamed).UnmarshalJSON refers to embedded (*big.Int).UnmarshalJSON, not (*Embedding).UnmarshalJSON
@bcmills
Copy link
Member

@bcmills bcmills commented Sep 30, 2020

The vet warning could be silenced by overriding the method explicitly to clarify. Either:

func (n *NewNamed) UnmarshalJSON(b []byte) error { return n.Int.UnmarshalJSON(b) }

or

func (n *NewNamed) UnmarshalJSON(b []byte) error { return (*Embedding)(n).UnmarshalJSON(b) }
@mvdan
Copy link
Member Author

@mvdan mvdan commented Sep 30, 2020

I agree that a static analysis check for when one bypasses method overriding would be very useful; I brought it up earlier in this thread, but didn't seem to get much support :) I imagine it might not fit vet in terms of frequency, in which case we could hand the idea to staticcheck.

@bcmills
Copy link
Member

@bcmills bcmills commented Sep 30, 2020

I imagine it might not fit vet in terms of frequency, in which case we could hand the idea to staticcheck.

(@dominikh, FYI)

@jba
Copy link
Contributor

@jba jba commented Sep 30, 2020

@mvdan you asked for a warning for "any exposed type which embeds another type and overrides a method," which as @ianlancetaylor pointed out is a reasonable thing to do. @bcmills's suggestion is different.

@mvdan
Copy link
Member Author

@mvdan mvdan commented Sep 30, 2020

Right, my suggestion was more agressive because it intended to find this problem at the time of releasing an API with the potential flaw, not when the flaw is exposed at a later time when the API is consumed via a type NewNamed imported.Embedding. I imagine Bryan's is a better suggestion if we assume that most people will run vet/staticcheck, and that we would prefer false negatives to false positives.

@bcmills
Copy link
Member

@bcmills bcmills commented Sep 30, 2020

I do assume that most people will run vet, because I assume that most people will run go test.
(But that would also require adding the check to the set implicitly run by go test, which IIRC is an even higher bar than vet in general.)

@mvdan
Copy link
Member Author

@mvdan mvdan commented Oct 12, 2020

A blog post sounds like a good idea.

Who should I talk to about that?

Friendly ping :)

Aside from that, I'm happy to repurpose this issue around clarifying the spec a little, either with clearer wording or with an added example. I also think that the vet check that @bcmills proposes would be worthwhile, and I think we should file a separate issue about that. Bryan, I'm happy to help co-author a short proposal if you're up for it.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 12, 2020

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Dec 16, 2020

Contrary to what @Merovius says, it is not a struct type, so the passage he quotes about what methods are in a struct type's method set should be irrelevant.

Whether a type is defined is orthogonal to its kind. For example, bool is both a boolean type and a defined type: https://golang.org/ref/spec#Boolean_types

Thus in type T struct { ... }, T is both a defined type and a struct type.

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

Successfully merging a pull request may close this issue.

None yet
9 participants