Repository: mvdan/sh (go)

Issue:
interp: Params isn't supported as an option to New
First of all, great project. We're tentatively planning to use it in an open project we'll be launching in two weeks (an automation tool with a twist) as syntax for our Makefile-like tasks. There are plenty of opportunities there as opposed to using bash directly, including better error messages and even debugging!

While integrating it with our tool, I noticed a problem with `Params`. While I was working on a fix, tests I added uncovered another issue. 

PR: https://github.com/mvdan/sh/pull/364

I hope you'll find it not entirely worthless and can merge it, it's vital for us here, would hate to have to create a fork. :) 

Pull request description:
interp: fix using Params as an option to New
As correctly pointed out by Marcin Bilski, using the exported function
in that way would lead to a panic. And, even after working around the
panic, any shell flags used would be overwritten by the first implicit
call to Reset.

To fix the crash, move the updateExpandOpts method call to the end of
the "set" builtin. After all, it's only needed there; the Run function
already ends up calling the method later via fillExpandConfig.

To keep the shell options from being deleted in Reset, simply whitelist
them from the list of fields to clear. We do that for Params, so it
follows that the same should happen for the opts field.

Finally, add a couple of tests covering both scenarios to
TestRunnerOpts.

Fixes #365.

Patch:
diff --git a/interp/builtin.go b/interp/builtin.go
index d4a44abb..0fad50c4 100644
--- a/interp/builtin.go
+++ b/interp/builtin.go
@@ -70,6 +70,7 @@ func (r *Runner) builtinCode(ctx context.Context, pos syntax.Pos, name string, a
 			r.errf("set: %v\n", err)
 			return 2
 		}
+		r.updateExpandOpts()
 	case "shift":
 		n := 1
 		switch len(args) {
diff --git a/interp/interp.go b/interp/interp.go
index d6ec8189..151dab27 100644
--- a/interp/interp.go
+++ b/interp/interp.go
@@ -262,7 +262,6 @@ func Params(args ...string) func(*Runner) error {
 			args = args[1:]
 		}
 		r.Params = args
-		r.updateExpandOpts()
 		return nil
 	}
 }
@@ -501,6 +500,7 @@ func (r *Runner) Reset() {
 		Exec:        r.Exec,
 		Open:        r.Open,
 		KillTimeout: r.KillTimeout,
+		opts:        r.opts,
 
 		// emptied below, to reuse the space
 		Vars:      r.Vars,
diff --git a/syntax/nodes.go b/syntax/nodes.go
index 8527af97..e20d13b2 100644
--- a/syntax/nodes.go
+++ b/syntax/nodes.go
@@ -756,8 +756,12 @@ func (a *ArrayExpr) Pos() Pos { return a.Lparen }
 func (a *ArrayExpr) End() Pos { return posAddCol(a.Rparen, 1) }
 
 // ArrayElem represents a Bash array element.
+//
+// Index can be nil; for example, declare -a x=(value).
+// Value can be nil; for example, declare -A x=([index]=).
+// Finally, neither can be nil; for example, declare -A x=([index]=value)
 type ArrayElem struct {
-	Index    ArithmExpr // [i]=, ["k"]=
+	Index    ArithmExpr
 	Value    *Word
 	Comments []Comment
 }
@@ -768,7 +772,12 @@ func (a *ArrayElem) Pos() Pos {
 	}
 	return a.Value.Pos()
 }
-func (a *ArrayElem) End() Pos { return a.Value.End() }
+func (a *ArrayElem) End() Pos {
+	if a.Value != nil {
+		return a.Value.End()
+	}
+	return posAddCol(a.Index.Pos(), 1)
+}
 
 // ExtGlob represents a Bash extended globbing expression. Note that these are
 // parsed independently of whether shopt has been called or not.
diff --git a/syntax/parser.go b/syntax/parser.go
index 298d9313..7fa46873 100644
--- a/syntax/parser.go
+++ b/syntax/parser.go
@@ -1609,11 +1609,16 @@ func (p *Parser) getAssign(needEqual bool) *Assign {
 				p.follow(left, `"[x]"`, assgn)
 			}
 			if ae.Value = p.getWord(); ae.Value == nil {
-				if p.tok == leftParen {
+				switch p.tok {
+				case leftParen:
 					p.curErr("arrays cannot be nested")
+					return nil
+				case _Newl, rightParen, leftBrack:
+					// TODO: support [index]=[
+				default:
+					p.curErr("array element values must be words")
+					break
 				}
-				p.curErr("array element values must be words")
-				break
 			}
 			if len(p.accComs) > 0 {
 				c := p.accComs[0]
diff --git a/syntax/printer.go b/syntax/printer.go
index f68e8656..4419e40f 100644
--- a/syntax/printer.go
+++ b/syntax/printer.go
@@ -788,7 +788,9 @@ func (p *Printer) elemJoin(elems []*ArrayElem, last []Comment) {
 		if p.wroteIndex(el.Index) {
 			p.WriteByte('=')
 		}
-		p.word(el.Value)
+		if el.Value != nil {
+			p.word(el.Value)
+		}
 		p.comments(left...)
 	}
 	if len(last) > 0 {
diff --git a/syntax/walk.go b/syntax/walk.go
index a67be8c6..d3f6332e 100644
--- a/syntax/walk.go
+++ b/syntax/walk.go
@@ -201,7 +201,9 @@ func Walk(node Node, f func(Node) bool) {
 		if x.Index != nil {
 			Walk(x.Index, f)
 		}
-		Walk(x.Value, f)
+		if x.Value != nil {
+			Walk(x.Value, f)
+		}
 	case *ExtGlob:
 		Walk(x.Pattern, f)
 	case *ProcSubst:


Test patch:
diff --git a/interp/interp_test.go b/interp/interp_test.go
index 3bdebbd4..55918325 100644
--- a/interp/interp_test.go
+++ b/interp/interp_test.go
@@ -2743,6 +2743,16 @@ func TestRunnerOpts(t *testing.T) {
 			"[[ $PWD == foo ]]",
 			"exit status 1",
 		},
+		{
+			opts(Params("foo")),
+			"echo $@",
+			"foo\n",
+		},
+		{
+			opts(Params("-u", "--", "foo")),
+			"echo $@; echo $unset",
+			"foo\nunset: unbound variable\nexit status 1",
+		},
 	}
 	p := syntax.NewParser()
 	for i, c := range cases {
diff --git a/syntax/filetests_test.go b/syntax/filetests_test.go
index 87d84579..0fa438df 100644
--- a/syntax/filetests_test.go
+++ b/syntax/filetests_test.go
@@ -4008,6 +4008,19 @@ var fileTests = []testCase{
 			}}},
 		}}},
 	},
+	{
+		Strs: []string{
+			"a=([x]= [y]=)",
+			"a=(\n[x]=\n[y]=\n)",
+		},
+		bash: &CallExpr{Assigns: []*Assign{{
+			Name: lit("a"),
+			Array: &ArrayExpr{Elems: []*ArrayElem{
+				{Index: litWord("x")},
+				{Index: litWord("y")},
+			}},
+		}}},
+	},
 	{
 		Strs:   []string{"a]b"},
 		common: litStmt("a]b"),
@@ -4579,7 +4592,9 @@ func clearPosRecurse(tb testing.TB, src string, v interface{}) {
 		if x.Index != nil {
 			recurse(x.Index)
 		}
-		recurse(x.Value)
+		if x.Value != nil {
+			recurse(x.Value)
+		}
 	case *ExtGlob:
 		setPos(&x.OpPos, x.Op.String())
 		checkSrc(posAddCol(x.End(), -1), ")")