Elements ignored when encoding struct embedding a Selfer

[2opremio]

Here's a simplified repro:

package main

import (
        "bytes"
        "fmt"

        "github.com/ugorji/go/codec"
)

// We have defined Selfers for a type (either with codecgen or manually)
type Embedded struct {
        Embedded1 string
        Embedded2 string
}

func (e Embedded) CodecEncodeSelf(encoder *codec.Encoder) {
        // phon4y output, in a real example this would encode the struct properly
        encoder.Encode("['phony']")
}

func (e *Embedded) CodecDecodeSelf(decoder *codec.Decoder) {
}

// We use embedded in a different structure
type Foo struct {
        Bar string
        Embedded
}

func main() {
        foo := Foo{
                Bar: "bar",
                Embedded: Embedded{
                        Embedded1: "embedded1",
                        Embedded2: "embedded2",
                },
        }

        // The Decoder only encodes the Embedded struct
        // and ignores Bar!
        buffer := &bytes.Buffer{}
        codec.NewEncoder(buffer, &codec.JsonHandle{}).Encode(foo)

         fmt.Printf("Output: %s\n", buffer.String())
}

The program above outputs:

Output: "['phony']"

completely ignoring the Bar element.

I would expect something in the lines of:

Output: { Bar: "Bar", Embedded: "phony" }

Or, with a properly defined selfer:

Output: { Bar: "Bar", Embedded1: "embedded1", Embedded2: "embedded2"  }

I guess that, since the embedded Selfer makes the full struct a Selfer too, the encoder is blindly applying (e Embedded) CodecEncodeSelf(), ignoring other fields. I think the Encoder must somehow distinguish between the case in which the struct embeds a Selfer and when the struct itself implements a Selfer directly.

I haven't tried it yet it but, as a workaround, I guess one can define a Selfer for Foo.

I haven't tested the decoding, but maybe that's broken too.

[ugorji]

This is a limitation of Go's embedded rules.

There's no way from within Go's reflection mechanism that you can determine whether a type implements an interface directly or if it does it based on what it embedded.

The only other option would be to statically look at every file in the current directory to see if the type has an exact implementation of the types. I tried going down that road before, but it was fraught with peril. Currently, codecgen depends on a runtime analysis of the reflection information and static AST of the file(s) that you are running codecgen upon.

Unfortunately, I don't think this can be fixed.

This has been a headache for me also! I have spent days trying to resolve this, and finally punted and put it on users to understand how Go's embedding works.

See #100 for another place where this was exhibited.

BTW, what was the problem with -tags=unsafe? This should be a low-cost, high-impact change for you.

[ugorji]

Closing as "WorkAsDesigned". See #100 for similar manifestation of the Go Reflection Limitation and how Go works per inheriting implementations of embedded types.

[2opremio]

This is a limitation of Go's embedded rules.

That cannot be true. I know for a fact that encoding/json respects extra elements when using embedded types implementing encoding/json.{Marshaller,Unmarshaller}.

In fact, before migrating to your library, elements in structs with embedded types using custom encoding/json.Unmarshallers were being marshaled as expected.

I think they just do a more sophisticated check to distinguish whether the type implements the interface directly or it contains an embedded type implementing it. Did you look into how they do it?

EDIT: You are right. I just confirmed that this is the behavior for encoding/json as well in https://play.golang.org/p/YFpfyCQSd7

What was happening in our case was that we were only using codecgen for a package (report) and embedded a type from that package:

type ControlInstance struct {
    ProbeID string `json:"probeId"`
    NodeID  string `json:"nodeId"`
    report.Control
}

The same would had happened if we defined custom encoding/json.{Marshaller,Unmarshaller} for all types in that package.

However, one rarely implements encoding/json.{Marshaller,Unmarshaller} making this Go's embedding behavior particularly bad for code-generators.

Thanks for the explanation.

[2opremio]

BTW, what was the problem with -tags=unsafe? This should be a low-cost, high-impact change for you.

I didn't investigate in depth, but I am getting binary garbage on the decoder's end (both with JSON and Msgpack). I am using -u with codecgen though and generating codecs for most types.

Would -tags=unsafe help a lot in that case too?

[ugorji]

Check out https://play.golang.org/p/LiVUN4-Z0F to see that this is a limitation of go's rules that even encoding/json cannot work around.

