Embedding of other packages' Selfer-implementing types

[sttts]

In https://github.com/ugorji/go/commit/5d64d76d3ac4d5450009b0f5fa1530b790fccc71 support was added to skip Selfer codec generation for a type if it already implements Selfer.

If a type Tembeds another type from another package which also uses ugorji/go to generate codecs, the embedded type's Selfer implementation is promoted to T. The commit above assumes then that T implements Selfer (which is technically correct) and does not generate new, complete codecs instead for the whole of T.

Example:

package xyz

type Bar struct {
  otherpackage.Foo `json:"foo"`
  x string
}

Reflection in golang does not make a difference between implemented or promoted funcs in the case of type embedding. Hence, it is not trivial to extend the test to check for the situation.

This issue bites us in https://github.com/kubernetes/kubernetes/pull/37557

[ugorji]

Unfortunately, golang reflection has that limitation as you alluded to, that you cannot (at runtime) determine whether a Type explicitly/directly (itself) implements an interface or whether it implements that interface implicitly (via embedding).

However, we can do a limited solution:

• only generate types if the type doesn't explicitly implement Selfer methods in the same file that we are running codecgen on.
• we can check this while doing the AST analysis for type names to generate
• in this mode, we don't check anymore at the top of Gen(...) to see if the reflect.Type implements codec.Selfer

This will handle the following:

• file1.go: defines type T, and CodecEncodeSelfer and CodecDecodeSelfer
• run codecgen on file1.go

However, it will NOT handle the following

• file1.go: defines type T
• file2.go: defines CodecEncodeSelfer and CodecDecodeSelfer for type T
• run codecgen on file1.go (without passing in file2.go also)

In practice, folks can easily work around this limitation.

Thoughts?

[ugorji]

@sttts ^^

[ugorji]

@sttts The comment above should help resolve the issue with kubernetes/kubernetes#37557

[sttts]

What you propose (using the ast) is pretty similar to my PoC https://github.com/ugorji/go/issues/186. I like the idea to move the ast code to the place where the type names are collected.

Why do you want to restrict the ast check to file1.go? Why not the whole package? At least in kube we remove the codecs of the entire package and re-generated them with one call to codecgen.

[ugorji]

Yes, it is similar to #186, and yes, first (initial) phase of codecgen is the only place where we inspect the AST to infer the types to codecgen on.

By design, an execution of codecgen only works on files within a specific directory (within a single package). It makes the problem easier to handle and solve.

This means that the following works also:

• file1.go: defines type T
• file2.go: defines CodecEncodeSelfer and CodecDecodeSelfer for type T
• run codecgen on file1.go AND file2.go (in one execution)

[ugorji]

@sttts ^^ (see reply above)

Fundamentally, what i am saying is that you can execute on the whole package by passing in all the files in the package in one go.

[sttts]

@ugorji thanks for implementing this immediately 👍