Merge pull request #113 from jhump/jh/add-support-for-enum-reserved-r…

…anges

add support for enum reserved ranges

Makefile

@@ -77,7 +77,7 @@ generate:
 
 .PHONY: testcover
 testcover:
-	@echo go test -covermode=atomic ./... 
+	@echo go test -race -covermode=atomic ./...
 	@echo "mode: atomic" > coverage.out
 	@for dir in $$(go list ./...); do \
 		go test -race -coverprofile profile.out -covermode=atomic $$dir ; \

desc/builder/builders.go

@@ -1593,8 +1593,9 @@ func (mb *MessageBuilder) AddReservedRange(start, end int32) *MessageBuilder {
 // SetReservedRanges replaces all of this message's reserved ranges with the
 // given slice of ranges. Unlike AddReservedRange and unlike the way ranges are
 // defined in proto IDL source, a DescriptorProto_ReservedRange struct treats
-// the end of the range as *exclusive*. So the range is inclusive of the start
-// but exclusive of the end. This returns the message, for method chaining.
+// the end of the range as *exclusive* (so it would be the value defined in the
+// IDL plus one). So the range is inclusive of the start but exclusive of the
+// end. This returns the message, for method chaining.
 func (mb *MessageBuilder) SetReservedRanges(ranges []*dpb.DescriptorProto_ReservedRange) *MessageBuilder {
 	mb.ReservedRanges = ranges
 	return mb
@@ -2366,7 +2367,9 @@ func (oob *OneOfBuilder) Build() (*desc.OneOfDescriptor, error) {
 type EnumBuilder struct {
 	baseBuilder
 
-	Options *dpb.EnumOptions
+	Options        *dpb.EnumOptions
+	ReservedRanges []*dpb.EnumDescriptorProto_EnumReservedRange
+	ReservedNames  []string
 
 	values  []*EnumValueBuilder
 	symbols map[string]*EnumValueBuilder
@@ -2392,6 +2395,8 @@ func FromEnum(ed *desc.EnumDescriptor) (*EnumBuilder, error) {
 func fromEnum(ed *desc.EnumDescriptor, localEnums map[*desc.EnumDescriptor]*EnumBuilder) (*EnumBuilder, error) {
 	eb := NewEnum(ed.GetName())
 	eb.Options = ed.GetEnumOptions()
+	eb.ReservedRanges = ed.AsEnumDescriptorProto().GetReservedRange()
+	eb.ReservedNames = ed.AsEnumDescriptorProto().GetReservedName()
 	setComments(&eb.comments, ed.GetSourceInfo())
 
 	localEnums[ed] = eb
@@ -2503,6 +2508,39 @@ func (eb *EnumBuilder) TryAddValue(evb *EnumValueBuilder) error {
 	return nil
 }
 
+// AddReservedRange adds the given reserved range to this message. The range is
+// inclusive of both the start and end, just like defining a range in proto IDL
+// source. This returns the message, for method chaining.
+func (eb *EnumBuilder) AddReservedRange(start, end int32) *EnumBuilder {
+	rr := &dpb.EnumDescriptorProto_EnumReservedRange{
+		Start: proto.Int32(start),
+		End:   proto.Int32(end),
+	}
+	eb.ReservedRanges = append(eb.ReservedRanges, rr)
+	return eb
+}
+
+// SetReservedRanges replaces all of this enum's reserved ranges with the
+// given slice of ranges. This returns the enum, for method chaining.
+func (eb *EnumBuilder) SetReservedRanges(ranges []*dpb.EnumDescriptorProto_EnumReservedRange) *EnumBuilder {
+	eb.ReservedRanges = ranges
+	return eb
+}
+
+// AddReservedName adds the given name to the list of reserved value names for
+// this enum. This returns the enum, for method chaining.
+func (eb *EnumBuilder) AddReservedName(name string) *EnumBuilder {
+	eb.ReservedNames = append(eb.ReservedNames, name)
+	return eb
+}
+
+// SetReservedNames replaces all of this enum's reserved value names with the
+// given slice of names. This returns the enum, for method chaining.
+func (eb *EnumBuilder) SetReservedNames(names []string) *EnumBuilder {
+	eb.ReservedNames = names
+	return eb
+}
+
 func (eb *EnumBuilder) buildProto(path []int32, sourceInfo *dpb.SourceCodeInfo) (*dpb.EnumDescriptorProto, error) {
 	addCommentsTo(sourceInfo, path, &eb.comments)
 
@@ -2548,9 +2586,11 @@ func (eb *EnumBuilder) buildProto(path []int32, sourceInfo *dpb.SourceCodeInfo)
 	}
 
 	return &dpb.EnumDescriptorProto{
-		Name:    proto.String(eb.name),
-		Options: eb.Options,
-		Value:   values,
+		Name:          proto.String(eb.name),
+		Options:       eb.Options,
+		Value:         values,
+		ReservedRange: eb.ReservedRanges,
+		ReservedName:  eb.ReservedNames,
 	}, nil
 }
 

desc/internal/util.go

@@ -52,6 +52,8 @@ const (
 	Enum_nameTag              = 1
 	Enum_valuesTag            = 2
 	Enum_optionsTag           = 3
+	Enum_reservedRangeTag     = 4
+	Enum_reservedNameTag      = 5
 	EnumVal_nameTag           = 1
 	EnumVal_numberTag         = 2
 	EnumVal_optionsTag        = 3

desc/protoparse/ast.go

@@ -711,15 +711,16 @@ type extensionRangeNode struct {
 
 type rangeNode struct {
 	basicCompositeNode
-	st, en *intLiteralNode
+	stNode, enNode node
+	st, en         int32
 }
 
 func (n *rangeNode) rangeStart() node {
-	return n.st
+	return n.stNode
 }
 
 func (n *rangeNode) rangeEnd() node {
-	return n.en
+	return n.enNode
 }
 
 type reservedNode struct {
@@ -732,13 +733,21 @@ type enumNode struct {
 	basicCompositeNode
 	name  *identNode
 	decls []*enumElement
+
+	// This field is populated after parsing, to make it easier to find them
+	// without searching decls. The parse result has a map of descriptors to
+	// nodes which makes the other declarations easily discoverable. But these
+	// elements do not map to descriptors -- they are just stored as strings in
+	// the message descriptor.
+	reserved []*stringLiteralNode
 }
 
 type enumElement struct {
 	// a discriminated union: only one field will be set
-	option *optionNode
-	value  *enumValueNode
-	empty  *basicNode
+	option   *optionNode
+	value    *enumValueNode
+	reserved *reservedNode
+	empty    *basicNode
 }
 
 func (n *enumElement) start() *SourcePos {

desc/protoparse/parser.go

@@ -367,7 +367,7 @@ func (r *parseResult) getExtensionRangeNode(e *dpb.DescriptorProto_ExtensionRang
 	return r.nodes[e].(rangeDecl)
 }
 
-func (r *parseResult) getReservedRangeNode(rr *dpb.DescriptorProto_ReservedRange) rangeDecl {
+func (r *parseResult) getMessageReservedRangeNode(rr *dpb.DescriptorProto_ReservedRange) rangeDecl {
 	if r.nodes == nil {
 		return noSourceNode{pos: unknownPos(r.fd.GetName())}
 	}
@@ -388,6 +388,13 @@ func (r *parseResult) getEnumValueNode(e *dpb.EnumValueDescriptorProto) enumValu
 	return r.nodes[e].(enumValueDecl)
 }
 
+func (r *parseResult) getEnumReservedRangeNode(rr *dpb.EnumDescriptorProto_EnumReservedRange) rangeDecl {
+	if r.nodes == nil {
+		return noSourceNode{pos: unknownPos(r.fd.GetName())}
+	}
+	return r.nodes[rr].(rangeDecl)
+}
+
 func (r *parseResult) getServiceNode(s *dpb.ServiceDescriptorProto) node {
 	if r.nodes == nil {
 		return noSourceNode{pos: unknownPos(r.fd.GetName())}
@@ -430,7 +437,7 @@ func (r *parseResult) putExtensionRangeNode(e *dpb.DescriptorProto_ExtensionRang
 	r.nodes[e] = n
 }
 
-func (r *parseResult) putReservedRangeNode(rr *dpb.DescriptorProto_ReservedRange, n *rangeNode) {
+func (r *parseResult) putMessageReservedRangeNode(rr *dpb.DescriptorProto_ReservedRange, n *rangeNode) {
 	r.nodes[rr] = n
 }
 
@@ -442,6 +449,10 @@ func (r *parseResult) putEnumValueNode(e *dpb.EnumValueDescriptorProto, n *enumV
 	r.nodes[e] = n
 }
 
+func (r *parseResult) putEnumReservedRangeNode(rr *dpb.EnumDescriptorProto_EnumReservedRange, n *rangeNode) {
+	r.nodes[rr] = n
+}
+
 func (r *parseResult) putServiceNode(s *dpb.ServiceDescriptorProto, n *serviceNode) {
 	r.nodes[s] = n
 }
@@ -714,8 +725,8 @@ func (r *parseResult) asExtensionRanges(node *extensionRangeNode) []*dpb.Descrip
 	ers := make([]*dpb.DescriptorProto_ExtensionRange, len(node.ranges))
 	for i, rng := range node.ranges {
 		er := &dpb.DescriptorProto_ExtensionRange{
-			Start: proto.Int32(int32(rng.st.val)),
-			End:   proto.Int32(int32(rng.en.val + 1)),
+			Start: proto.Int32(rng.st),
+			End:   proto.Int32(rng.en + 1),
 		}
 		if len(opts) > 0 {
 			er.Options = &dpb.ExtensionRangeOptions{UninterpretedOption: opts}
@@ -771,11 +782,28 @@ func (r *parseResult) asEnumDescriptor(en *enumNode) *dpb.EnumDescriptorProto {
 			ed.Options.UninterpretedOption = append(ed.Options.UninterpretedOption, r.asUninterpretedOption(decl.option))
 		} else if decl.value != nil {
 			ed.Value = append(ed.Value, r.asEnumValue(decl.value))
+		} else if decl.reserved != nil {
+			for _, n := range decl.reserved.names {
+				en.reserved = append(en.reserved, n)
+				ed.ReservedName = append(ed.ReservedName, n.val)
+			}
+			for _, rng := range decl.reserved.ranges {
+				ed.ReservedRange = append(ed.ReservedRange, r.asEnumReservedRange(rng))
+			}
 		}
 	}
 	return ed
 }
 
+func (r *parseResult) asEnumReservedRange(rng *rangeNode) *dpb.EnumDescriptorProto_EnumReservedRange {
+	rr := &dpb.EnumDescriptorProto_EnumReservedRange{
+		Start: proto.Int32(rng.st),
+		End:   proto.Int32(rng.en),
+	}
+	r.putEnumReservedRangeNode(rr, rng)
+	return rr
+}
+
 func (r *parseResult) asMessageDescriptor(node *messageNode, isProto3 bool) *dpb.DescriptorProto {
 	msgd := &dpb.DescriptorProto{Name: proto.String(node.name.val)}
 	r.putMessageNode(msgd, node)
@@ -831,18 +859,18 @@ func (r *parseResult) addMessageDecls(msgd *dpb.DescriptorProto, reservedNames *
 				msgd.ReservedName = append(msgd.ReservedName, n.val)
 			}
 			for _, rng := range decl.reserved.ranges {
-				msgd.ReservedRange = append(msgd.ReservedRange, r.asReservedRange(rng))
+				msgd.ReservedRange = append(msgd.ReservedRange, r.asMessageReservedRange(rng))
 			}
 		}
 	}
 }
 
-func (r *parseResult) asReservedRange(rng *rangeNode) *dpb.DescriptorProto_ReservedRange {
+func (r *parseResult) asMessageReservedRange(rng *rangeNode) *dpb.DescriptorProto_ReservedRange {
 	rr := &dpb.DescriptorProto_ReservedRange{
-		Start: proto.Int32(int32(rng.st.val)),
-		End:   proto.Int32(int32(rng.en.val + 1)),
+		Start: proto.Int32(rng.st),
+		End:   proto.Int32(rng.en + 1),
 	}
-	r.putReservedRangeNode(rr, rng)
+	r.putMessageReservedRangeNode(rr, rng)
 	return rr
 }
 
@@ -942,7 +970,6 @@ func (r *parseResult) generateSourceCodeInfo() *dpb.SourceCodeInfo {
 			}, mtd.Options.GetUninterpretedOption(), append(mtdPath, internal.Method_optionsTag))
 		}
 	}
-
 	return &dpb.SourceCodeInfo{Location: sci.locs}
 }
 
@@ -1068,9 +1095,9 @@ func (r *parseResult) generateSourceCodeInfoForMessage(sci *sourceCodeInfo, msg
 		rangePath := append(path, internal.Message_extensionRangeTag, int32(i))
 		rn := r.getExtensionRangeNode(er).(*rangeNode)
 		sci.newLoc(rn, rangePath)
-		sci.newLoc(rn.st, append(rangePath, internal.ExtensionRange_startTag))
-		if rn.st != rn.en {
-			sci.newLoc(rn.en, append(rangePath, internal.ExtensionRange_endTag))
+		sci.newLoc(rn.stNode, append(rangePath, internal.ExtensionRange_startTag))
+		if rn.stNode != rn.enNode {
+			sci.newLoc(rn.enNode, append(rangePath, internal.ExtensionRange_endTag))
 		}
 		// now we have to find the extension decl and options that correspond to this range :(
 		for _, d := range decls {
@@ -1095,11 +1122,11 @@ func (r *parseResult) generateSourceCodeInfoForMessage(sci *sourceCodeInfo, msg
 	// reserved ranges
 	for i, rr := range msg.ReservedRange {
 		rangePath := append(path, internal.Message_reservedRangeTag, int32(i))
-		rn := r.getReservedRangeNode(rr).(*rangeNode)
+		rn := r.getMessageReservedRangeNode(rr).(*rangeNode)
 		sci.newLoc(rn, rangePath)
-		sci.newLoc(rn.st, append(rangePath, internal.ReservedRange_startTag))
-		if rn.st != rn.en {
-			sci.newLoc(rn.en, append(rangePath, internal.ReservedRange_endTag))
+		sci.newLoc(rn.stNode, append(rangePath, internal.ReservedRange_startTag))
+		if rn.stNode != rn.enNode {
+			sci.newLoc(rn.enNode, append(rangePath, internal.ReservedRange_endTag))
 		}
 	}
 
@@ -1132,6 +1159,22 @@ func (r *parseResult) generateSourceCodeInfoForEnum(sci *sourceCodeInfo, enum *d
 			return n.(*optionNode)
 		}, ev.Options.GetUninterpretedOption(), append(evPath, internal.EnumVal_optionsTag))
 	}
+
+	// reserved ranges
+	for i, rr := range enum.GetReservedRange() {
+		rangePath := append(path, internal.Enum_reservedRangeTag, int32(i))
+		rn := r.getEnumReservedRangeNode(rr).(*rangeNode)
+		sci.newLoc(rn, rangePath)
+		sci.newLoc(rn.stNode, append(rangePath, internal.ReservedRange_startTag))
+		if rn.stNode != rn.enNode {
+			sci.newLoc(rn.enNode, append(rangePath, internal.ReservedRange_endTag))
+		}
+	}
+
+	// reserved names
+	for i, rn := range n.reserved {
+		sci.newLoc(rn, append(path, internal.Enum_reservedNameTag, int32(i)))
+	}
 }
 
 func (r *parseResult) generateSourceCodeInfoForField(sci *sourceCodeInfo, fld *dpb.FieldDescriptorProto, path []int32) {
@@ -1480,7 +1523,7 @@ func validateMessage(res *parseResult, isProto3 bool, prefix string, md *dpb.Des
 	// reserved ranges should not overlap
 	rsvd := make(tagRanges, len(md.ReservedRange))
 	for i, r := range md.ReservedRange {
-		n := res.getReservedRangeNode(r)
+		n := res.getMessageReservedRangeNode(r)
 		rsvd[i] = tagRange{start: r.GetStart(), end: r.GetEnd(), node: n}
 
 	}
@@ -1598,6 +1641,37 @@ func validateEnum(res *parseResult, isProto3 bool, prefix string, ed *dpb.EnumDe
 		}
 	}
 
+	// reserved ranges should not overlap
+	rsvd := make(tagRanges, len(ed.ReservedRange))
+	for i, r := range ed.ReservedRange {
+		n := res.getEnumReservedRangeNode(r)
+		rsvd[i] = tagRange{start: r.GetStart(), end: r.GetEnd(), node: n}
+	}
+	sort.Sort(rsvd)
+	for i := 1; i < len(rsvd); i++ {
+		if rsvd[i].start <= rsvd[i-1].end {
+			return ErrorWithSourcePos{Pos: rsvd[i].node.start(), Underlying: fmt.Errorf("%s: reserved ranges overlap: %d to %d and %d to %d", scope, rsvd[i-1].start, rsvd[i-1].end, rsvd[i].start, rsvd[i].end)}
+		}
+	}
+
+	// now, check that fields don't re-use tags and don't try to use extension
+	// or reserved ranges or reserved names
+	rsvdNames := map[string]struct{}{}
+	for _, n := range ed.ReservedName {
+		rsvdNames[n] = struct{}{}
+	}
+	for _, ev := range ed.Value {
+		evn := res.getEnumValueNode(ev)
+		if _, ok := rsvdNames[ev.GetName()]; ok {
+			return ErrorWithSourcePos{Pos: evn.getName().start(), Underlying: fmt.Errorf("%s: value %s is using a reserved name", scope, ev.GetName())}
+		}
+		// check reserved ranges
+		r := sort.Search(len(rsvd), func(index int) bool { return rsvd[index].end >= ev.GetNumber() })
+		if r < len(rsvd) && rsvd[r].start <= ev.GetNumber() {
+			return ErrorWithSourcePos{Pos: evn.getNumber().start(), Underlying: fmt.Errorf("%s: value %s is using number %d which is in reserved range %d to %d", scope, ev.GetName(), ev.GetNumber(), rsvd[r].start, rsvd[r].end)}
+		}
+	}
+
 	return nil
 }