[ugorji]

@2opremio I just see now that you updated the comment and already realized that encoding/json is also limited in the same way.

Like you say, and like I said in #100, Go's embedded method inheritance is "wonky" :( and is good when it's useful, but it's bad at times like these :(

[ugorji]

In addition, this issue happens if you implement encoding/json.(M|Unm)arshaler or the replacements encoding.Text(M|Unm)arshaler. So the impact is spread.

[ugorji]

Regarding the unsafe, it is sufficient to pass the -u flag to codecgen.

The unsafe use is pretty low-risk also. It is only used when encoding structs, as we don't convert []byte to string when decoding the name of the struct field just so we can do a match.

Please explain what you mean by "I am getting binary garbage on the decoder's end".

[2opremio]

Please explain what you mean by "I am getting binary garbage on the decoder's end".

When I compile the library with -tags unsafe the JSON encoder seems to be generating binary garbage. I don't have a simplified repro, but I can reproduce it with https://github.com/weaveworks/scope/

I can prepare a branch and some instructions to reproduce if that's OK with you, I don't have time to work on an isolated right now.

[ugorji]

Fixed: See 9f21f8e (unsafe string-to-bytes conversion) and 5d64d76 (skip creating Selfer if already explicitly done).

Both of these changes should carry you over. Please test again and let me know.

[ugorji]

@2opremio Fixed: See 9f21f8e (unsafe string-to-bytes conversion) and 5d64d76 (skip creating Selfer if already explicitly done).

Both of these changes should carry you over. Please test again and let me know.

[2opremio]

I have bad news. 5d64d76 doesn't work for structs with embedded types, because its's using reflection. In the example I used above:

type ControlInstance struct {
    ProbeID string `json:"probeId"`
    NodeID  string `json:"nodeId"`
    report.Control
}

The Selfer for ControlInstance won't get generated if the Selfer for report.Control has been generated first. I have managed to make it work by being careful about the generation order (generate the Selfer for ControlInstance before the Selfers in the report package). However, that's really ugly and won't work in general (you can define and embed a Selfer in the same package or even the same file).

An AST-based approach would work (I was assuming you were going to do this, sorry for not warning you in advance). However, it would require the Selfers to be implemented in the input files of codecgen, which sounds more than reasonable to me.

[2opremio]

9f21f8e does fix the problem I was experiencing, thanks! However compiling with -tags unsafe, doesn't seem to impact the CPU consumption when already using codecgen -u.

[ugorji]

@2opremio I was aware that we could use the AST, but it was fraught with peril (even when I tried to do it previously). For example:

