Enumerated Type Unique Identification String Key

[wenovus]

enumgen.go currently generates the unique ID incorrectly in a few cases:

• enumerated typedefs are identified using module-name + enum/identity name. This causes same-name enumerated values defined within the same module to be shadowed by each other.
• enumerated typedefs' module-name is incorrectly identified, and the user module is currently being used. This causes same-name enumerated values defined in different modules, but used within the same module, to shadow one another.
• enumeration leaves are identified using its path, but without the module name. This causes identically-pathed enumerations between different modules to shadow one another in the generated code.

When an instance of "shadowing one another in the generated code" occurs, it means only one of the conflicting enums is generated and used everywhere, and no error is emitted by the generator, and compilable, but incorrect code, is generated.

This issue can be solved by making the mapping of {enum value: unique ID} the one-to-one function that was originally intended.

[wenovus]

@robshakir The above-mentioned PRs address all of the issues with leaf, typedef and union combinations for enumerated types, with one exception:

The one exception is the case of a typedef union containing a non-typedef enumeration. In this case, the current naming behaviour is to treat it as if the typedef were inlined into the YANG tree where it's used, meaning it's not de-duped at all. If these enums are also named DefiningModule_TypedefName, then another 30+ usages will be deduped into as few as 3 enums, including

E_OpenconfigMplsTypes_MplsLabel_Enum
E_OpenconfigPacketMatchTypes_PortNumRange_Enum
E_OpenconfigSegmentRoutingTypes_SrSidType_Enum

At least 20 de-duplications are for mpls-label, the most popular of these.

Should we just keep inlining them? Or is it also useful to make this change as well, which would be hidden by the same flag that hides #400 ?

If we should change this as well, we should first answer whether #245 is something we're planning to support, because that will affect what name to give to these enums.

[robshakir]

Sorry @wenovus -- I'd missed this comment. Thanks for the detailed thought about this issue.

1. I think that we should de-duplicate these if we can, this seems like a good enhancement.
2. Let's make sure that this is covered by the same flag as #400
3. On #245 -- this is likely something that we should support, especially given that there are now occurrences of this within the schema. Further design discussion below:

In the case of the multiple enumerations in a union, I'm not entirely sure how we should name these things. A quick brainstorm of a few options:

leaf foo {
  type union {
    type enumeration { enum BLUE; }
    type enumeration { enum RED; }
  }
}

In this case, we could simply output one enumeration based on the path of the foo leaf. This would collapse all enumeration statements to one another, and output a single type named after the leaf's path. We'd need to have a per-leaf type because there could be multiple union leaves that use different enum combinations. We could maybe de-duplicate based on the node path to ensure that we don't have duplicates for a grouping used in multiple places, as with the existing implementation.

I think this (above) is the simplest case. There are also:

typedef containing union with multiple enumerations

typedef foo {
  type union {
     type enumeration { enum RED: }
     type enumeration { enum BLUE; }
  }
}

Typedef enumerations used in a leaf

typedef fooenum {
   type enumeration { enum RED; }
}

typedef barenum {
  type enumeration { enum BLUE; }
}

leaf a {
  type union {
    type fooenum;
    type barenum;
  }
}

Typedef enumerations used in a typedef union

typedef fooenum {
   type enumeration { enum RED; }
}

typedef barenum {
  type enumeration { enum BLUE; }
}

typedef foo {
  type union {
     type fooenum;
     type barenum;
  }
}

In the latter two cases, the schema could be referencing the typedef'd enumeration leaves individually, so we'd need to think about whether the trick of just collapsing all the enumeration types within a particular union would be the best user experience here, or whether it is best to generate these, and then have them separately be used within the interface that we generate for the union.

It sounds like we probably need to write a quick design document here to discuss the options for these. Do you agree?

If so, is this PR going to be blocked by fixing #245? I didn't quite understand why it would be - but I didn't spend a lot of time thinking about it.

[wenovus]

Thanks @robshakir I see now, I agree #245 is not blocking this PR, so I will continue with it.

On the question of dealing with #245, I see two reasonable approaches:

First of all, just clarifying that the issue doesn't exist for identityrefs -- ygot seems to be handling the case of multiple identityrefs and identityref+enum correctly. So, the problem arises only when there is more than 1 enumeration type in the union.

1. Merge all the enums within the union, including all the typedef enums. The usability advantage is that user will always need to deal with only a single enum type for every union leaf.
2. Merge all unnamed enums within the union, but don't merge the typedef enums. Treat them as if they're a completely different type. The usability advantages here are that 1) enum values are de-duped within the same defined type (e.g. OpenconfigMplsTypes_MplsLabel_Enum_NO_LABEL within OpenconfigMplsTypes_MplsLabel_Enum), and the de-duped type may make more sense to the user than PrefixSid_SidId_NO_LABEL and RecordRouteObject_ReportedLabel_NO_LABEL.

Possibly orthogonal to this is how to handle the case where two enumerations have the same enum name within their definitions. This is problematic for 2., since if we won't error out in the case of a conflict involving typedef'ed enums, we're still allowing users to make an error by specifying a value that will never be deserialized on the other side. This doesn't happen in practice within OpenConfig, so I don't know whether this is something that's important enough to tip the balance between approaches 1 and 2.

