lang: Improve string interpolation

The original string interpolation was based on hil which didn't allow
proper escaping, since they used a different escape pattern. Secondly,
the golang Unquote function didn't deal with the variable substitution,
which meant it had to be performed in a second step.

Most importantly, because we did this partial job in Unquote (the fact
that is strips the leading and trailing quotes tricked me into thinking
I was done with interpolation!) it was impossible to remedy the
remaining parts in a second pass with hil. Both operations needs to be
done in a single step. This is logical when you aren't tunnel visioned.

This patch replaces both of these so that string interpolation works
properly. This removes the ability to allow inline function calls in a
string, however this was an incidental feature, and it's not clear that
having it is a good idea. It also requires you wrap the var name with
curly braces. (They are not optional.)

This comes with a load of tests, but I think I got some of it wrong,
since I'm quite new at ragel. If you find something, please say so =D In
any case, this is much better than the original hil implementation, and
easy for a new contributor to patch to make the necessary fixes.

examples/lang/contains0.mcl

@@ -21,7 +21,8 @@ $x = if sys.hostname() in ["h1", "h3",] {
 	fmt.printf("i (%s) was not chosen :(\n", sys.hostname())
 }
 
-file "/tmp/mgmt/hello-${sys.hostname()}" {
+$host = sys.hostname()
+file "/tmp/mgmt/hello-${host}" {
 	state => $const.res.file.state.exists,
 	content => $x,
 }

examples/lang/escaping1.mcl

@@ -0,0 +1,30 @@
+print "escaping1" {
+	msg => "\${hello}",
+
+	Meta:autogroup => false,
+}
+
+$hello = "hi"
+print "escaping2" {
+	msg => "${hello}",
+
+	Meta:autogroup => false,
+}
+
+print "escaping3" {
+	msg => "\\${hello}",
+
+	Meta:autogroup => false,
+}
+
+print "escaping4" {
+	msg => "\\ ${hello}",
+
+	Meta:autogroup => false,
+}
+
+print "escaping5" {
+	msg => "\"${hello}",
+
+	Meta:autogroup => false,
+}

examples/lang/exchange0.mcl

@@ -12,7 +12,8 @@ import "world"
 $rand = random1(8)
 $exchanged = world.exchange("keyns", $rand)
 
-file "/tmp/mgmt/exchange-${sys.hostname()}" {
+$host = sys.hostname()
+file "/tmp/mgmt/exchange-${host}" {
 	state => $const.res.file.state.exists,
 	content => template("Found: {{ . }}\n", $exchanged),
 }

examples/lang/hostname0.mcl

@@ -1,6 +1,7 @@
 import "sys"
 
-file "/tmp/mgmt/${sys.hostname()}" {
+$host = sys.hostname()
-	content => "hello from ${sys.hostname()}!\n",
+file "/tmp/mgmt/${host}" {
+	content => "hello from ${host}!\n",
 	state => $const.res.file.state.exists,
 }

examples/lang/os.mcl

@@ -1,5 +1,5 @@
 import "golang/os"
-import "golang/exec"
+import "golang/os/exec"
 import "fmt"
 
 $tmpdir = os.temp_dir()
@@ -9,9 +9,10 @@ file "${tmpdir}/execinfo" {
 	content => fmt.printf("mgmt is at %s\n", os.executable()),
 }
 
+$home = os.getenv("HOME")
 file "${tmpdir}/mgmtenv" {
 	state => $const.res.file.state.exists,
-	content => os.expand_env("$HOME sweet ${os.getenv(\"HOME\")}\n"),
+	content => os.expand_env("$HOME sweet ${home}\n"),
 }
 
 file "${tmpdir}/mgmtos" {
@@ -19,9 +20,10 @@ file "${tmpdir}/mgmtos" {
 	content => os.readlink("/bin"),
 }
 
+$cache_dir = os.user_cache_dir()
+$home_dir = os.user_home_dir()
 $rm = exec.look_path("rm")
-
 file "${tmpdir}/cache" {
 	state => $const.res.file.state.exists,
-	content => "Plz cache in ${os.user_cache_dir()}.\nYour home is ${os.user_home_dir()}. Remove with ${rm}\n",
+	content => "Plz cache in ${cache_dir}.\nYour home is ${home_dir}. Remove with ${rm}\n",
 }

examples/lang/schedule0.mcl

@@ -16,7 +16,8 @@ $set = world.schedule("xsched", $opts)
 # and if you want, you can omit the options entirely:
 #$set = world.schedule("xsched")
 
-file "/tmp/mgmt/scheduled-${sys.hostname()}" {
+$host = sys.hostname()
+file "/tmp/mgmt/scheduled-${host}" {
 	state => $const.res.file.state.exists,
 	content => template("set: {{ . }}\n", $set),
 }

integration/basic_test.go

@@ -167,9 +167,10 @@ func TestCluster1(t *testing.T) {
 		code := util.Code(`
 		import "sys"
 		$root = sys.getenv("MGMT_TEST_ROOT")
+		$host = sys.hostname()
 
 		file "${root}/mgmt-hostname" {
-			content => "i am ${sys.hostname()}\n",
+			content => "i am ${host}\n",
 			state => $const.res.file.state.exists,
 		}
 		`)
@@ -192,9 +193,10 @@ func TestCluster1(t *testing.T) {
 		code := util.Code(`
 		import "sys"
 		$root = sys.getenv("MGMT_TEST_ROOT")
+		$host = sys.hostname()
 
 		file "${root}/mgmt-hostname" {
-			content => "i am ${sys.hostname()}\n",
+			content => "i am ${host}\n",
 			state => $const.res.file.state.exists,
 		}
 		`)

lang/Makefile

@@ -22,13 +22,13 @@ OLDGOYACC := $(shell go version | grep -E 'go1.6|go1.7')
 
 all: build
 
-build: lexer.nn.go y.go
+build: lexer.nn.go y.go interpolate/parse.generated.go
 	@# recursively run make in child dir named types
 	@$(MAKE) --quiet -C types
 
 clean:
 	$(MAKE) --quiet -C types clean
-	@rm -f lexer.nn.go y.go y.output || true
+	@rm -f lexer.nn.go y.go y.output interpolate/parse.generated.go || true
 
 lexer.nn.go: lexer.nex
 	@echo "Generating: lexer..."
@@ -45,5 +45,15 @@ else
 endif
 	@ROOT="$$( cd "$$( dirname "$${BASH_SOURCE[0]}" )" && cd .. && pwd )" && $$ROOT/misc/header.sh 'y.go'
 
+interpolate/parse.generated.go: interpolate/parse.rl
+	@echo "Generating: interpolation..."
+	ragel -Z -G2 -o interpolate/parse.generated.go interpolate/parse.rl
+	#@ROOT="$$( cd "$$( dirname "$${BASH_SOURCE[0]}" )" && cd .. && pwd )" && $$ROOT/misc/header.sh 'interpolate/parse.generated.go'
+	# XXX: I have no idea why I need to sed twice. I give up :P
+	# remove the ragel header so our header test passes
+	@sed -i -e "1d" 'interpolate/parse.generated.go'
+	@sed -i -e "1d" 'interpolate/parse.generated.go'
+	gofmt -s -w 'interpolate/parse.generated.go'
+
 fuzz:
 	@$(MAKE) --quiet -C fuzz

lang/interpolate.go

@@ -21,12 +21,20 @@ import (
 	"fmt"
 
 	"github.com/purpleidea/mgmt/lang/interfaces"
+	"github.com/purpleidea/mgmt/lang/interpolate"
 	"github.com/purpleidea/mgmt/util/errwrap"
 
 	"github.com/hashicorp/hil"
 	hilast "github.com/hashicorp/hil/ast"
 )
 
+const (
+	// UseHilInterpolation specifies that we use the legacy Hil interpolate.
+	// This can't properly escape a $ in the standard way. It's here in case
+	// someone wants to play with it and examine how the AST stuff worked...
+	UseHilInterpolation = false
+)
+
 // Pos represents a position in the code.
 // TODO: consider expanding with range characteristics.
 type Pos struct {
@@ -35,12 +43,72 @@ type Pos struct {
 	Filename string // optional source filename, if known
 }
 
-// InterpolateStr interpolates a string and returns the representative AST. This
+// InterpolateStr interpolates a string and returns the representative AST.
-// particular implementation uses the hashicorp hil library and syntax to do so.
 func InterpolateStr(str string, pos *Pos, data *interfaces.Data) (interfaces.Expr, error) {
 	if data.Debug {
 		data.Logf("interpolating: %s", str)
 	}
+
+	if UseHilInterpolation {
+		return InterpolateHil(str, pos, data)
+	}
+	return InterpolateRagel(str, pos, data)
+}
+
+// InterpolateRagel interpolates a string and returns the representative AST. It
+// uses the ragel parser to perform the string interpolation.
+func InterpolateRagel(str string, pos *Pos, data *interfaces.Data) (interfaces.Expr, error) {
+	sequence, err := interpolate.Parse(str)
+	if err != nil {
+		return nil, errwrap.Wrapf(err, "parser failed")
+	}
+
+	exprs := []interfaces.Expr{}
+	for _, term := range sequence {
+
+		switch t := term.(type) {
+		case interpolate.Literal:
+			expr := &ExprStr{
+				V: t.Value,
+			}
+			exprs = append(exprs, expr)
+
+		case interpolate.Variable:
+			expr := &ExprVar{
+				Name: t.Name,
+			}
+			exprs = append(exprs, expr)
+		default:
+			return nil, fmt.Errorf("unknown term (%T): %+v", t, t)
+		}
+	}
+
+	// If we didn't find anything of value, we got an empty string...
+	if len(sequence) == 0 && str == "" { // be doubly sure...
+		expr := &ExprStr{
+			V: "",
+		}
+		exprs = append(exprs, expr)
+	}
+
+	// The parser produces non-optimal results where two strings are next to
+	// each other, when they could be statically combined together.
+	simplified, err := simplifyExprList(exprs)
+	if err != nil {
+		return nil, errwrap.Wrapf(err, "expr list simplify failed")
+	}
+
+	result, err := concatExprListIntoCall(simplified)
+	if err != nil {
+		return nil, errwrap.Wrapf(err, "concat expr list failed")
+	}
+
+	return result, errwrap.Wrapf(result.Init(data), "init failed")
+}
+
+// InterpolateHil interpolates a string and returns the representative AST. This
+// particular implementation uses the hashicorp hil library and syntax to do so.
+func InterpolateHil(str string, pos *Pos, data *interfaces.Data) (interfaces.Expr, error) {
 	var line, column int = -1, -1
 	var filename string
 	if pos != nil {
@@ -246,6 +314,7 @@ func concatExprListIntoCall(exprs []interfaces.Expr) (interfaces.Expr, error) {
 	}
 
 	return &ExprCall{
+		// NOTE: if we don't set the data field we need Init() called on it!
 		Name: operatorFuncName, // concatenate the two strings with + operator
 		Args: []interfaces.Expr{
 			operator, // operator first
@@ -254,3 +323,42 @@ func concatExprListIntoCall(exprs []interfaces.Expr) (interfaces.Expr, error) {
 		},
 	}, nil
 }
+
+// simplifyExprList takes a list of *ExprStr and *ExprVar and groups the
+// sequential *ExprStr's together. If you pass it a list of Expr's that contains
+// a different type of Expr, then this will error.
+func simplifyExprList(exprs []interfaces.Expr) ([]interfaces.Expr, error) {
+	last := false
+	result := []interfaces.Expr{}
+
+	for _, x := range exprs {
+		switch v := x.(type) {
+		case *ExprStr:
+			if !last {
+				last = true
+				result = append(result, x)
+				continue
+			}
+
+			// combine!
+			expr := result[len(result)-1] // there has to be at least one
+			str, ok := expr.(*ExprStr)
+			if !ok {
+				// programming error
+				return nil, fmt.Errorf("unexpected type (%T)", expr)
+			}
+			str.V += v.V // combine!
+			//last = true // redundant, it's already true
+			// ... and don't append, we've combined!
+
+		case *ExprVar:
+			last = false // the next one can't combine with me
+			result = append(result, x)
+
+		default:
+			return nil, fmt.Errorf("unsupported type (%T)", x)
+		}
+	}
+
+	return result, nil
+}

lang/interpolate/.gitignore

@@ -0,0 +1 @@
+parse.generated.go

lang/interpolate/parse.rl

@@ -0,0 +1,151 @@
+// Mgmt
+// Copyright (C) 2013-2021+ James Shubin and the project contributors
+// Written by James Shubin <james@shubin.ca> and the project contributors
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package interpolate
+
+import (
+	"fmt"
+)
+
+%%{
+	machine interpolate;
+	write data;
+}%%
+
+// Parse performs string interpolation on the input. It returns the list of
+// tokens found. It looks for variables of the format ${foo}. The curly braces
+// are required.
+func Parse(data string) (out Stream, _ error) {
+	var (
+		// variables used by Ragel
+		cs  = 0 // current state
+		p   = 0 // current position in data
+		pe  = len(data)
+		eof = pe // eof == pe if this is the last data block
+
+		// Index in data where the currently captured string started.
+		idx int
+
+		x string   // The string we use for holding a temporary value.
+		l Literal  // The string literal being read, if any.
+		v Variable // The variable being read, if any.
+
+		// Current token. This is either the variable that we just read
+		// or the string literal. We will append it to `out` and move
+		// on.
+		t Token
+	)
+
+	%%{
+		# Record the current position as the start of a string. This is
+		# usually used with the entry transition (>) to start capturing
+		# the string when a state machine is entered.
+		#
+		# fpc is the current position in the string (basically the same
+		# as the variable `p` but a special Ragel keyword) so after
+		# executing `start`, data[idx:fpc+1] is the string from when
+		# start was called to the current position (inclusive).
+		action start { idx = fpc }
+
+		# A variable always starts with an lowercase alphabetical char
+		# and contains lowercase alphanumeric characters or numbers,
+		# underscores, and non-consecutive dots. The last char must not
+		# be an underscore or a dot.
+		# XXX: check that we don't get consecutive underscores or dots!
+		var_name = ( [a-z] ([a-z0-9_] | ('.' | '_') [a-z0-9_])* )
+		>start
+		@{
+			v.Name = data[idx:fpc+1]
+		};
+
+		# var is a reference to a variable.
+		var = '${' var_name '}' ;
+
+		# Any special escape characters are matched here.
+		escaped_lit = '\\' ( 'a' | 'b' | 'f' | 'n' | 'r' | 't' | 'v' | '\\' | '"' | '$' )
+		@{
+			switch s := data[fpc:fpc+1]; s {
+			case "a":
+				x = "\a"
+			case "b":
+				x = "\b"
+			//case "e":
+			//	x = "\e" // non-standard
+			case "f":
+				x = "\f"
+			case "n":
+				x = "\n"
+			case "r":
+				x = "\r"
+			case "t":
+				x = "\t"
+			case "v":
+				x = "\v"
+			case "\\":
+				x = "\\"
+			case "\"":
+				x = "\""
+			case "$":
+				x = "$"
+			//case "0":
+			//	x = "\x00"
+			default:
+				//x = s // in case we want to avoid erroring
+				// this is a programming (parser) error I think
+				return nil, fmt.Errorf("unhandled escape sequence token: %s", s)
+			}
+			l = Literal{Value: x}
+		};
+
+		# XXX: explicitly try and add this one?
+		#escape_lit = '\\\\'
+		#@{
+		#	l = Literal{Value: "\\\\"}
+		#};
+
+		# Anything followed by a '$' that is not a '{' is used as-is
+		# with the dollar.
+		dollar_lit = '$' (any - '{')
+		@{
+			l = Literal{Value: data[fpc-1:fpc+1]}
+		};
+
+		# Literal strings that don't contain '$' or '\'.
+		simple_lit = (any - '$' - '\\')+
+		>start
+		@{
+			l = Literal{Value: data[idx:fpc + 1]}
+		};
+
+		lit = escaped_lit | dollar_lit | simple_lit;
+
+		# Tokens are the two possible components in a string. Either a
+		# literal or a variable reference.
+		token = (var @{ t = v }) | (lit @{ t = l });
+
+		main := (token %{ out = append(out, t) })**;
+
+		write init;
+		write exec;
+	}%%
+
+	if cs < %%{ write first_final; }%% {
+		return nil, fmt.Errorf("cannot parse string: %s", data)
+	}
+
+	return out, nil
+}

lang/interpolate/types.go

@@ -0,0 +1,46 @@
+// Mgmt
+// Copyright (C) 2013-2021+ James Shubin and the project contributors
+// Written by James Shubin <james@shubin.ca> and the project contributors
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package interpolate
+
+// Stream is the list of tokens that are produced after interpolating a string.
+// It is created by using the generated Parse function.
+// TODO: In theory a more advanced parser could produce an AST here instead.
+type Stream []Token
+
+// Token is the interface that every token must implement.
+type Token interface {
+	token()
+}
+
+// Literal is a string literal that we have found after interpolation parsing.
+type Literal struct {
+	Value string
+}
+
+// token ties the Literal to the Token interface.
+func (Literal) token() {}
+
+// Variable is a variable name that we have found after interpolation parsing.
+type Variable struct {
+	Name string
+}
+
+// token ties the Variable to the Token interface.
+func (Variable) token() {}
+
+// TODO: do we want to allow inline-function calls in a string?

lang/interpolate_test.go

@@ -30,6 +30,7 @@ import (
 
 	"github.com/davecgh/go-spew/spew"
 	"github.com/kylelemons/godebug/pretty"
+	"github.com/sanity-io/litter"
 )
 
 func TestInterpolate0(t *testing.T) {
@@ -127,6 +128,66 @@ func TestInterpolate0(t *testing.T) {
 			ast:  ast,
 		})
 	}
+	{
+		ast := &StmtProg{
+			Prog: []interfaces.Stmt{
+				&StmtRes{
+					Kind: "test",
+					Name: &ExprStr{
+						V: "t1",
+					},
+					Contents: []StmtResContents{
+						&StmtResField{
+							Field: "stringptr",
+							Value: &ExprStr{
+								V: "${hello}",
+							},
+						},
+					},
+				},
+			},
+		}
+		testCases = append(testCases, test{
+			name: "variable escaping 1",
+			code: `
+			test "t1" {
+				stringptr => "\${hello}",
+			}
+			`,
+			fail: false,
+			ast:  ast,
+		})
+	}
+	{
+		ast := &StmtProg{
+			Prog: []interfaces.Stmt{
+				&StmtRes{
+					Kind: "test",
+					Name: &ExprStr{
+						V: "t1",
+					},
+					Contents: []StmtResContents{
+						&StmtResField{
+							Field: "stringptr",
+							Value: &ExprStr{
+								V: `\` + `$` + `{hello}`,
+							},
+						},
+					},
+				},
+			},
+		}
+		testCases = append(testCases, test{
+			name: "variable escaping 2",
+			code: `
+			test "t1" {
+				stringptr => "` + `\\` + `\$` + "{hello}" + `",
+			}
+			`,
+			fail: false,
+			ast:  ast,
+		})
+	}
 
 	names := []string{}
 	for index, tc := range testCases { // run all the tests
@@ -189,14 +250,28 @@ func TestInterpolate0(t *testing.T) {
 				return
 			}
 			// double check because DeepEqual is different since the logf exists
+			lo := &litter.Options{
+				//Compact: false,
+				StripPackageNames: true,
+				HidePrivateFields: true,
+				HideZeroValues:    true,
+				//FieldExclusions: regexp.MustCompile(`^(data)$`),
+				//FieldFilter       func(reflect.StructField, reflect.Value) bool
+				//HomePackage       string
+				//Separator         string
+			}
+			if lo.Sdump(iast) == lo.Sdump(exp) { // simple diff
+				return
+			}
+
 			diff := pretty.Compare(iast, exp)
 			if diff == "" { // bonus
 				return
 			}
 			t.Errorf("test #%d: AST did not match expected", index)
 			// TODO: consider making our own recursive print function
-			t.Logf("test #%d:   actual: \n%s", index, spew.Sdump(iast))
+			t.Logf("test #%d:   actual: \n%s", index, lo.Sdump(iast))
-			t.Logf("test #%d: expected: \n%s", index, spew.Sdump(exp))
+			t.Logf("test #%d: expected: \n%s", index, lo.Sdump(exp))
 			t.Logf("test #%d: diff:\n%s", index, diff)
 		})
 	}
@@ -341,7 +416,7 @@ func TestInterpolateBasicStmt(t *testing.T) {
 		}
 		resName := &ExprCall{
 			Name: operatorFuncName,
-			// incorrect sig for this function, but correct interpolation
+			// incorrect sig for this function, and now invalid interpolation
 			Args: []interfaces.Expr{
 				&ExprStr{
 					V: "+",
@@ -370,11 +445,12 @@ func TestInterpolateBasicStmt(t *testing.T) {
 				},
 			},
 		}
+		_ = exp // historical
 		testCases = append(testCases, test{
 			name: "expanded invalid resource name",
 			ast:  ast,
-			fail: false,
+			fail: true,
-			exp:  exp,
+			//exp:  exp,
 		})
 	}
 
@@ -524,7 +600,7 @@ func TestInterpolateBasicExpr(t *testing.T) {
 	//}
 	{
 		ast := &ExprStr{
-			V: "sweetie${3.14159}", // invalid but only at type check
+			V: "sweetie${3.14159}", // invalid
 		}
 		exp := &ExprCall{
 			Name: operatorFuncName,
@@ -540,11 +616,11 @@ func TestInterpolateBasicExpr(t *testing.T) {
 				},
 			},
 		}
+		_ = exp // historical
 		testCases = append(testCases, test{
 			name: "float expansion",
 			ast:  ast,
-			fail: false,
+			fail: true,
-			exp:  exp,
 		})
 	}
 	{
@@ -566,11 +642,11 @@ func TestInterpolateBasicExpr(t *testing.T) {
 				},
 			},
 		}
+		_ = exp // historical
 		testCases = append(testCases, test{
 			name: "function expansion",
 			ast:  ast,
-			fail: false,
+			fail: true,
-			exp:  exp,
 		})
 	}
 	{
@@ -599,11 +675,11 @@ func TestInterpolateBasicExpr(t *testing.T) {
 				},
 			},
 		}
+		_ = exp // historical
 		testCases = append(testCases, test{
 			name: "function expansion arg",
 			ast:  ast,
-			fail: false,
+			fail: true,
-			exp:  exp,
 		})
 	}
 	// FIXME: i am broken, i don't deal well with negatives for some reason

lang/interpret_test.go

@@ -772,8 +772,7 @@ func TestAstFunc1(t *testing.T) {
 				return
 			}
 			if fail1 && err != nil {
-				// TODO: %+v instead?
+				s := err.Error() // convert to string
-				s := fmt.Sprintf("%s", err) // convert to string
 				if s != expstr {
 					t.Errorf("test #%d: FAIL", index)
 					t.Errorf("test #%d: expected different error", index)
@@ -839,8 +838,7 @@ func TestAstFunc1(t *testing.T) {
 				return
 			}
 			if fail2 && err != nil {
-				// TODO: %+v instead?
+				s := err.Error() // convert to string
-				s := fmt.Sprintf("%s", err) // convert to string
 				if s != expstr {
 					t.Errorf("test #%d: FAIL", index)
 					t.Errorf("test #%d: expected different error", index)
@@ -872,8 +870,7 @@ func TestAstFunc1(t *testing.T) {
 				return
 			}
 			if fail3 && err != nil {
-				// TODO: %+v instead?
+				s := err.Error() // convert to string
-				s := fmt.Sprintf("%s", err) // convert to string
 				if s != expstr {
 					t.Errorf("test #%d: FAIL", index)
 					t.Errorf("test #%d: expected different error", index)
@@ -897,8 +894,7 @@ func TestAstFunc1(t *testing.T) {
 				return
 			}
 			if fail4 && err != nil { // can't process graph if it's nil
-				// TODO: %+v instead?
+				s := err.Error() // convert to string
-				s := fmt.Sprintf("%s", err) // convert to string
 				if s != expstr {
 					t.Errorf("test #%d: FAIL", index)
 					t.Errorf("test #%d: expected different error", index)
@@ -977,6 +973,7 @@ func TestAstFunc2(t *testing.T) {
 	const magicError = "# err: "
 	const magicError1 = "err1: "
 	const magicError9 = "err9: " // TODO: rename
+	const magicError8 = "err8: " // TODO: rename
 	// TODO: move them all down by one
 	const magicError2 = "err2: "
 	const magicError3 = "err3: "
@@ -1013,6 +1010,7 @@ func TestAstFunc2(t *testing.T) {
 	type errs struct {
 		fail1 bool
 		fail9 bool // TODO: rename
+		fail8 bool // TODO: rename
 		// TODO: move them all down by one
 		fail2 bool
 		fail3 bool
@@ -1073,6 +1071,7 @@ func TestAstFunc2(t *testing.T) {
 		errStr := ""
 		fail1 := false
 		fail9 := false // TODO: rename
+		fail8 := false // TODO: rename
 		// TODO: move them all down by one
 		fail2 := false
 		fail3 := false
@@ -1093,6 +1092,11 @@ func TestAstFunc2(t *testing.T) {
 				str = errStr
 				fail9 = true
 			}
+			if strings.HasPrefix(str, magicError8) { // TODO: rename
+				errStr = strings.TrimPrefix(str, magicError8)
+				str = errStr
+				fail8 = true
+			}
 			// TODO: move them all down by one
 			if strings.HasPrefix(str, magicError2) {
 				errStr = strings.TrimPrefix(str, magicError2)
@@ -1130,6 +1134,7 @@ func TestAstFunc2(t *testing.T) {
 			errs: errs{
 				fail1: fail1,
 				fail9: fail9, // TODO: rename
+				fail8: fail8, // TODO: rename
 				// TODO: move them all down by one
 				fail2: fail2,
 				fail3: fail3,
@@ -1171,6 +1176,7 @@ func TestAstFunc2(t *testing.T) {
 			src := dir + path // location of the test
 			fail1 := errs.fail1
 			fail9 := errs.fail9 // TODO: rename
+			fail8 := errs.fail8 // TODO: rename
 			// TODO: move them all down by one
 			fail2 := errs.fail2
 			fail3 := errs.fail3
@@ -1252,8 +1258,7 @@ func TestAstFunc2(t *testing.T) {
 				return
 			}
 			if fail1 && err != nil {
-				// TODO: %+v instead?
+				s := err.Error() // convert to string
-				s := fmt.Sprintf("%s", err) // convert to string
 				if s != expstr {
 					t.Errorf("test #%d: FAIL", index)
 					t.Errorf("test #%d: expected different error", index)
@@ -1305,8 +1310,7 @@ func TestAstFunc2(t *testing.T) {
 				return
 			}
 			if fail9 && err != nil {
-				// TODO: %+v instead?
+				s := err.Error() // convert to string
-				s := fmt.Sprintf("%s", err) // convert to string
 				if s != expstr {
 					t.Errorf("test #%d: FAIL", index)
 					t.Errorf("test #%d: expected different error", index)
@@ -1322,9 +1326,24 @@ func TestAstFunc2(t *testing.T) {
 			}
 
 			iast, err := ast.Interpolate()
-			if err != nil {
+			if (!fail || !fail8) && err != nil {
 				t.Errorf("test #%d: FAIL", index)
-				t.Errorf("test #%d: interpolate failed with: %+v", index, err)
+				t.Errorf("test #%d: Interpolate failed with: %+v", index, err)
+				return
+			}
+			if fail8 && err != nil {
+				s := err.Error() // convert to string
+				if s != expstr {
+					t.Errorf("test #%d: FAIL", index)
+					t.Errorf("test #%d: expected different error", index)
+					t.Logf("test #%d: err: %s", index, s)
+					t.Logf("test #%d: exp: %s", index, expstr)
+				}
+				return // fail happened during lex parse, don't run init/interpolate!
+			}
+			if fail8 && err == nil {
+				t.Errorf("test #%d: FAIL", index)
+				t.Errorf("test #%d: Interpolate passed, expected fail", index)
 				return
 			}
 
@@ -1336,8 +1355,7 @@ func TestAstFunc2(t *testing.T) {
 				return
 			}
 			if fail2 && err != nil {
-				// TODO: %+v instead?
+				s := err.Error() // convert to string
-				s := fmt.Sprintf("%s", err) // convert to string
 				if s != expstr {
 					t.Errorf("test #%d: FAIL", index)
 					t.Errorf("test #%d: expected different error", index)
@@ -1369,8 +1387,7 @@ func TestAstFunc2(t *testing.T) {
 				return
 			}
 			if fail3 && err != nil {
-				// TODO: %+v instead?
+				s := err.Error() // convert to string
-				s := fmt.Sprintf("%s", err) // convert to string
 				if s != expstr {
 					t.Errorf("test #%d: FAIL", index)
 					t.Errorf("test #%d: expected different error", index)
@@ -1394,8 +1411,7 @@ func TestAstFunc2(t *testing.T) {
 				return
 			}
 			if fail4 && err != nil { // can't process graph if it's nil
-				// TODO: %+v instead?
+				s := err.Error() // convert to string
-				s := fmt.Sprintf("%s", err) // convert to string
 				if s != expstr {
 					t.Errorf("test #%d: FAIL", index)
 					t.Errorf("test #%d: expected different error", index)
@@ -1501,8 +1517,7 @@ func TestAstFunc2(t *testing.T) {
 				return
 			}
 			if fail5 && err != nil { // can't process graph if it's nil
-				// TODO: %+v instead?
+				s := err.Error() // convert to string
-				s := fmt.Sprintf("%s", err) // convert to string
 				if s != expstr {
 					t.Errorf("test #%d: FAIL", index)
 					t.Errorf("test #%d: expected different error", index)
@@ -1525,8 +1540,7 @@ func TestAstFunc2(t *testing.T) {
 				return
 			}
 			if fail6 && err != nil {
-				// TODO: %+v instead?
+				s := err.Error() // convert to string
-				s := fmt.Sprintf("%s", err) // convert to string
 				if s != expstr {
 					t.Errorf("test #%d: FAIL", index)
 					t.Errorf("test #%d: expected different error", index)

lang/interpret_test/TestAstFunc2/escaping1.output

@@ -0,0 +1,21 @@
+Vertex: test[A: ${test}]
+Vertex: test[B: $]
+Vertex: test[C: This is c1]
+Vertex: test[D: \$]
+Vertex: test[E: {}]
+Vertex: test[F: hello]
+Vertex: test[G: This is g1 EOF]
+Vertex: test[H: {hhh} EOF]
+Vertex: test[I: This is ii EOF]
+Vertex: test[J: $ is a dollar sign]
+Vertex: test[K: $ {zzz} EOF]
+Vertex: test[L: $$This is l1 EOF]
+Vertex: test[M: $ $$]
+Vertex: test[N: hello " world]
+Vertex: test[O: hello "" world]
+Vertex: test[P: hello \ world]
+Vertex: test[Q: hello \\ world]
+Vertex: test[R: \This is r1 EOF]
+Vertex: test[S: \$ EOF]
+Vertex: test[T: newline
+EOF]

lang/interpret_test/TestAstFunc2/escaping1/main.mcl

@@ -0,0 +1,68 @@
+# escaping examples
+
+test "A: \${test}" {}
+
+test "B: \$" {}
+
+$c1 = "This is c1"
+test "C: ${c1}" {}
+
+test "D: \\\$" {}
+
+test "E: {}" {}
+
+test "F: hello" {}
+
+$g1 = "This is g1"
+test "G: ${g1} EOF" {}
+
+test "H: {hhh} EOF" {}
+
+$i_i = "This is ii"
+test "I: ${i_i} EOF" {}
+
+# is this okay?
+test "J: $ is a dollar sign" {}
+
+test "K: $ {zzz} EOF" {}
+
+$l1 = "This is l1"
+test "L: $$${l1} EOF" {}
+
+test "M: $ $$" {}
+
+test "N: hello \" world" {}
+
+test "O: hello \"\" world" {}
+
+test "P: hello \\ world" {}
+
+test "Q: hello \\\\ world" {}
+
+$r1 = "This is r1"
+test "R: \\${r1} EOF" {}
+
+test "S: \\$ EOF" {}
+
+test "T: newline\nEOF" {}
+
+# XXX: possible bugs or misunderstood expectations:
+
+#test "W: \\$" {}
+# got: <error>
+# exp: W: \$
+
+#$x1 = "This is x1"
+#test "X: $${x1} EOF" {}
+# got: X: $${x1} EOF
+# exp: X: $This is x1 EOF
+
+#$unused = "i am unused"
+#$y1 = "{unused}"
+#test "Y: $${y1} EOF" {} # check there isn't double parsing
+# got: Y: $${y1} EOF
+# exp: Y: ${unused} EOF
+
+#test "Z: $$$" {}
+# got: <error>
+# exp: Z: $$$ EOF

lang/interpret_test/TestAstFunc2/escaping2.output

@@ -0,0 +1 @@
+# err: err8: parser failed: cannot parse string: X: ${foo()}

lang/interpret_test/TestAstFunc2/escaping2/main.mcl

@@ -0,0 +1,2 @@
+# this is invalid (you can't have a function inside a var lookup)
+test "X: ${foo()}" {}

lang/interpret_test/TestAstFunc2/escaping3.output

@@ -0,0 +1 @@
+# err: err8: parser failed: cannot parse string: X: ${}

lang/interpret_test/TestAstFunc2/escaping3/main.mcl

@@ -0,0 +1,2 @@
+# this is invalid (you can't have an empty var lookup)
+test "X: ${}" {}

lang/interpret_test/TestAstFunc2/escaping4.output

@@ -0,0 +1 @@
+# err: err8: parser failed: cannot parse string: X: \z

lang/interpret_test/TestAstFunc2/escaping4/main.mcl

@@ -0,0 +1,2 @@
+# this is invalid (you can't escape a z, right?)
+test "X: \z" {}

lang/interpret_test/TestAstFunc2/escaping5.output

@@ -0,0 +1 @@
+# err: err8: parser failed: cannot parse string: X: there is no \j sequence

lang/interpret_test/TestAstFunc2/escaping5/main.mcl

@@ -0,0 +1,2 @@
+# this is invalid (there's no \j escape sequence)
+test "X: there is no \j sequence" {}

lang/lang_test.go

@@ -224,7 +224,7 @@ func TestInterpret4(t *testing.T) {
 			# comment 3
 			stringptr => "the actual field name is: StringPtr", # comment 4
 			int8ptr => 99, # comment 5
-			comment => "☺\thello\u263a\nwo\"rld\\2\u263a", # must escape these
+			comment => "☺\thello\nwo\"rld\\2", # must escape these
 		}
 	`
 	graph, err := runInterpret(t, code)
@@ -244,7 +244,7 @@ func TestInterpret4(t *testing.T) {
 	x.StringPtr = &stringptr
 	int8ptr := int8(99)
 	x.Int8Ptr = &int8ptr
-	x.Comment = "☺\thello☺\nwo\"rld\\2\u263a" // must escape the escaped chars
+	x.Comment = "☺\thello\nwo\"rld\\2" // must escape the escaped chars
 
 	expected := &pgraph.Graph{}
 	expected.AddVertex(x)

lang/lexer.nex

@@ -242,26 +242,13 @@
 			yylex.pos(lval) // our pos
 			s := yylex.Text()
 
-			x, err := strconv.Unquote(s) // expects quote wrapping!
+			if s[0:1] != "\"" || s[len(s)-1:] != "\"" {
-			if err == nil {
-				lval.str = x // it comes out all cleanly escaped
-			} else if err == strconv.ErrSyntax {
-				// this catches improper escape codes or lone \
-				lp := yylex.cast()
-				lp.lexerErr = &LexParseErr{
-					Err: ErrLexerStringBadEscaping,
-					Str: s,
-					Row: yylex.Line(),
-					Col: yylex.Column(),
-				}
-				return ERROR
-			} else if err != nil {
 				// unhandled error
-				panic(fmt.Sprintf("error lexing STRING, got: %v", err))
+				panic(fmt.Sprintf("error lexing STRING, got: %s", s))
-			} else { // no chars to escape found
+				//return ERROR // unreachable
-				lval.str = s[1:len(s)-1] // remove the two quotes
 			}
 
+			lval.str = s[1:len(s)-1] // remove the two quotes
 			return STRING
 		}
 /\-?[0-9]+/

lang/lexparse_test.go

@@ -94,6 +94,17 @@ func TestLexParse0(t *testing.T) {
 			fail: true,
 		})
 	}
+	{
+		testCases = append(testCases, test{
+			name: "bad escaping 2",
+			code: `
+			test "t1" {
+				str => "he\\ llo", # incorrect escaping
+			}
+			`,
+			fail: true,
+		})
+	}
 	{
 		testCases = append(testCases, test{
 			name: "int overflow",

lang/structs.go

@@ -8353,7 +8353,9 @@ func (obj *ExprVar) Apply(fn func(interfaces.Node) error) error { return fn(obj)
 
 // Init initializes this branch of the AST, and returns an error if it fails to
 // validate.
-func (obj *ExprVar) Init(*interfaces.Data) error { return nil }
+func (obj *ExprVar) Init(*interfaces.Data) error {
+	return langutil.ValidateVarName(obj.Name)
+}
 
 // Interpolate returns a new node (aka a copy) once it has been expanded. This
 // generally increases the size of the AST when it is used. It calls Interpolate

lang/util/util.go

@@ -19,8 +19,11 @@ package util
 
 import (
 	"fmt"
+	"regexp"
+	"strings"
 
 	"github.com/purpleidea/mgmt/lang/types"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // HasDuplicateTypes returns an error if the list of types is not unique.
@@ -89,3 +92,38 @@ func FnMatch(typ *types.Type, fns []*types.FuncValue) (int, error) {
 
 	return 0, fmt.Errorf("unable to find a compatible function for type: %+v", typ)
 }
+
+// ValidateVarName returns an error if the string pattern does not match the
+// format for a valid variable name. The leading dollar sign must not be passed
+// in.
+func ValidateVarName(name string) error {
+	if name == "" {
+		return fmt.Errorf("got empty var name")
+	}
+
+	// A variable always starts with an lowercase alphabetical char and
+	// contains lowercase alphanumeric characters or numbers, underscores,
+	// and non-consecutive dots. The last char must not be an underscore or
+	// a dot.
+	// TODO: put the variable matching pattern in a const somewhere?
+	pattern := `^[a-z]([a-z0-9_]|(\.|_)[a-z0-9_])*$`
+
+	matched, err := regexp.MatchString(pattern, name)
+	if err != nil {
+		return errwrap.Wrapf(err, "error matching regex")
+	}
+	if !matched {
+		return fmt.Errorf("invalid var name: `%s`", name)
+	}
+
+	// Check that we don't get consecutive underscores or dots!
+	// TODO: build this into the above regexp and into the parse.rl file!
+	if strings.Contains(name, "..") {
+		return fmt.Errorf("var name contains multiple periods: `%s`", name)
+	}
+	if strings.Contains(name, "__") {
+		return fmt.Errorf("var name contains multiple underscores: `%s`", name)
+	}
+
+	return nil
+}

lang/util/util_test.go

@@ -0,0 +1,82 @@
+// Mgmt
+// Copyright (C) 2013-2021+ James Shubin and the project contributors
+// Written by James Shubin <james@shubin.ca> and the project contributors
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package util
+
+import (
+	"fmt"
+	"sort"
+	"testing"
+)
+
+func TestValidateVarName(t *testing.T) {
+	testCases := map[string]error{
+		"":        fmt.Errorf("got empty var name"),
+		"hello":   nil,
+		"NOPE":    fmt.Errorf("invalid var name: `NOPE`"),
+		"$foo":    fmt.Errorf("invalid var name: `$foo`"),
+		".":       fmt.Errorf("invalid var name: `.`"),
+		"..":      fmt.Errorf("invalid var name: `..`"),
+		"_":       fmt.Errorf("invalid var name: `_`"),
+		"__":      fmt.Errorf("invalid var name: `__`"),
+		"0":       fmt.Errorf("invalid var name: `0`"),
+		"1":       fmt.Errorf("invalid var name: `1`"),
+		"42":      fmt.Errorf("invalid var name: `42`"),
+		"X":       fmt.Errorf("invalid var name: `X`"),
+		"x":       nil,
+		"x0":      nil,
+		"x1":      nil,
+		"x42":     nil,
+		"x42.foo": nil,
+		"x42_foo": nil,
+
+		// XXX: fix these test cases
+		//"x.": fmt.Errorf("invalid var name: x."),
+		//"x_": fmt.Errorf("invalid var name: x_"),
+	}
+
+	keys := []string{}
+	for k := range testCases {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+	for _, k := range keys {
+		e, ok := testCases[k]
+		if !ok {
+			// programming error
+			t.Errorf("missing test case: %s", k)
+			continue
+		}
+
+		err := ValidateVarName(k)
+		if err == nil && e == nil {
+			continue
+		}
+		if err == nil && e != nil {
+			t.Errorf("key: %s did not error, expected: %s", k, e.Error())
+			continue
+		}
+		if err != nil && e == nil {
+			t.Errorf("key: %s expected no error, got: %s", k, err.Error())
+			continue
+		}
+		if err.Error() != e.Error() {
+			t.Errorf("key: %s did not have correct error, expected: %s", k, err.Error())
+			continue
+		}
+	}
+}

misc/make-deps.sh

@@ -50,6 +50,7 @@ if [ -n "$YUM" ]; then
 	$sudo_command $YUM install -y augeas-devel
 	$sudo_command $YUM install -y ruby-devel rubygems
 	$sudo_command $YUM install -y time
+	$sudo_command $YUM install -y ragel
 	# dependencies for building packages with fpm
 	$sudo_command $YUM install -y gcc make rpm-build libffi-devel bsdtar mkosi || true
 	$sudo_command $YUM install -y graphviz || true # for debugging
@@ -59,6 +60,7 @@ if [ -n "$APT" ]; then
 	$sudo_command $APT install -y libaugeas-dev || true
 	$sudo_command $APT install -y ruby ruby-dev || true
 	$sudo_command $APT install -y libpcap0.8-dev || true
+	$sudo_command $APT install -y ragel || true
 	# dependencies for building packages with fpm
 	$sudo_command $APT install -y build-essential rpm bsdtar || true
 	# `realpath` is a more universal alternative to `readlink -f` for absolute path resolution
@@ -73,11 +75,11 @@ fi
 # Prevent linuxbrew installing redundant deps in CI
 if [ -n "$BREW" -a "$RUNNER_OS" != "Linux" ]; then
 	# coreutils contains gtimeout, gstat, etc
-	$BREW install pkg-config libvirt augeas coreutils || true
+	$BREW install pkg-config libvirt augeas coreutils ragel || true
 fi
 
 if [ -n "$PACMAN" ]; then
-	$sudo_command $PACMAN -S --noconfirm --asdeps --needed libvirt augeas rubygems libpcap
+	$sudo_command $PACMAN -S --noconfirm --asdeps --needed libvirt augeas rubygems libpcap ragel
 fi
 fold_end "Install dependencies"
 
@@ -104,6 +106,24 @@ if ! in_ci; then
 	fi
 fi
 
+if in_ci; then
+	# TODO: consider bumping to new package manager version
+	RAGEL_VERSION='6.10'	# current stable version
+	RAGEL_TMP='/tmp/ragel/'
+	RAGEL_FILE="${RAGEL_TMP}ragel-${RAGEL_VERSION}.tar.gz"
+	RAGEL_DIR="${RAGEL_TMP}ragel-${RAGEL_VERSION}/"
+	mkdir -p "$RAGEL_TMP"
+	cd "$RAGEL_TMP"
+	wget "https://www.colm.net/files/ragel/ragel-${RAGEL_VERSION}.tar.gz" -O "$RAGEL_FILE"
+	tar -xvf "$RAGEL_FILE"
+	cd -
+	cd "$RAGEL_DIR"
+	./configure --prefix=/usr/local --disable-manual
+	make
+	sudo make install
+	cd -
+fi
+
 # attempt to workaround old ubuntu
 if [ -n "$APT" -a "$goversion" -lt "$mingoversion" ]; then
 	echo "install golang from a ppa."

test/comment_parser.go

@@ -24,6 +24,7 @@ import (
 	"go/token"
 	"log"
 	"os"
+	"regexp"
 	"strings"
 	"unicode"
 )
@@ -250,6 +251,9 @@ func IsNewStart(word string) bool {
 	if word == "*" { // bullets
 		return true
 	}
+	if IsNumberBullet(word) {
+		return true
+	}
 
 	return false
 }
@@ -269,3 +273,12 @@ func IsSpecialLine(line string) bool {
 
 	return false
 }
+
+// IsNumberBullet returns true if the word starts with a number bullet like 42).
+func IsNumberBullet(word string) bool {
+	matched, err := regexp.MatchString(`[0-9]+\)*`, word)
+	if err != nil {
+		return false
+	}
+	return matched
+}

test/shell/exchange0.mcl

@@ -11,8 +11,9 @@ import "world"
 
 $rand = random1(8)
 $exchanged = world.exchange("keyns", $rand)
+$host = sys.hostname()
 
-file "/tmp/mgmt/exchange-${sys.hostname()}" {
+file "/tmp/mgmt/exchange-${host}" {
 	state => $const.res.file.state.exists,
 	content => template("Found: {{ . }}\n", $exchanged),
 }

test/test-gometalinter.sh

@@ -49,6 +49,7 @@ gml="$gml --exclude=lang/lexer.nn.go"
 gml="$gml --exclude=lang/y.go"
 gml="$gml --exclude=bindata/bindata.go"
 gml="$gml --exclude=lang/types/kind_stringer.go"
+gml="$gml --exclude=lang/interpolate/parse.generated.go"
 
 gometalinter="$gml"	# final
 echo "Using: $gometalinter"

test/test-govet.sh

@@ -113,6 +113,10 @@ function reflowed-comments() {
 		return 0
 	fi
 
+	if [ "$1" = './lang/interpolate/parse.generated.go' ]; then
+		return 0
+	fi
+
 	./test/comment_parser "$1"
 }
 

test/test-headerfmt.sh

@@ -16,7 +16,7 @@ while IFS='' read -r line; do	# find what header should look like
 done < "$FILE"
 
 find_files() {
-	git ls-files | grep '\.go$' | grep -v '^examples/' | grep -v '^test/'
+	git ls-files | grep -E '\.go$|\.rl$' | grep -v '^examples/' | grep -v '^test/'
 }
 
 bad_files=$(