Support explicitly setting field tags

[jasonewang]

The YANG to Protobuf specification mentions using extensions to explicitly set
the field tag of generated Protobuf fields. Add support for annotating field
tags.

• Support setting the field number with occodegenext:field-number.

• Support offsetting the field number with occodegenext:field-number-offset.

• Reject field number annotations that result in a value in the Protobuf
  reserved range.

[coveralls]

Coverage decreased (-0.03%) to 90.948% when pulling 4664808 on jasonewang:support-field-number-gen into ef8f2a1 on openconfig:master.

[wenovus]

Thanks for helping to meet the specification, looks like this implementation was missing.

Could you also add an integration test by modifying one of the yang files used as a test for TestGenerateProto3 within codegen_test.go?

[jasonewang]

Added an integration test and changed the code to only allow annotated field numbers in range 1-1000.

[wenovus]

Hi Jason, thanks for your changes. It seems that ygot doesn't have good support for extensions (no helpers), and as a result I see two immediate concerns (due to this ygot limitation). I've listed them below and some thoughts on how to address them. I'd like to answer these concerns before this change is merged. Would be happy to hear your thoughts on it.

1. The module prefix used by the extension is essentially part of the name -- there is no cross-checking that the extension actually exists in the imported module, or even that the module corresponding to the extension prefix matches (e.g. oc-ext -> openconfig-extensions).
2. There are edge cases in the extension usages that goyang/ygot needs to choose whether to handle:
   • duplicate extensions (goyang records both instead of throwing an error)
   • extension conflicts due to semantics, e.g. overlapping proto field numbers -- for this, maybe allow it and let the generated code error out, or maybe throw an error.

Thoughts on solutions:

1. goyang's FindModuleByPrefix can verify the module name of the extension (using its prefix), which partially solves the problem.
2. We'll need to choose what to do for each edge case.
3. In any case, however, we might need to actually define an extension in a YANG file for field-number and field-number-offset.

[jasonewang]

Hi Wen,

The proposed solutions make sense to me. My take on the edge cases

• duplicate extensions: I don't have enough context to know if duplicate extensions should ever be allowed.
• conflicts due to semantics: Seems more user friendly for the ygot to throw an error if some combination of extensions doesn't work.

[wenovus]

TLDR: In any case, I agree that we should add both duplicate-check and overlap-check functionality into proto generation, if you don't mind. Then, assuming the extensions get pushed (and there is no objections right now), goyang's FindModuleByPrefix should also be used to verify the name of the extension module before processing these extensions. Thanks!

Thanks Jason. It's a good point that it's hard to rule out allowing duplicate extensions in general. However, following the idea that ygot should throw an error if the extension's sematics is invalid, then at least for this extension it should also error out on duplicates. It totally makes sense to me that if there is an error, that ygot should be able to catch it before it causes any trouble, as ygot is responsible for generating usable proto in the first place, and has already assumed understanding of everything about proto generation in order to always generate valid and usable protos.

[jasonewang]

On further thought, should we allow multiple field-number-offset extensions? I'm thinking of the case where there are multiple nested groupings and each grouping applies a field-number-offset extension. An entity may end up having multiple offsets applied to it. It seems reasonable that all offsets would be added to the field number since the offset is applied from a level up.

Erroring on getting multiple field-number extensions sounds like a good idea.

[wenovus]

Good eye, nested uses statements definitely makes sense as a use-case to me. Added openconfig/goyang#113 to test extensions for nested use statements.

List of Error Checks:

• duplicate field-number extensions.
• field-number extensions on non-leaves (or just ignore?).
• field-number-offset on non-uses statements (or just ignore?).
• overlapping ranges within a directory entry (i.e. a container/list).
• (once the extensions are added) use yang.FindModuleByPrefix to verify extension module name ("openconfig-extensions", "openconfig-codegen-extensions" or otherwise).

[wenovus]

@jasonewang btw I'm working on getting your changes in.

[wenovus]

I have the change ready now, though it's dependent on openconfig/goyang#183.

[wenovus]

@jasonewang Do you think you could consent again at #549? Thanks. For some reason googlebot needs another consent if the PR submitter changed.

[wenovus]

@robshakir I realized I can just push to Jason's fork. When you have time could you take a look? changes at ygen/protogen.go