Repository: openconfig/goyang
Pull request #186 (2021-07-09 19:20:03)

Change call location of initTypes for typeDictionary
Add `newTypeDictionary()` helper and call `initTypes` closer to where the dictionary is initialized.

Also change tests to use the new method to avoid using the global typeDict.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/41257765/badge)](https://coveralls.io/builds/41257765)

Coverage decreased (-0.06%) to 76.524% when pulling **facde9533aeaa156cb4ab3fdd140bbda047278c9 on refactor-typedict** into **4b27b851cee313cc9d038176e1f849f113c65bb6 on master**.


Patch:
diff --git a/pkg/util/build_yang.go b/pkg/util/build_yang.go
index 9f39221..5dbf385 100644
--- a/pkg/util/build_yang.go
+++ b/pkg/util/build_yang.go
@@ -12,6 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+// Package util contains goyang utility functions that could be useful for
+// external users.
 package util
 
 import (
@@ -20,11 +22,13 @@ import (
 	"github.com/openconfig/goyang/pkg/yang"
 )
 
-// ProcessModules takes a list of modules, and a path specification and
-// runs the yang parser against them, returning a slice of yang.Entry
-// pointers which represent the top level modules that are to be parsed
-// by the struct generation.
-func ProcessModules(yangf, path []string) (map[string]*yang.Entry, []error) {
+// ProcessModules takes a list of either module/submodule names or .yang file
+// paths, and a list of include paths. It runs the yang parser on the YANG
+// files by searching for them in the include paths or in the current
+// directory, returning a slice of yang.Entry pointers which represent the
+// parsed top level modules. It also returns a list of errors encountered while
+// parsing, if any.
+func ProcessModules(yangfiles, path []string) (map[string]*yang.Entry, []error) {
 	for _, p := range path {
 		yang.AddPath(fmt.Sprintf("%s/...", p))
 	}
@@ -32,7 +36,7 @@ func ProcessModules(yangf, path []string) (map[string]*yang.Entry, []error) {
 	ms := yang.NewModules()
 
 	var processErr []error
-	for _, name := range yangf {
+	for _, name := range yangfiles {
 		if err := ms.Read(name); err != nil {
 			processErr = append(processErr, err)
 		}
diff --git a/pkg/yang/ast.go b/pkg/yang/ast.go
index 6fcf11e..83cb61f 100644
--- a/pkg/yang/ast.go
+++ b/pkg/yang/ast.go
@@ -33,13 +33,16 @@ import (
 // A yangStatement contains all information needed to build a particular
 // type of statement into an AST node.
 type yangStatement struct {
-	// funcs is the map of YANG field names to the function that handle them.
+	// funcs is the map of YANG field names to the function that populates
+	// the statement into the AST node.
 	funcs map[string]func(*Statement, reflect.Value, reflect.Value) error
 	// required is a list of fields that must be present in the statement.
 	required []string
-	// sRequired maps statement type to a list of required fields.
-	//    If a field is required by statement type foo, then only foo should
-	//    have the field.
+	// sRequired maps a statement name to a list of required sub-field
+	// names. The statement name can be an alias of the primary field type.
+	//    e.g. If a field is required by statement type foo, then only foo
+	//    should have the field. If bar is an alias of foo, it must not
+	//    have this field.
 	sRequired map[string][]string
 	// addext is the function to handle possible extensions.
 	addext func(*Statement, reflect.Value, reflect.Value) error
@@ -78,6 +81,12 @@ type meta struct {
 
 // aliases is a map of "aliased" names, that is, two types of statements
 // that parse (nearly) the same.
+// NOTE: This only works for root-level aliasing for now, which is good enough
+// for module/submodule. This is because yangStatement.funcs doesn't store the
+// handler function for aliased fields, and sRequired also may only store the
+// correct values when processing a root-level statement due to aliasing. These
+// issues would need to be fixed in order to support aliasing for non-top-level
+// statements.
 var aliases = map[string]string{
 	"submodule": "module",
 }
@@ -92,7 +101,6 @@ func BuildAST(s *Statement) (Node, error) {
 // root node. It also takes as input a type dictionary into which any
 // encountered typedefs within the statement are cached.
 func buildASTWithTypeDict(s *Statement, d *typeDictionary) (Node, error) {
-	initTypes(reflect.TypeOf(&meta{}), d)
 	v, err := build(s, nilValue, d)
 	if err != nil {
 		return nil, err
diff --git a/pkg/yang/modules.go b/pkg/yang/modules.go
index 60ccc55..8bba228 100644
--- a/pkg/yang/modules.go
+++ b/pkg/yang/modules.go
@@ -20,6 +20,7 @@ package yang
 
 import (
 	"fmt"
+	"reflect"
 )
 
 // Modules contains information about all the top level modules and
@@ -35,14 +36,16 @@ type Modules struct {
 
 // NewModules returns a newly created and initialized Modules.
 func NewModules() *Modules {
-	return &Modules{
+	ms := &Modules{
 		Modules:    map[string]*Module{},
 		SubModules: map[string]*Module{},
 		includes:   map[*Module]bool{},
 		byPrefix:   map[string]*Module{},
 		byNS:       map[string]*Module{},
-		typeDict:   &typeDictionary{dict: map[Node]map[string]*Typedef{}},
+		typeDict:   newTypeDictionary(),
 	}
+	initTypes(reflect.TypeOf(&meta{}), ms.typeDict)
+	return ms
 }
 
 // Read reads the named yang module into ms.  The name can be the name of an
diff --git a/pkg/yang/types.go b/pkg/yang/types.go
index 829e816..4d255ee 100644
--- a/pkg/yang/types.go
+++ b/pkg/yang/types.go
@@ -31,6 +31,10 @@ type typeDictionary struct {
 	dict map[Node]map[string]*Typedef
 }
 
+func newTypeDictionary() *typeDictionary {
+	return &typeDictionary{dict: map[Node]map[string]*Typedef{}}
+}
+
 // typeDict is a protected global dictionary of all typedefs.
 // TODO(borman): should this be made as part of some other structure, rather
 // than a singleton.  That can be done later when we replumb everything to more
diff --git a/pkg/yang/types_builtin.go b/pkg/yang/types_builtin.go
index 1998942..ee408d1 100644
--- a/pkg/yang/types_builtin.go
+++ b/pkg/yang/types_builtin.go
@@ -250,6 +250,7 @@ func (r YRange) String() string {
 	return r.Min.String() + ".." + r.Max.String()
 }
 
+// Equal compares whether two YRanges are equal.
 func (r YRange) Equal(s YRange) bool {
 	return r.Min.Equal(s.Min) && r.Max.Equal(s.Max)
 }
@@ -341,7 +342,7 @@ func (r YangRange) Contains(s YangRange) bool {
 		// min is always within range
 		if ss.Min.Kind != MinNumber {
 			for r[ri].Max.Less(ss.Min) {
-				ri += 1
+				ri++
 				if ri == len(r) {
 					return false
 				}


Test patch:
diff --git a/pkg/util/build_yang_test.go b/pkg/util/build_yang_test.go
index 8b9cf23..5b30d07 100644
--- a/pkg/util/build_yang_test.go
+++ b/pkg/util/build_yang_test.go
@@ -31,6 +31,11 @@ func TestProcessModules(t *testing.T) {
 		inFiles:  []string{"testdata/00-valid-module.yang"},
 		inPath:   []string{"testdata"},
 		wantMods: []string{"test-module"},
+	}, {
+		name:     "simple valid module without .yang extension",
+		inFiles:  []string{"00-valid-module"},
+		inPath:   []string{"testdata"},
+		wantMods: []string{"test-module"},
 	}, {
 		name:    "simple invalid module",
 		inFiles: []string{"testdata/01-invalid-module.yang"},
diff --git a/pkg/yang/ast_test.go b/pkg/yang/ast_test.go
index d46844c..2b16ab7 100644
--- a/pkg/yang/ast_test.go
+++ b/pkg/yang/ast_test.go
@@ -32,10 +32,17 @@ type MainNode struct {
 	ChildNode  *SubNode   `yang:"child_node"`
 	ChildSlice []*SubNode `yang:"child_slice"`
 	ReqNode    *ReqNode   `yang:"req_node"`
-	AltReqNode *ReqNode   `yang:"alt_req_node"`
+	MainField  *Value     `yang:"main_field,required=main_node"`
+	AltField   *Value     `yang:"alt_field,required=alt_node"`
+}
+
+func (m *MainNode) Kind() string {
+	if m.AltField != nil {
+		return "alt_node"
+	}
+	return "main_node"
 }
 
-func (MainNode) Kind() string             { return "main_node" }
 func (m *MainNode) ParentNode() Node      { return m.Parent }
 func (m *MainNode) NName() string         { return m.Name }
 func (m *MainNode) Statement() *Statement { return m.Source }
@@ -103,15 +110,15 @@ func (m *MainNode) checkEqual(n Node) string {
 			return fmt.Sprintf("req_node: %s", s)
 		}
 	}
-	if (m.AltReqNode == nil) != (o.AltReqNode == nil) {
-		if m.AltReqNode == nil {
-			return "unexpected alt_req_node entry"
+	if (m.AltField == nil) != (o.AltField == nil) {
+		if m.AltField == nil {
+			return "unexpected alt_field entry"
 		}
-		return "missing expected alt_req_node entry"
+		return "missing expected alt_field entry"
 	}
-	if m.AltReqNode != nil {
-		if s := m.AltReqNode.checkEqual(o.AltReqNode); s != "" {
-			return fmt.Sprintf("alt_req_node: %s", s)
+	if m.AltField != nil {
+		if m.AltField.Name != o.AltField.Name {
+			return fmt.Sprintf("got alt_field of %s, want %s", o.AltField.Name, m.AltField.Name)
 		}
 	}
 	return ""
@@ -164,9 +171,6 @@ type ReqNode struct {
 }
 
 func (s *ReqNode) Kind() string {
-	if s.AltReqField != nil {
-		return "alt_req_node"
-	}
 	return "req_node"
 }
 func (s *ReqNode) ParentNode() Node      { return s.Parent }
@@ -235,11 +239,10 @@ func TestAST(t *testing.T) {
 	type meta struct {
 		MainNode []*MainNode `yang:"main_node"`
 	}
-	initTypes(reflect.TypeOf(&meta{}), &typeDict)
 
 	old_aliases := aliases
 	aliases = map[string]string{
-		"alt_req_node": "req_node",
+		"alt_node": "main_node",
 	}
 
 	for _, tt := range []struct {
@@ -258,17 +261,22 @@ main_node the_node {
 	// See https://tools.ietf.org/html/rfc6020#section-7.17
 	ex:ext1 value1;
 	ex:ext2 value2;
+	main_field foo;
 }
 `,
 			out: &MainNode{
 				Source: SA("main_node", "the_node",
 					SA("ex:ext1", "value1"),
-					SA("ex:ext2", "value2")),
+					SA("ex:ext2", "value2"),
+					SA("main_field", "foo")),
 				Name: "the_node",
 				Extensions: []*Statement{
 					SA("ex:ext1", "value1"),
 					SA("ex:ext2", "value2"),
 				},
+				MainField: &Value{
+					Name: "foo",
+				},
 			},
 		},
 		{
@@ -288,6 +296,7 @@ main_node the_node {
 	child_slice element2 {
 		sub_field el2;
 	}
+	main_field foo;
 }`,
 			out: &MainNode{
 				Source: SA("main_node", "the_node",
@@ -300,6 +309,7 @@ main_node the_node {
 						SA("sub_field", "el1")),
 					SA("child_slice", "element2",
 						SA("sub_field", "el2")),
+					SA("main_field", "foo"),
 				),
 				Name: "the_node",
 				Field: &Value{
@@ -339,56 +349,48 @@ main_node the_node {
 						},
 					},
 				},
+				MainField: &Value{
+					Name: "foo",
+				},
 			},
 		},
 		{
 			line: line(),
 			in: `
+// This test tests for the presence of a required field.
+// main_node requires the field named "main_field".
 main_node the_node {
-	// This test tests for the presence of a required field.
-	// req_node requires the field named "req_field".
-	// alt_req_node requires the field named "alt_req_field".
-	req_node value1 {
-		req_field foo {
-		}
-	}
-
-	alt_req_node value2 {
-		req_field foo {
-		}
-		alt_req_field bar {
-		}
+	main_field value1 {
 	}
 }
 `,
 			out: &MainNode{
 				Source: SA("main_node", "the_node",
-					SA("req_node", "value1",
-						SA("req_field", "foo")),
-					SA("alt_req_node", "value2",
-						SA("req_field", "foo"),
-						SA("alt_req_field", "bar")),
+					SA("main_field", "value1"),
 				),
 				Name: "the_node",
-				ReqNode: &ReqNode{
-					Source: SA("req_node", "value1",
-						SA("req_field", "foo")),
+				MainField: &Value{
 					Name: "value1",
-					ReqField: &Value{
-						Name: "foo",
-					},
 				},
-				AltReqNode: &ReqNode{
-					Source: SA("alt_req_node", "value2",
-						SA("req_field", "foo"),
-						SA("alt_req_field", "bar")),
+			},
+		},
+		{
+			line: line(),
+			in: `
+// This test tests for the presence of a required= field.
+// alt_node requires the field named "alt_field".
+alt_node the_node {
+	alt_field value2 {
+	}
+}
+`,
+			out: &MainNode{
+				Source: SA("alt_node", "the_node",
+					SA("alt_field", "value2"),
+				),
+				Name: "the_node",
+				AltField: &Value{
 					Name: "value2",
-					ReqField: &Value{
-						Name: "foo",
-					},
-					AltReqField: &Value{
-						Name: "bar",
-					},
 				},
 			},
 		},
@@ -412,9 +414,44 @@ main_node the_node {
 			line: line(),
 			in: `
 main_node the_node {
-	// This test tests that the absence of a required field.
+	// This test tests for the presence of a required field.
+	// req_node requires the field named "req_field".
+	req_node value1 {
+		req_field foo {
+		}
+	}
+	main_field foo;
+}
+`,
+			out: &MainNode{
+				Source: SA("main_node", "the_node",
+					SA("req_node", "value1",
+						SA("req_field", "foo")),
+					SA("main_field", "foo"),
+				),
+				Name: "the_node",
+				ReqNode: &ReqNode{
+					Source: SA("req_node", "value1",
+						SA("req_field", "foo")),
+					Name: "value1",
+					ReqField: &Value{
+						Name: "foo",
+					},
+				},
+				MainField: &Value{
+					Name: "foo",
+				},
+			},
+		},
+		{
+			line: line(),
+			in: `
+main_node the_node {
+	// This test tests that the absence of a required field fails.
 	// req_node requires the field named "req_field".
-	req_node value1 ;
+	req_node value1 {
+	}
+	main_field foo;
 }
 `,
 			err: `ast.yang:5:2: missing required req_node field: req_field`,
@@ -423,32 +460,50 @@ main_node the_node {
 			line: line(),
 			in: `
 main_node the_node {
-	// This test tests that the alt_req_field, specified with
-	// required=alt_req_node, causes the AST construction to error when a
-	// req_node contains it.
+	// This test tests that the absence of a required field.
+	// main_node requires the field named "main_field".
 	req_node value1 {
 		req_field foo {
 		}
-		alt_req_field foo {
-		}
 	}
 }
 `,
-			err: `ast.yang:6:2: unknown req_node field: alt_req_field`,
+			err: `ast.yang:2:1: missing required main_node field: main_field`,
 		},
 		{
 			line: line(),
 			in: `
+// This test tests that the alt_field, specified with
+// required=alt_node, causes the AST construction to error when a
+// main_node contains it.
 main_node the_node {
-	// This test tests that required=alt_req_node enforces that
-	// alt_req_node must contain it.
-	alt_req_node value2 {
-		req_field foo {
-		}
-	}
+	main_field foo;
+	alt_field foo;
 }
 `,
-			err: `ast.yang:5:2: missing required alt_req_node field: alt_req_field`,
+			err: `ast.yang:5:1: unknown main_node field: alt_field`,
+		},
+		{
+			line: line(),
+			in: `
+// This test tests that required=alt_node enforces that
+// alt_node must contain it.
+alt_node the_node {
+	main_field foo;
+	alt_field foo;
+}
+`,
+			err: `ast.yang:4:1: unknown alt_node field: main_field`,
+		},
+		{
+			line: line(),
+			in: `
+// This test tests that required=alt_node enforces that
+// alt_node must contain it.
+alt_node the_node {
+}
+`,
+			err: `ast.yang:4:1: missing required alt_node field: alt_field`,
 		},
 	} {
 		ss, err := Parse(tt.in, "ast.yang")
@@ -460,7 +515,11 @@ main_node the_node {
 			t.Errorf("%d: got %d results, want 1", tt.line, len(ss))
 			continue
 		}
-		ast, err := BuildAST(ss[0])
+
+		typeDict := newTypeDictionary()
+		initTypes(reflect.TypeOf(&meta{}), typeDict)
+
+		ast, err := buildASTWithTypeDict(ss[0], typeDict)
 		switch {
 		case err == nil && tt.err == "":
 			if s := tt.out.checkEqual(ast); s != "" {
diff --git a/pkg/yang/bgp_test.go b/pkg/yang/bgp_test.go
index 120ff5e..97cda41 100644
--- a/pkg/yang/bgp_test.go
+++ b/pkg/yang/bgp_test.go
@@ -14,7 +14,10 @@
 
 package yang
 
-import "testing"
+import (
+	"reflect"
+	"testing"
+)
 
 // TestBGP simply makes sure we are able to parse a version of Anees's
 // BGP model.  We don't actually attempt to validate we got the right
@@ -28,8 +31,9 @@ func TestBGP(t *testing.T) {
 	if len(ss) != 1 {
 		t.Fatalf("got %d results, want 1", len(ss))
 	}
-	_, err = BuildAST(ss[0])
-	if err != nil {
+	typeDict := newTypeDictionary()
+	initTypes(reflect.TypeOf(&meta{}), typeDict)
+	if _, err := buildASTWithTypeDict(ss[0], typeDict); err != nil {
 		t.Fatal(err)
 	}
 }
diff --git a/pkg/yang/entry_test.go b/pkg/yang/entry_test.go
index d8a88bf..7bb2331 100644
--- a/pkg/yang/entry_test.go
+++ b/pkg/yang/entry_test.go
@@ -161,7 +161,6 @@ module base {
 
 func TestBadYang(t *testing.T) {
 	for _, tt := range badInputs {
-		typeDict = typeDictionary{dict: map[Node]map[string]*Typedef{}}
 		ms := NewModules()
 		if err := ms.Parse(tt.in, tt.name); err != nil {
 			t.Fatalf("unexpected error %s", err)
diff --git a/pkg/yang/types_test.go b/pkg/yang/types_test.go
index 15e1872..7e5774b 100644
--- a/pkg/yang/types_test.go
+++ b/pkg/yang/types_test.go
@@ -343,7 +343,7 @@ func TestTypeResolve(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.desc, func(t *testing.T) {
 			// We can initialize a value to ourself, so to it here.
-			errs := tt.in.resolve(&typeDict)
+			errs := tt.in.resolve(newTypeDictionary())
 
 			// TODO(borman):  Do not hack out Root and Base.  These
 			// are hacked out for now because they can be self-referential,