Repository: openconfig/goyang
Pull request #204 (2021-08-30 23:15:08)

factor out `d *typeDictionary` as a parameter to `initTypes`.
Also call `initTypes` on `init()` only. This confirms that the global
variables `typeMap`, `nameMap`, `aliases` are always the same values, so
that they don't need to be created per instance of the `Modules` object.

This refactor is preparation work for #172 (allowing multiple sets of modules to be processed independently).

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

Coverage increased (+0.01%) to 81.616% when pulling **d0a46029cc8db6cebd14c6220c8d27fbb12c16cc on inittypes-refactor** into **1fb7893798e04830b4d7fbbcd6672b95668fcf73 on master**.


Patch:
diff --git a/pkg/yang/ast.go b/pkg/yang/ast.go
index 83cb61f..5639d02 100644
--- a/pkg/yang/ast.go
+++ b/pkg/yang/ast.go
@@ -30,12 +30,20 @@ import (
 	"strings"
 )
 
+func init() {
+	// Initialize the global variables `typeMap` and `nameMap`.
+	// By doing this, we are making the assumption that all modules will be
+	// parsed according to the type hierarchy rooted at `meta`, and thus
+	// all input YANG modules will be parsed in this manner.
+	initTypes(reflect.TypeOf(&meta{}))
+}
+
 // 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 populates
 	// the statement into the AST node.
-	funcs map[string]func(*Statement, reflect.Value, reflect.Value) error
+	funcs map[string]func(*Statement, reflect.Value, reflect.Value, *typeDictionary) error
 	// required is a list of fields that must be present in the statement.
 	required []string
 	// sRequired maps a statement name to a list of required sub-field
