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.

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.

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.

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

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.

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 }

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.

A blog post sounds like a good idea.

Who should I talk to about that?

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

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.

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

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.

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.

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

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

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.

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

(@dominikh, FYI)

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

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.

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

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.

CC @stevetraut

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.

I imagine it might not fit vet in terms of frequency,

I am not sure whether the frequency will warrant a vet check or not. On one hand, this seems like something that could remain silent and not be caught by testing. OTOH I am not sure folks will combine these features that often and also have a method shadowed. Data would be helpful here.