---

The only example in the OpenConfig models where more than 1 enumeration type exists within the union is what's identified in #245 (I just checked with the latest models). This is the current output (YANG copied here for convenience):

leaf sid-id {
      type union {
        type oc-srt:sr-sid-type;
        type enumeration {
            enum DYNAMIC {
              description
                "The SID chosen for the Adjacency SID should be dynamically
                allocated from the system's dynamic range of Segment
                Identifiers. For MPLS, this range should be considered to be
                those labels that do not fall within a reserved label block.";
            }
        }
      }
}

typedef sr-sid-type {
    type union {
      type oc-mplst:mpls-label;
      type oc-inet:ipv6-address;
    }
    description
      "The defined value of a segment identifier.";
}

const (
        // PrefixSid_SidId_UNSET corresponds to the value UNSET of PrefixSid_SidId
        PrefixSid_SidId_UNSET E_PrefixSid_SidId = 0
        // PrefixSid_SidId_IPV4_EXPLICIT_NULL corresponds to the value IPV4_EXPLICIT_NULL of PrefixSid_SidId
        PrefixSid_SidId_IPV4_EXPLICIT_NULL E_PrefixSid_SidId = 1
        // PrefixSid_SidId_ROUTER_ALERT corresponds to the value ROUTER_ALERT of PrefixSid_SidId
        PrefixSid_SidId_ROUTER_ALERT E_PrefixSid_SidId = 2
        // PrefixSid_SidId_IPV6_EXPLICIT_NULL corresponds to the value IPV6_EXPLICIT_NULL of PrefixSid_SidId
        PrefixSid_SidId_IPV6_EXPLICIT_NULL E_PrefixSid_SidId = 3
        // PrefixSid_SidId_IMPLICIT_NULL corresponds to the value IMPLICIT_NULL of PrefixSid_SidId
        PrefixSid_SidId_IMPLICIT_NULL E_PrefixSid_SidId = 4
        // PrefixSid_SidId_ENTROPY_LABEL_INDICATOR corresponds to the value ENTROPY_LABEL_INDICATOR of PrefixSid_SidId
        PrefixSid_SidId_ENTROPY_LABEL_INDICATOR E_PrefixSid_SidId = 8
        // PrefixSid_SidId_NO_LABEL corresponds to the value NO_LABEL of PrefixSid_SidId
        PrefixSid_SidId_NO_LABEL E_PrefixSid_SidId = 9
)

As mentioned in #245 the DYNAMIC value is missing.

Approach 1: Simply have PrefixSid_SidId_DYNAMIC inserted.

Approach 2: Define only the enum value for DYNAMIC, and reuse E_OpenconfigMplsTypes_MplsLabel_Enum:

const (
        // PrefixSid_SidId_UNSET corresponds to the value UNSET of PrefixSid_SidId
        PrefixSid_SidId_UNSET E_PrefixSid_SidId = 0
        // PrefixSid_SidId_DYNAMIC corresponds to the value DYNAMIC of PrefixSid_SidId
        PrefixSid_SidId_DYNAMIC E_PrefixSid_SidId = 1
)

... intervening code ...

        switch v := i.(type) {
        case E_PrefixSid_SidId:
                return &NetworkInstance_Protocol_Isis_Interface_Level_Af_SegmentRouting_PrefixSid_SidId_Union_E_PrefixSid_SidId{v}, nil
        case E_OpenconfigMplsTypes_MplsLabel_Enum:
                return &NetworkInstance_Protocol_Isis_Interface_Level_Af_SegmentRouting_PrefixSid_SidId_Union_E_OpenconfigMplsTypes_MplsLabel_Enum{v}, nil
        case string:
                return &NetworkInstance_Protocol_Isis_Interface_Level_Af_SegmentRouting_PrefixSid_SidId_Union_String{v}, nil
        case uint32:
                return &NetworkInstance_Protocol_Isis_Interface_Level_Af_SegmentRouting_PrefixSid_SidId_Union_Uint32{v}, nil
        default:
                return nil, fmt.Errorf("cannot convert %v to NetworkInstance_Protocol_Isis_Interface_Level_Af_SegmentRouting_PrefixSid_SidId_Union, unknown union type, got: %T, want any of [E_PrefixSid_SidId, string, uint32]", i, i)
        }

[wenovus]

The readability/usability advantage for approach 2 is very well illustrated with the sid-id example above (which exists in the current models). Namely, that since oc-mplst:mpls-label is actually used directly in many places, it's intuitive for the user to be able to use OpenconfigMplsTypes_MplsLabel_Enum to directly populate the value for sid-id, instead of using its own duplicated mpls-label enumeration values.

[robshakir]

We decided to use approach 2, since the merging approach negatively impacts comprehension by users of the schema. :-) During this conversation we noted that there will be an interim phase whereby merging #400 causes us to have something that wasn't assignable in the existing ygen-generated structs become assignable, and that might change - however, approach 2 here would likely not break the new assignability.