@@ -51,7 +59,7 @@ type yangStatement struct {
 // newYangStatement creates a new yangStatement.
 func newYangStatement() *yangStatement {
 	return &yangStatement{
-		funcs:     make(map[string]func(*Statement, reflect.Value, reflect.Value) error),
+		funcs:     make(map[string]func(*Statement, reflect.Value, reflect.Value, *typeDictionary) error),
 		sRequired: make(map[string][]string),
 	}
 }
@@ -100,20 +108,20 @@ func BuildAST(s *Statement) (Node, error) {
 // buildASTWithTypeDict creates an AST for the input statement, and returns its
 // 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) {
-	v, err := build(s, nilValue, d)
+func buildASTWithTypeDict(stmt *Statement, types *typeDictionary) (Node, error) {
+	v, err := build(stmt, nilValue, types)
 	if err != nil {
 		return nil, err
 	}
 	return v.Interface().(Node), nil
 }
 
-// build builds and returns an AST from the statement s and with parent p. It
-// also takes as input a type dictionary d into which any encountered typedefs
-// within the statement are cached. The type of value returned depends on the
-// keyword in s (see yang.go). It returns an error if it cannot build the
-// statement into its corresponding Node type.
-func build(s *Statement, p reflect.Value, d *typeDictionary) (v reflect.Value, err error) {
+// build builds and returns an AST from the statement stmt and with parent node
+// parent. It also takes as input a type dictionary types into which any
+// encountered typedefs within the statement are cached. The type of value
+// returned depends on the keyword in stmt (see yang.go). It returns an error
+// if it cannot build the statement into its corresponding Node type.
+func build(stmt *Statement, parent reflect.Value, types *typeDictionary) (v reflect.Value, err error) {
 	defer func() {
 		// If we are returning a real Node then call addTypedefs
 		// if the node possibly contains typedefs.
@@ -122,11 +130,11 @@ func build(s *Statement, p reflect.Value, d *typeDictionary) (v reflect.Value, e
 			return
 		}
 		if t, ok := v.Interface().(Typedefer); ok {
-			d.addTypedefs(t)
+			types.addTypedefs(t)
 		}
 	}()
-	keyword := s.Keyword
-	if k, ok := aliases[s.Keyword]; ok {
+	keyword := stmt.Keyword
+	if k, ok := aliases[stmt.Keyword]; ok {
 		keyword = k
 	}
 	t := nameMap[keyword]
@@ -142,25 +150,24 @@ func build(s *Statement, p reflect.Value, d *typeDictionary) (v reflect.Value, e
 	// Handle special cases that are not actually substatements:
 
 	if fn := y.funcs["Name"]; fn != nil {
-		// Name uses s directly.
-		if err := fn(s, v, p); err != nil {
+		// Name uses stmt directly.
+		if err := fn(stmt, v, parent, types); err != nil {
 			return nilValue, err
 		}
 	}
 	if fn := y.funcs["Statement"]; fn != nil {
-		// Statement uses s directly.
-		if err := fn(s, v, p); err != nil {
+		// Statement uses stmt directly.
+		if err := fn(stmt, v, parent, types); err != nil {
 			return nilValue, err
 		}
 	}
 	if fn := y.funcs["Parent"]; fn != nil {
-		// p is the parent.  If there is no parent then p is nilValue,
-		// which is the reflect.Value of nil.
-		// p.IsValid will return false when p is a nil interface
-		// p.IsValid will true if p is references a concrete type
+		// parent is the parent node, which is nilValue (reflect.ValueOf(nil)) if there is none.
+		// parent.IsValid will return false when parent is a nil interface
+		// parent.IsValid will true if parent references a concrete type
 		// (even if it is nil).
-		if p.IsValid() {
-			if err := fn(s, v, p); err != nil {
+		if parent.IsValid() {
+			if err := fn(stmt, v, parent, types); err != nil {
 				return nilValue, err
 			}
 		}
@@ -168,13 +175,13 @@ func build(s *Statement, p reflect.Value, d *typeDictionary) (v reflect.Value, e
 
 	// Now handle the substatements
 
-	for _, ss := range s.statements {
+	for _, ss := range stmt.statements {
 		found[ss.Keyword] = true
 		fn := y.funcs[ss.Keyword]
 		switch {
 		case fn != nil:
 			// Normal case, the keyword is known.
-			if err := fn(ss, v, p); err != nil {
+			if err := fn(ss, v, parent, types); err != nil {
 				return nilValue, err
 			}
 		case len(strings.Split(ss.Keyword, ":")) == 2:
@@ -183,34 +190,34 @@ func build(s *Statement, p reflect.Value, d *typeDictionary) (v reflect.Value, e
 			if y.addext == nil {
 				return nilValue, fmt.Errorf("%s: no extension function", ss.Location())
 			}
-			y.addext(ss, v, p)
+			y.addext(ss, v, parent)
 		default:
-			return nilValue, fmt.Errorf("%s: unknown %s field: %s", ss.Location(), s.Keyword, ss.Keyword)
+			return nilValue, fmt.Errorf("%s: unknown %s field: %s", ss.Location(), stmt.Keyword, ss.Keyword)
 		}
 	}
 
 	// Make sure all of our required field are there.
 	for _, r := range y.required {
 		if !found[r] {
-			return nilValue, fmt.Errorf("%s: missing required %s field: %s", s.Location(), s.Keyword, r)
+			return nilValue, fmt.Errorf("%s: missing required %s field: %s", stmt.Location(), stmt.Keyword, r)
 		}
 	}
 
 	// Make sure required fields based on our keyword are there (module vs submodule)
-	for _, r := range y.sRequired[s.Keyword] {
+	for _, r := range y.sRequired[stmt.Keyword] {
 		if !found[r] {
-			return nilValue, fmt.Errorf("%s: missing required %s field: %s", s.Location(), s.Keyword, r)
+			return nilValue, fmt.Errorf("%s: missing required %s field: %s", stmt.Location(), stmt.Keyword, r)
 		}
 	}
 
 	// Make sure we don't have any field set that is required by a different keyword.
 	for n, or := range y.sRequired {
-		if n == s.Keyword {
+		if n == stmt.Keyword {
 			continue
 		}
 		for _, r := range or {
 			if found[r] {
-				return nilValue, fmt.Errorf("%s: unknown %s field: %s", s.Location(), s.Keyword, r)
+				return nilValue, fmt.Errorf("%s: unknown %s field: %s", stmt.Location(), stmt.Keyword, r)
 			}
 		}
 	}
@@ -222,7 +229,7 @@ func build(s *Statement, p reflect.Value, d *typeDictionary) (v reflect.Value, e
 // Node, with its concrete type being a pointer to a struct defined in yang.go.
 //
 // This function also builds up the functions to populate the input type
-// dictionary d with any encountered typedefs within the statement.
+// dictionary types with any encountered typedefs within the statement.
 //
 // For each field of the struct with a yang tag (e.g., `yang:"command"`), a
 // function is created with "command" as its unique ID.  The complete map of
@@ -232,13 +239,14 @@ func build(s *Statement, p reflect.Value, d *typeDictionary) (v reflect.Value, e
 //
 // The functions have the form:
 //
-//	 func fn(ss *Statement, v, p reflect.Value) error
+//	 func fn(ss *Statement, v, p reflect.Value, types *typeDictionary) error
 //
-// Given s as a Statement of type at, ss is a substatement of s (in a few
+// Given stmt as a Statement of type at, ss is a substatement of stmt (in a few
 // exceptional cases, ss is the Statement itself).  v must have the same type
 // as at and is the structure being filled in.  p is the parent Node, or nil.
-// p is only used to set the Parent field of a Node.  For example, given the
-// following structure and variables:
+// types is the type dictionary cache of the current set of modules being parsed,
+// which is used for looking up typedefs.  p is only used to set the Parent
+// field of a Node.  For example, given the following structure and variables:
 //
 //	type Include struct {
 //		Name         string       `yang:"Name"`
@@ -256,15 +264,15 @@ func build(s *Statement, p reflect.Value, d *typeDictionary) (v reflect.Value, e
 // and revision-date.
 //
 // The function built for RevisionDate will be called for any substatement,
-// ds, of s that has the keyword "revision-date" along with the value of
+// ds, of stmt that has the keyword "revision-date" along with the value of
 // vInc and its parent:
 //
-//	typeMap[tInc]["revision-date"](ss, vInc, parent)
+//	typeMap[tInc]["revision-date"](ss, vInc, parent, types)
 //
 // Normal fields are all processed this same way.
 //
 // The other 4 fields are special.  In the case of Name, Statement, and Parent,
-// the function is passed s, rather than ss, as these fields are not filled in
+// the function is passed stmt, rather than ss, as these fields are not filled in
 // by substatements.
 //
 // The Name command must set its field to the Statement's argument.  The
@@ -286,7 +294,7 @@ func build(s *Statement, p reflect.Value, d *typeDictionary) (v reflect.Value, e
 //                   (This is to support merging Module and SubModule).
 //
 // If at contains substructures, initTypes recurses on the substructures.
-func initTypes(at reflect.Type, d *typeDictionary) {
+func initTypes(at reflect.Type) {
 	if at.Kind() != reflect.Ptr || at.Elem().Kind() != reflect.Struct {
 		panic(fmt.Sprintf("interface not a struct pointer, is %v", at))
 	}
@@ -326,13 +334,13 @@ func initTypes(at reflect.Type, d *typeDictionary) {
 
 		// Ext means this is where we squirrel away extensions
 		if name == "Ext" {
-			// s is the extension to put into v at for field f.
-			y.addext = func(s *Statement, v, _ reflect.Value) error {
+			// stmt is the extension to put into v at for field f.
+			y.addext = func(stmt *Statement, v, _ reflect.Value) error {
 				if v.Type() != at {
 					panic(fmt.Sprintf("given type %s, need type %s", v.Type(), at))
 				}
 				fv := v.Elem().Field(i)
-				fv.Set(reflect.Append(fv, reflect.ValueOf(s)))
+				fv.Set(reflect.Append(fv, reflect.ValueOf(stmt)))
 				return nil
 			}
 			continue
@@ -343,7 +351,7 @@ func initTypes(at reflect.Type, d *typeDictionary) {
 			switch nameMap[name] {
 			case nil:
 				nameMap[name] = dt
-				initTypes(dt, d) // Make sure that structure type is included
+				initTypes(dt) // Make sure that structure type is included
 			case dt:
 			default:
 				panic("redeclared type " + name)
@@ -353,7 +361,7 @@ func initTypes(at reflect.Type, d *typeDictionary) {
 		// Create a function, fn, that will build the field from a
 		// Statement.  These functions are used when actually making
 		// an AST from a Statement Tree.
-		var fn func(*Statement, reflect.Value, reflect.Value) error
+		var fn func(*Statement, reflect.Value, reflect.Value, *typeDictionary) error
 
 		// The field can be a pointer, a slice or a string
 		switch f.Type.Kind() {
@@ -365,7 +373,7 @@ func initTypes(at reflect.Type, d *typeDictionary) {
 			if name != "Parent" {
 				panic(fmt.Sprintf("interface field is %s, not Parent", name))
 			}
-			fn = func(s *Statement, v, p reflect.Value) error {
+			fn = func(stmt *Statement, v, p reflect.Value, types *typeDictionary) error {
 				if !p.Type().Implements(nodeType) {
 					panic(fmt.Sprintf("invalid interface: %v", f.Type.Kind()))
 				}
@@ -377,16 +385,16 @@ func initTypes(at reflect.Type, d *typeDictionary) {
 			if name != "Name" {
 				panic(fmt.Sprintf("string field is %s, not Name", name))
 			}
-			fn = func(s *Statement, v, _ reflect.Value) error {
+			fn = func(stmt *Statement, v, _ reflect.Value, types *typeDictionary) error {
 				if v.Type() != at {
 					panic(fmt.Sprintf("got type %v, want %v", v.Type(), at))
 				}
 				fv := v.Elem().Field(i)
 				if fv.String() != "" {
-					return errors.New(s.Keyword + ": already set")
+					return errors.New(stmt.Keyword + ": already set")
 				}
 
-				v.Elem().Field(i).SetString(s.Argument)
+				v.Elem().Field(i).SetString(stmt.Argument)
 				return nil
 			}
 
@@ -397,11 +405,11 @@ func initTypes(at reflect.Type, d *typeDictionary) {
 				if name != "Statement" {
 					panic(fmt.Sprintf("string field is %s, not Statement", name))
 				}
-				fn = func(s *Statement, v, _ reflect.Value) error {
+				fn = func(stmt *Statement, v, _ reflect.Value, types *typeDictionary) error {
 					if v.Type() != at {
 						panic(fmt.Sprintf("got type %v, want %v", v.Type(), at))
 					}
-					v.Elem().Field(i).Set(reflect.ValueOf(s))
+					v.Elem().Field(i).Set(reflect.ValueOf(stmt))
 					return nil
 				}
 				break
@@ -410,17 +418,17 @@ func initTypes(at reflect.Type, d *typeDictionary) {
 			// Make sure our field type is also setup.
 			descend(name, f.Type)
 
-			fn = func(s *Statement, v, p reflect.Value) error {
+			fn = func(stmt *Statement, v, p reflect.Value, types *typeDictionary) error {
 				if v.Type() != at {
 					panic(fmt.Sprintf("given type %s, need type %s", v.Type(), at))
 				}
 				fv := v.Elem().Field(i)
 				if !fv.IsNil() {
-					return errors.New(s.Keyword + ": already set")
+					return errors.New(stmt.Keyword + ": already set")
 				}
 
 				// Use build to build the value for this field.
-				sv, err := build(s, v, d)
+				sv, err := build(stmt, v, types)
 				if err != nil {
 					return err
 				}
@@ -439,11 +447,11 @@ func initTypes(at reflect.Type, d *typeDictionary) {
 				panic(fmt.Sprintf("invalid type: %v", st.Kind()))
 			case reflect.Ptr:
 				descend(name, st)
-				fn = func(s *Statement, v, p reflect.Value) error {
+				fn = func(stmt *Statement, v, p reflect.Value, types *typeDictionary) error {
 					if v.Type() != at {
 						panic(fmt.Sprintf("given type %s, need type %s", v.Type(), at))
 					}
-					sv, err := build(s, v, d)
+					sv, err := build(stmt, v, types)
 					if err != nil {
 						return err
 					}
diff --git a/pkg/yang/modules.go b/pkg/yang/modules.go
index 281d14d..ef5f10f 100644
--- a/pkg/yang/modules.go
+++ b/pkg/yang/modules.go
@@ -20,7 +20,6 @@ package yang
 
 import (
 	"fmt"
-	"reflect"
 	"sync"
 )
 
@@ -44,7 +43,6 @@ func NewModules() *Modules {
 		byNS:       map[string]*Module{},
 		typeDict:   newTypeDictionary(),
 	}
-	initTypes(reflect.TypeOf(&meta{}), ms.typeDict)
 	return ms
 }
 


Test patch:
diff --git a/pkg/yang/ast_test.go b/pkg/yang/ast_test.go
index 2b16ab7..9010e5b 100644
--- a/pkg/yang/ast_test.go
+++ b/pkg/yang/ast_test.go
@@ -517,7 +517,7 @@ alt_node the_node {
 		}
 
 		typeDict := newTypeDictionary()
-		initTypes(reflect.TypeOf(&meta{}), typeDict)
+		initTypes(reflect.TypeOf(&meta{}))
 
 		ast, err := buildASTWithTypeDict(ss[0], typeDict)
 		switch {
diff --git a/pkg/yang/bgp_test.go b/pkg/yang/bgp_test.go
index 97cda41..1d96a6d 100644
--- a/pkg/yang/bgp_test.go
+++ b/pkg/yang/bgp_test.go
@@ -32,7 +32,7 @@ func TestBGP(t *testing.T) {
 		t.Fatalf("got %d results, want 1", len(ss))
 	}
 	typeDict := newTypeDictionary()
-	initTypes(reflect.TypeOf(&meta{}), typeDict)
+	initTypes(reflect.TypeOf(&meta{}))
 	if _, err := buildASTWithTypeDict(ss[0], typeDict); err != nil {
 		t.Fatal(err)
 	}