Type unification always fails with embedded structs in a resource due to field naming contradiction

[frebib]

Versions:

• mgmt version (eg: mgmt --version): 01c2131 (master HEAD)
• operating system/distribution (eg: uname -a): Arch
• golang version (eg: go version): 1.15.6

Description:

This bug (oversight? I'm not sure) was actually already discovered 2 years ago #432 (comment) in a similar issue that may actually be the same root problem as this. The error thrown is

could not unify types: can't unify, invariant illogicality with equals: struct fields differ

Using the example from that comment we can see that the resource struct has a struct child, which in theory mcl should support just fine:

type IPRange struct {
	From string `lang:"from" yaml:"from"`
	To   string `lang:"to" yaml:"to"`
}

// DHCPDRes is a dhcp daemon resource.
type DHCPDRes struct {
	traits.Base // add the base methods without re-implementation
	traits.Groupable
	traits.Refreshable

	init *engine.Init

	// Range defines the upper and lower limits for ip address assignments.
	Range IPRange `lang:"range" yaml:"range"`
	...

with some matching mcl ala

dhcpd "foo" {
    range => struct{
        from => "192.168.42.100/24",
        to => "192.168.42.200/24",
    },
...

logic would dictate that this should work fine. The structs are tagged with the lang tag to indicate that the mcl fields should be matched to the struct field with the same tag: mcl range.from should match to the Go field Range.From. This is exactly how it works for resource struct fields. Even the lang struct tags are superfluous because the "default" behaviour is to use the struct field name in lowercase. That is the expected behavior and if it's not already explicitly documented, it probably should be.

The lang tags are the first clue as to what the issue is here. Looking at the code, we'd expect to find something doing roughly:

list fields in the go struct
enumerate their "mcl" struct field names
match the mcl fields to the go struct fields according to the name mappings
happy days

What the code actually does, is convert the Go reflect.Type for the struct into a mgmt type.Type with TypeOf

mgmt/lang/types/type.go

Lines 137 to 157 in 01c2131

	case reflect.Struct:
	m := make(map[string]*Type)
	ord := []string{}
	for i := 0; i < typ.NumField(); i++ {
	field := typ.Field(i)
	tt, err := TypeOf(field.Type)
	if err != nil {
	return nil, err
	}
	// TODO: should we skip over fields with field.Anonymous ?
	m[field.Name] = tt
	ord = append(ord, field.Name) // in order
	// NOTE: we discard the field.Tag data
	}
	return &Type{
	Kind: KindStruct,
	Map: m,
	Ord: ord,
	}, nil

and compare that new type.Type struct against the struct parsed from the mcl. Note there is no "conversion" in there of Go struct field names to mcl field names: they are simply copied verbatim. This entirely ignores the lang tag on fields if it's provided, and the implicit "lowercase by default" behaviour is skipped here too.

Herein lies the first issue. Looking forward a little, the "converted" Go struct into this lang representation with uppercase named fields is compared directly with the mcl parsed struct during the unification phase, which is a logical contradiction. The two structs with fields of differing names will never match. The naive answer to this would be to just make the Go struct use lowercase names to match the mcl struct, so then they will match, but of course this isn't the correct answer either because lowercase field names in Go are unexported, therefore cannot be reflected. With these constraints understood it's clear that the current "take structs at face value" approach can never work.

Now, of course, we could just fix the code to convert Go structs to take the name conversion into account. A tiny change such as this is enough to convert uppercase Go struct field names to lowercase according to the lang tag, which actually makes the above example work:

diff --git a/lang/types/type.go b/lang/types/type.go
index e51bafb..84662b3 100644
--- a/lang/types/type.go
+++ b/lang/types/type.go
@@ -145,8 +145,12 @@ func TypeOf(t reflect.Type) (*Type, error) {
                                return nil, err
                        }
                        // TODO: should we skip over fields with field.Anonymous ?
-                       m[field.Name] = tt
-                       ord = append(ord, field.Name) // in order
+                       fieldName := field.Name
+                       if alias, ok := field.Tag.Lookup("lang"); ok {
+                               fieldName = alias
+                       }
+                       m[fieldName] = tt
+                       ord = append(ord, fieldName) // in order
                        // NOTE: we discard the field.Tag data
                }

but this isn't all ☀️ and 🌈 either. It does make the type unification succeed because the struct field names match now, but a little while after starting mgmt, it panics because type.Reflect is called on a struct with unexported fields here:

mgmt/lang/types/type.go

Lines 742 to 767 in 01c2131

	case KindStruct: // {a bool; b int}
	if obj.Map == nil {
	panic("malformed struct type")
	}
	if len(obj.Map) != len(obj.Ord) {
	panic("malformed struct length")
	}
	fields := []reflect.StructField{}
	for _, k := range obj.Ord {
	t, ok := obj.Map[k]
	if !ok {
	panic("malformed struct order")
	}
	if t == nil {
	panic("malformed struct field")
	}
	fields = append(fields, reflect.StructField{
	Name: k, // struct field name
	Type: t.Reflect(),
	//Tag: `mgmt:"foo"`, // unused
	})
	}
	return reflect.StructOf(fields)

#432 was raised for exactly this problem, and I think only got half way to discovering what the underlying issues are.

I do find this situation a bit odd though, because this behaviour isn't present for the struct that represents the resource. Both name conversions (implicit and explicit) work fine between the Go struct and the mcl resource definition. This indicates that the codepaths for this behaviour are different and it doesn't use the general case struct code, which seems to be the case

mgmt/lang/structs.go

Line 612 in 01c2131

mapping, err := engineUtil.LangFieldNameToStructFieldName(obj.Kind)

Should the special-casing be removed and this code be unified with the struct handling code? idk

So some takeaways from this that I have gleaned

• There is information loss when converting Go (tagged) structs to mcl structs - the tags are both ignored, and then discarded
  • Conversion back the other way is even worse, because the tags that were present in the Go struct before conversion are no longer present, so the reverse conversion produces the wrong struct entirely
• The lang tags on struct fields are ignored sometimes but they probably shouldn't be
• Rules for uppercase/lowercase/exported/unexported field name conversions are not well defined when converting/comparing structs of different origins. (Should the names always be honored? Probably)
• The extra tag information should probably be tracked in the types.Type data so it's available for consistent bi-directional conversions

My gut reaction for how to tackle this is probably to track the additional Go field names in the type.Type data structure. The field name in that type should always be the lowercase mcl variant, but then additionally (in Ord or in another field) the Go field name should also be stored, because it will probably never match. I do have concerns about inflating the memory usage of that struct though, so this would have to be a carefully considered change.

A side-note to this is I've noticed this comment, which seems to be a good idea, and perhaps this is the appropriate time to rewrite this system to use an interface and sub-types instead of the monolithic Type struct. (A union here would be perfect)

mgmt/lang/types/type.go

Line 58 in 01c2131

// TODO: should we create a `Type` interface?

[purpleidea]

We talked a bit about this on IRC... I think this is blocked on golang/go#25401 ... So:

1. I don't want to dig down into this myself until 24501 is fixed.
2. But eventually I might have to do so and either fix that upstream or make a workaround here
3. And I'd probably want a simple failing test case as a PR before I started
4. That last one is easy to do, but I'm focused on other areas at the moment, so this issue is free if someone is interested in taking it.

// TODO: should we create a Type interface?

I should probably remove this comment, but maybe it's fine to leave in to at least provoke thought in future passers by?

[frebib]

I'm happy to work on this. It's something I want to rely on in a resource that I'm working on, and I think fixing/changing this behavior will appear more intuitive and consistent in the long run.
I've been trying to wrap my head around how this works, and this is where I'm at currently:

• There are two things to do here IMO:
  • Wait until reflect: permit unexported fields in StructOf golang/go#25401 is fixed, then write test cases & validate that unexported struct fields work as intended
  • Rework type matching to allow matching mcl fields to tagged (and exported) Go fields

I will submit a PR for a failing test case, then attempt to actually make the field mapping work, which is proving rather difficult, unsurprisingly.

The TODO causes no harm as it is, but also it would be good to clean through the old comments and prune anything inaccurate or resolved as an act of declaring stability.

[frebib]

After thoroughly straining every neuron in my brain trying to trace through the several layers of the code, I think I understand the variable journey from mcl to resource now.
One thing I'm reasonably sure about is this only affects specifically structs inside a resource, or nested within a resource eventually through other structs, maps, lists etc.

Fixing the primary part of this is simple, by tracking the name of the Golang fields, the variables can be mapped when matching the structs up. The code change for that part is pretty simple (https://gist.github.com/frebib/f5409ff4719a465489c6ec464eefc66c/raw, with some cleanup; I'm still hacking at this)

Now the part I'm stuck at wasn't apparent to me at first, but makes sense: above takes the data from the mcl struct into the intermediate AST/graph form. We haven't covered converting it back into the resource yet.
Values in the Resource struct are set here:

mgmt/lang/structs.go

Line 729 in 01c2131

f.Set(value) // set it !

but are set at the top-level in the struct, i.e. there is no recursion through the structs, maps etc to resolve anything.

An example to explain ^ better:

type Res {
    X struct{ SomeName string }
}

value := struct{ somename string }{
    somename: "this field will not appear",
}
...
// will succeed but SomeName will be an empty/default value
x.FieldByName("X").Set(value)

The problem is that we have the data in the value but the struct fields have the incorrect name, so are unexported/missing/unmatched/something? Anyway, the data is just absent in the Golang struct because the names don't match even though it's passed all the way through the graph engine just fine.

The fix for this is to map the field names in ExprStruct.Value(), the inverse lookup used to map from from the mcl:

mgmt/lang/structs.go

Lines 6537 to 6558 in 01c2131

	// Value returns the value of this expression in our type system. This will
	// usually only be valid once the engine has run and values have been produced.
	// This might get called speculatively (early) during unification to learn more.
	func (obj *ExprStruct) Value() (types.Value, error) {
	fields := make(map[string]types.Value)
	typ := &types.Type{
	Kind: types.KindStruct,
	Map: make(map[string]*types.Type),
	//Ord: obj.typ.Ord, // assume same order
	}
	ord := []string{} // can't use obj.typ b/c it can be nil during speculation
	for i, x := range obj.Fields {
	// vals
	t, err := x.Value.Type()
	if err != nil {
	return nil, errwrap.Wrapf(err, "field val, index `%d` did not return a type", i)
	}
	if _, exists := typ.Map[x.Name]; exists {
	return nil, fmt.Errorf("struct type field index `%d` already exists", i)
	}
	typ.Map[x.Name] = t

I can come up with two solutions, neither of which are straight forward:

1. During the type unification stage, when the mcl struct is matched to the Golang struct, we write the field name mappings into the type.Type representation to be stored and used in ExprStruct.Value(). This is a bit nasty because the unification feels like a read-only stage to me.
2. The f.Set(value) line is rewritten into a recursive descent set of functions in the lang/types package that sets Golang types from lang types.

I'm not which option I prefer. Numero 2 would be cleaner, but is also a larger change, and possibly slower to execute too

[purpleidea]

There's a lot to discuss here, and TBH, I think we might be near the limit of how efficient we can be over text... (Of course please do attempt to send appropriate patches and tests that I can merge if you like...)

However, if it would help, we can do a gmeet session on the weekend and try and live hack this out if you'd like. It would likely take at least an hour I think because I've not looked at this code recently, but if that's helpful to you, lmk...

The TODO causes no harm as it is, but also it would be good to clean through the old comments and prune anything inaccurate or resolved as an act of declaring stability.

I don't disagree... Kindly remember that when maintaining a big complex project with no budget you heavily prioritize what you work on and aren't always happy with everything.

The code change for that part is pretty simple (https://gist.github.com/frebib/f5409ff4719a465489c6ec464eefc66c/raw, with some cleanup; I'm still hacking at this)

I think I'm roughly okay with this sort of thing modulo some harsh style nitpicking ;) -- This lib is used everywhere so I need to be careful too.

f.Set(value) // set it !

This relies on the core golang reflect magic.

There may be situations where a more complex, verbose implementation could replace something I've taken a short-cut on. I can't guarantee this, but I think it's likely. Set may be part of it... Also some of my methods in the types package are implemented as "hacks"... You should know what I mean if you have a deeper look.

HTH

[purpleidea]

Seems this is fixed with your recent patches! Thanks =D