• we have to track that ControlInstance is not a Selfer directly (not hard, but could affect a redesign)
• More critically, it creates a discordance between codecgen and non-codecgen mode i.e. in non-codecgen mode, ControlInstance delegates to report.Control for its CodecEncodeSelf method (per the way Go's embedded method inheritance works), but in codecgen, we are hacking a different implementation. A design principle of the codecgen was that it gives exactly the same result as non-codecgen, only faster by bypassing the reflection overhead.

I really wanted this, and really wanted reflection to support it, but can't use it because we support down to Go 1.3.

[ugorji]

@2opremio per the -tags=unsafe, yes, that mostly is used in non-codecgen (aka reflection) mode. In codecgen mode, most of the reflection code is bypassed, and so the unsafe generated one is used.

However, where you have interface fields within your structs, you would still benefit from the -tags=unsafe. This is because codecgen cannot do anything with interfaces, so those have to use the reflection-based code.

[2opremio]

I was aware that we could use the AST, but it was fraught with peril (even when I tried to do it previously).

If you are not bought on the AST solution (and it seems you are not) I think it's safer to simply revert 5d64d76 . It makes code-generation non-deterministic because of ordering and it will break the codecs of users (at the very least it breaks mine :) ).

More critically, it creates a discordance between codecgen and non-codecgen mode

Well, they are used very differently. Users will rarely create their own Selfers explicitly, and if they do, only a small amount of types will be covered. However, when using codecgen users will generate Selfers for almost all types. This makes a reflection-based detection of Selfers much worse in codecgen.

A design principle of the codecgen was that it gives exactly the same result as non-codecgen, only faster by bypassing the reflection overhead.

5d64d76 breaks this. If we look at ControlInstance and the user doesn't explicitly implement any Selfers:

• non-codecgen will marshall all elements
• codecgen without 5d64d76 will marshal all the elements
• codecgen with 5d64d76 will marshal all elements or only Control depending on the generation ordering
• An AST approach would still generate all the fields.

In fact, codecgen without 5d64d76 is also broken in this respect. Let's now assume that the user himself defines a selfer for report.Control

• non-codecgen will only marshal the Control element
• codecgen without 5d64d76 will marshall all the elements
• codecgen with 5d64d76 will only marshal the Control field
• An AST approach mixed with a Selfer check of the fields could detect that it should only marshal the Control field. However, this requires distinguishing Selfers explicitly provided by the user from Selfers generated by codecgen.

If I were to choose, I would go for the mixed AST+Selfer detection approach, followed reverting 5d64d76 but providing a mechanism to disable automatic Selfer-generation for certain types (like #140)

[2opremio]

However, where you have interface fields within your structs, you would still benefit from the -tags=unsafe. This is because codecgen cannot do anything with interfaces, so those have to use the reflection-based code.

I think we now use explicit Selfers for all the interfaces we serialize. Thanks a lot for the explanation.

[ugorji]

Having a hard time following the logic.

To be clear, the only design principle is:
codecgen-created files should mirror the default reflection-based model at runtime. If it doesn't, that's a bug

If this design principle is not honored, it's a bug and I will be happy to fix it.

It's possible that one can alter the default behaviour by careful ordering. Once manual invocation is done, that's a possibility. If we can prevent this, we should also. Since go doesn't allow circular package dependenciess, then there is always a clear non-circular dependency graph. You should mirror that in running codecgen. Embedding is fraught with peril because serialization is by default reflection-based, and Go always containing structs inherit the methods of anonymous fields. The best solution here is:
do not embed fields which explicitly define how they are serialized (ie which define json.(Unm|M)arshaler, encoding.(Text|Binary)(Unm|M)arshaler, codec.Selfer, etc). Anything else, and you are fighting against the way the language works, which is not generally a winning proposition.

So I agree: 5d64d76 was a real bug that we fixed. With the fix, both reflection and codecgen work the same way at runtime.

[2opremio]

The best solution here is: do not embed fields which explicitly define how they are serialized

My point is 5d64d76 causes codecgen to break the serialization of embedded fields, even if you don't explicitly define how they are serialized.

EDIT: Crap, I meant 5d64d76 instead of 9f21f8e in all the comments above. I changed it, sorry.

[2opremio]

So I agree: 5d64d76 was a real bug that we fixed. With the fix, both reflection and codecgen work the same way at runtime.

The problem is 5d64d76 breaks serialization for embedded types for which the user didn't explicitly define Selfers.

[ugorji]

I think you mean 5d64d76 . I'm still not clear what "break" means. If it was already "broken" in reflection mode, then it's already broken, and we just maintain the same behaviour in code-generation mode.

Question: can you explain what you mean by "it breaks serialization"? I just want to ensure that I understand exactly what you are alluding to.

Note that This was a sore spot for me too. I had already written the code to handle it (ie look through all files in the folder, find the CodecSelfer methods, track them, etc), before I realized that tests broke because behaviour between reflection and codecgen mode was different. I had to remove the change, drugingly. That's why I said it was fraught with peril.

I know I sound like I'm defending the behaviour. But I actually am sad; I gave up fighting it and just resigned to how it works. Embedding is good, until it's not, and it's a tough lesson. When you embed, you get to inherit ALL the methods, even those you don't want. In my code now, I only embed types which are purely value types, or if the method is consistent e.g. a type has an id field and adds an ID method, which all containing types must have.

[2opremio]

I think you mean 5d64d76

Yes, I am deeply sorry, I am used to keep track of issue numbers not git SHAs. I have corrected it. Do my comments make more sense now?

Question: can you explain what you mean by "it breaks serialization"? I just want to ensure that I understand exactly what you are alluding to.

Let's get back to my example:

type ControlInstance struct {
    ProbeID string `json:"probeId"`
    NodeID  string `json:"nodeId"`
    report.Control
}

I don't explicitly define selfers for any of the fields so, the non-codecgen behavior is to serialize all elements of ControlInstance, including ProbeId and NodeID.

With 5d64d76, if codecgen generates Selfers for the report package (where Control is defined) before it does for ControlInstance, the ProbeID and NodeID elements are ignored for serialization.

This contrasts with codecgen before 5d64d76 and the non-codecgen behavior, and that's what I mean by it breaks serialization for embedded types.

But not only that, 5d64d76 makes code generation non-deterministic, which is a maintenance nightmare. Even if I come up with an initial code-generation order which makes it work (i.e. serialize the ProbeID and NodeID fields), the second time I generate Selfers for ControlInstance (e.g. due to a file change) will make the ProbeID and NodeID fields be ignored when serializing unless I carefully remove the Selfers from the report package first.

[2opremio]

To be clear, the only design principle is:
codecgen-created files should mirror the default reflection-based model at runtime. If it doesn't, that's a bug

I understand. Do you see how 5d64d76 doesn't respect that principle for embedded types due to its impact of code-generation ordering?

I would much rather get compile-time clashes on Selfers (i.e. pre-5d64d76) than make the encoding/decoding behavior depend on the code-generation order.

[2opremio]

A solution would be to modify 5d64d76 so that it skipped generating Selfers only if the Selfer is explicitly implemented by the user (i.e. not due to a Selfer implementation previously generated by codecgen, in particular, a Selfer implementation generated by codecgen for an embedded field). But ... I am not sure how to detect this or whether it's possible at all.

[ugorji]

With your example, you should want to run codecgen in dependency order

1. run codecgen on report package
2. run codecgen on package containing ControlInstance

If you don't run the codecgen in dependency order, you have bad results e.g. Selfer method on a regular field is not called because the Selfer method on that type was not yet done, etc. Best practice is:

• run codecgen on all files in a package together
• run codecgen on different packages in dependency order

With this, you get same behaviour as reflection mode.

Assuming you write custom Selfer implementations for some types, then the system will behave completely the same whether or not you do 1. and/or 2. above. There's really no way around it: if you embed a type which defines a known method, that method will be used at runtime, and codecgen shouldn't help you workaround that.

I don't think there's any way to workaround this. Gotta just bite the bullet, and be deliberate when you embed types which have methods which could affect behaviour. It's a tough pill to swallow ;(

[2opremio]

With your example, you should want to run codecgen in dependency order

1. run codecgen on report package
2. run codecgen on package containing ControlInstance

I guess you mean the other way around, otherwise the non-embedded fields will be ignored.

With this, you get same behaviour as reflection mode.

True, but:

• Relying on a specific order is really inconvenient.
• Relying on a specific order is not enough. 5d64d76 also breaks incremental code generation. If I want to regenerate the code of a package I must first wipe out the code generated for all its dependencies.

I don't think there's any way to workaround this

I am not an expert in Go's reflect but I think I have a way with which to distinguish when a user implements implements Selfer explicitly for type from when the implementation was generated by codecgen:

Codecgen could generate an extra method identifying the type for which the Selfer was generated.

For instance, when invoking codecgen for type Foo:

func (*Foo) GetCodecgenTypeName() string{
  return "Foo"
}

func (*Foo) CodecEncodeSelf(*Encoder) {
}
func (*Foo) CodecDecodeSelf(*Decoder) {
}

Then, you could modify 5d64d76 to only skip generating a Selfer for some type T when:

1. T implements Selfer
2. T doesn't implement GetCodecgenTypeName or, if it does, then invoking GetCodecgenTypeName() != "T"

(2) ensures that the Selfer was provided by the user and not due to codecgen-generated Selfer, including the particular case of an embedded codecgen Selfer

[ugorji]

You are suggesting ways to workaround it, but i'm not willing to look at workarounds because the fundamental premise is broken i.e. enclosing types inherit the embedded methods, and there's no workaround for that. Anything we do will cause discordance.

Forget the inconvenience of the ordering, etc. The main design principle doesn't give us room to play.

[2opremio]

Alright, I give up :) Are you still willing to merge #140 ?

[ugorji]

Yes - definitely will merge #140. I can't look at it today (working on a
deliverable for work now). Will think through it, review and merge tomorrow
morning.

On Wed, Feb 17, 2016 at 11:36 PM, Alfonso Acosta [email protected]
wrote:

Alright, I give up :) Are you still willing to merge #140
#140 ?

—
Reply to this email directly or view it on GitHub
#141 (comment).