Repository: mvdan/sh
Pull request #480 (2020-01-17 16:19:40)

interp: rename sub to Subshell, and fix some bugs
Copy makes a copy of the given Runner, suitable for use in parallel to the original.  The copy will have the same parameters, environment variables, etc, but they can all be modified without affecting the original.

Also fixes a tiny typo in a comment in the definition of Runner ("sub" vs "Sub").

Edit/clarification: This PR started life adding a `Copy` method on `Runner`; only later was it updated to rename `sub` to `Subshell` and fix some bugs in it that `Copy` had fixed.

theclapp: I'm a little unsure about Runner.Env.  I just copied it.  If Env is read-only, that's fine, but I couldn't tell if that's actually the case.
mvdan: Could you expand a bit on your use case here?

I'm a bit unsure about adding API to do a "deep copy" of sorts. We have `sub` for spawning subshells, which I'm sure you've seen, but that's different - it's named after its purpose, and doesn't attempt to be a generic copy method.
theclapp: I'm using sh/interp to write a graphical shell of sorts.  Each command run should inherit the environment (pwd, functions, env vars, etc) from the previously run command.  _But also_ each command needs to be prepared to be backgrounded at will (similar to `^Z bg` in a regular shell with job control).  So when I run a command, I want to copy the previous Runner and use that to run the new command.  If that new command runs to completion and isn't backgrounded, I'll use its Runner for the "source" Runner of the next command.  But if it _is_ backgrounded, then I want to use the _previous_ Runner for the "source" of the next command.

If I don't copy the Runner, then I'd end up using the same one twice (or more!), concurrently.  Bad idea.

I tried doing a shallow copy (`newR := *r`) and then doing a deeper copy of the appropriate public fields (Funcs and so on) (and that seemed to work okay), but then you added `alias` (unexported), and I thought that perhaps something a bit more internal would be better.  And in the process saw that my previous code was good but incomplete, so I'm glad I did.

(If you say "TIL about `^Z bg`", well, I'm gonna laugh. 😆  But for those that don't know, it's like using `&` to run a command in background (thus "bg") except after the program is already running.)

I don't think `Runner.sub()` is sufficient (that is, renaming `sub()` to `Copy()` wouldn't work for this), because I don't think I could use the two Runners concurrently, and also I think my `Copy` and your `sub` perform subtly different copies — which are, to be clear, appropriate for their subtly different purposes!  But still different.
kaey: I couldn't fully understand your use case, but to me that sounds like you want proper job control.
mvdan: Thanks for the explanation.

For the record, I do think we should export the `alias` map somehow, similar to what we did with `Funcs`. I'm just not quite ready to do that right now, given that I might change the implementation before I consider it finished.

> I don't think `Runner.sub()` is sufficient (that is, renaming `sub()` to `Copy()` wouldn't work for this), because I don't think I could use the two Runners concurrently

Hmm, I disagree:

```
func (r *Runner) stmt(ctx context.Context, st *syntax.Stmt) {
        if r.stop(ctx) {
                return
        }
        if st.Background {
                r2 := r.sub()
                st2 := *st
                st2.Background = false
                r.bgShells.Go(func() error {
                        return r2.Run(ctx, &st2)
                })
        } else {
                r.stmtSync(ctx, st)
        }
}
```

As you can see, we use it for `&`, and we do use both shells concurrently. If that's racy or buggy, then we just have a bug to fix.

The only question left is the semantics. Is spawning a sub-shell the same thing as spawning a background-shell? `interp` itself uses `Runner.sub` for both `sub-cmd &` and `foo $(sub-cmd)`, so we do use the exact same semantics. Wouldn't the same apply to you? Or am I missing something here?

So yes, I am leaning towards exposing `Runner.sub`, probably under a better name like `Subshell`.
mvdan: Here's bits from `man bash` that seem to support my idea that background shells are the same thing as subshells:

> Each command in a pipeline is executed as a separate process (i.e., in a subshell)

> If a command is terminated by the control operator &, the shell executes the command in the background in a subshell.

My only last thought on the matter is that, if `Runner.Subshell` is not enough, we probably want API to better support job control, like @kaey mentioned. But I hope we don't need extra API for that.
theclapp: > For the record, I do think we should export the alias map somehow, similar to what we did with Funcs. I'm just not quite ready to do that right now, given that I might change the implementation before I consider it finished.

Okay.

> If that's racy or buggy, then we just have a bug to fix.

I think we do have a bug, then.  `Runner.sub` does not, for example, make a deep-enough copy of Runner.Funcs.  Below, I have a function `f` which I'm running in a background shell, and the foreground shell alters `f`, and this also alters the background version.

```sh
% gosh
$ function f { echo 1 ; }
$ function g { function f { echo 2 ; } ; }

# show that f & g do what's expected
$ f
1
$ g
$ f
2

# redefine f again
$ function f { echo 1 ; }

# start a background shell using f
$ ( for (( n=0; n < 5; n++ )) ; do echo "$(date): f is $(f)" ; sleep 5 ; done ; echo done; ) &
$ Sat Jan 18 15:11:54 EST 2020: f is 1

# foreground f still the same (of course)
$ f
1
$
$ Sat Jan 18 15:11:59 EST 2020: f is 1

# run g, which redefines the foreground f (and, as it turns out, the background version too)
$ g

# run foreground f
$ f
2

# ... wait for the background loop ...
$
$ Sat Jan 18 15:12:04 EST 2020: f is 2  # f is (incorrectly) 2!
Sat Jan 18 15:12:09 EST 2020: f is 2
Sat Jan 18 15:12:14 EST 2020: f is 2
done
```

Note specifically the last three lines where the background shell prints "f is 2".  So the foreground and background `Runners` share the same definition of `f`.  Bash, suffice to say, doesn't do this.  (It prints `f is 1` in the background thread the whole time.)

`sub` needs to make a deeper copy of Funcs, and possibly a deeper copy of `Vars`.  (It already makes a new map, but wouldn't it want to make deep copies of each `Var`?  IIUC expand.Variable.Map can be modified in-place (e.g. [here](https://github.com/mvdan/sh/blob/d96c17acdda1c43fb4d99adf0c60a0da0ef6dbc4/interp/vars.go#L206)).)  Probably some/all of the others my `Copy()` does, too.

> Wouldn't the same apply to you?

I think it would, yes.

> So yes, I am leaning towards exposing Runner.sub, probably under a better name like Subshell.

Yeah, I think that'd work for me, too (once we fix the above bugs).  I think I'd prefer a name like `Copy` since I wouldn't be using it as a sub-shell _per se_.  But I could live with `Subshell`.

Regarding job control, I'll be doing that more or less outside the shell code.  If you want to implement it, I won't object, but on the other hand I don't expect to be using it, either.
mvdan: > I think we do have a bug, then. `Runner.sub` does not, for example, make a deep-enough copy of Runner.Funcs.

Yup, you're right! The fix for master was four lines, so I've done it there with a small test case, giving attribution. I hope that's OK with you.

> `sub` needs to make a deeper copy of Funcs, and possibly a deeper copy of `Vars`. (It already makes a new map, but wouldn't it want to make deep copies of each `Var`? IIUC expand.Variable.Map can be modified in-place (e.g. [here](https://github.com/mvdan/sh/blob/d96c17acdda1c43fb4d99adf0c60a0da0ef6dbc4/interp/vars.go#L206)).)

I think you are right. Do you want to provide a patch? Writing a test case should be pretty simple; you can follow what I've done with the fix for funcs. Unfortunately, I don't know of a built-in way for subshells to synchronize with their parent shell (such as one would do with channels or waitgroups in Go), so I had to use millisecond sleeps. That's not great, but as long as it's only a few test cases using <10ms sleeps, I don't think it matters too much. And the bug would still get caught the vast majority of the time.

If you don't have time for a patch, no worries - just file an issue and I can take a look.

> Probably some/all of the others my `Copy()` does, too.

I think your code is fine for the most part, but I'd prefer to keep the design of `Runner.sub`. In particular, note how it zeroes all fields except a few that we explicitly keep. Your `r2 := *r` is comparatively harder to maintain in the future. It's far too easy to share too much and introduce races.

I do agree that your function does some things better, though. So I'll be happy to accept those as patches.

> I think I'd prefer a name like `Copy` since I wouldn't be using it as a sub-shell _per se_. But I could live with `Subshell`.

I'm not set on the name, but I don't like `Copy` because that, to me, says "a full deep copy". Which is not what we're doing here. For example, in the future we might want to do lazy copies of the vars and funcs maps, since the vast majority of subshells will not be modifying those, and we could save allocations.

I think `Subshell` is the closest coined term here. As I showed before, `man bash` refers to background shells as `subshells` too. Isn't that the same stuff you're doing with job control?

The only other name that occurs to me is the likes of `Fork` or `Child`, but these terms don't really make much sense in either Shell or Go.
mvdan: > I'm not set on the name, but I don't like `Copy` because that, to me, says "a full deep copy". Which is not what we're doing here.

I forgot the best example - plenty of unexported fields are zeroed, and not copied. So, at best, we could call the method `PartialCopy` or `ExternalCopy`, neither of which seem great either.
theclapp: I'll work on a patch, and update this PR in-place, if that's okay.

`SubShell` works for me ... though if we can (now or later) come up with a more descriptive name for how you and I are both using it, I'd be up for that, too.  Perhaps more usage will spark inspiration.

I'll use the design of `Runner.sub()`.
mvdan: > I'll work on a patch, and update this PR in-place, if that's okay.

Sure!

> a more descriptive name for how you and I are both using it

Makes sense. The only blocker would be a v3.1.0 release, which I'd like to put out sometime around March. So we can expose `Subshell` for now, and start using it from other modules, reserving the power to rename it before it's set in stone in a stable tag.
theclapp: Please take another look.

Note that all the tests I added fail for current master.
theclapp: Squashed 'em all.  The final change seems surprisingly short, tbh.  :)  So left the final commit message short and sweet, too.
mvdan: Thanks again for your help! This was really helpful - it uncovered three bugs in the process.

Patch:
diff --git a/interp/interp.go b/interp/interp.go
index 1f5f2222..000fb1fa 100644
--- a/interp/interp.go
+++ b/interp/interp.go
@@ -87,7 +87,7 @@ func (r *Runner) fillExpandConfig(ctx context.Context) {
 				_, err = io.Copy(w, f)
 				return err
 			}
-			r2 := r.sub()
+			r2 := r.Subshell()
 			r2.stdout = w
 			r2.stmts(ctx, cs.Stmts)
 			return r2.err
@@ -108,7 +108,7 @@ func (r *Runner) fillExpandConfig(ctx context.Context) {
 			if err := mkfifo(path, 0666); err != nil {
 				return "", err
 			}
-			r2 := r.sub()
+			r2 := r.Subshell()
 			stdout := r.origStdout
 			r.wgProcSubsts.Add(1)
 			go func() {
@@ -402,7 +402,7 @@ type Runner struct {
 	stderr io.Writer
 
 	ecfg *expand.Config
-	ectx context.Context // just so that Runner.Sub can use it again
+	ectx context.Context // just so that Runner.Subshell can use it again
 
 	// didReset remembers whether the runner has ever been reset. This is
 	// used so that Reset is automatically called when running any program
@@ -753,7 +753,7 @@ func (r *Runner) stmt(ctx context.Context, st *syntax.Stmt) {
 		return
 	}
 	if st.Background {
-		r2 := r.sub()
+		r2 := r.Subshell()
 		st2 := *st
 		st2.Background = false
 		r.bgShells.Go(func() error {
@@ -799,7 +799,17 @@ func (r *Runner) stmtSync(ctx context.Context, st *syntax.Stmt) {
 	}
 }
 
-func (r *Runner) sub() *Runner {
+// Subshell makes a copy of the given Runner, suitable for use concurrently
+// with the original.  The copy will have the same environment, including
+// variables and functions, but they can all be modified without affecting the
+// original.
+//
+// Subshell is not safe to use concurrently with Run.  Orchestrating this is
+// left up to the caller; no locking is performed.
+//
+// To replace e.g. stdin/out/err, do StdIO(r.stdin, r.stdout, r.stderr)(r) on
+// the copy.
+func (r *Runner) Subshell() *Runner {
 	// Keep in sync with the Runner type. Manually copy fields, to not copy
 	// sensitive ones like errgroup.Group, and to do deep copies of slices.
 	r2 := &Runner{
@@ -813,12 +823,23 @@ func (r *Runner) sub() *Runner {
 		stderr:      r.stderr,
 		filename:    r.filename,
 		opts:        r.opts,
+		usedNew:     r.usedNew,
 
 		origStdout: r.origStdout, // used for process substitutions
 	}
 	r2.Vars = make(map[string]expand.Variable, len(r.Vars))
 	for k, v := range r.Vars {
-		r2.Vars[k] = v
+		v2 := v
+		// Make deeper copies of List and Map, but ensure that they remain nil
+		// if they are nil in v.
+		v2.List = append([]string(nil), v.List...)
+		if v.Map != nil {
+			v2.Map = make(map[string]string, len(v.Map))
+			for k, v := range v.Map {
+				v2.Map[k] = v
+			}
+		}
+		r2.Vars[k] = v2
 	}
 	r2.funcVars = make(map[string]expand.Variable, len(r.funcVars))
 	for k, v := range r.funcVars {
@@ -832,6 +853,13 @@ func (r *Runner) sub() *Runner {
 	for k, v := range r.Funcs {
 		r2.Funcs[k] = v
 	}
+	if l := len(r.alias); l > 0 {
+		r2.alias = make(map[string]alias, l)
+		for k, v := range r.alias {
+			r2.alias[k] = v
+		}
+	}
+
 	r2.dirStack = append(r2.dirBootstrap[:0], r.dirStack...)
 	r2.fillExpandConfig(r.ectx)
 	r2.didReset = true
@@ -846,7 +874,7 @@ func (r *Runner) cmd(ctx context.Context, cm syntax.Command) {
 	case *syntax.Block:
 		r.stmts(ctx, x.Stmts)
 	case *syntax.Subshell:
-		r2 := r.sub()
+		r2 := r.Subshell()
 		r2.stmts(ctx, x.Stmts)
 		r.exit = r2.exit
 		r.setErr(r2.err)
@@ -898,7 +926,7 @@ func (r *Runner) cmd(ctx context.Context, cm syntax.Command) {
 			}
 		case syntax.Pipe, syntax.PipeAll:
 			pr, pw := io.Pipe()
-			r2 := r.sub()
+			r2 := r.Subshell()
 			r2.stdout = pw
 			if x.Op == syntax.PipeAll {
 				r2.stderr = pw


Test patch:
diff --git a/interp/interp_test.go b/interp/interp_test.go
index e3cc45f2..5a96a341 100644
--- a/interp/interp_test.go
+++ b/interp/interp_test.go
@@ -668,6 +668,22 @@ var runTests = []runTest{
 		"(fn() { :; }) & pwd >/dev/null",
 		"",
 	},
+	{
+		"x[0]=x; (echo ${x[0]}; x[0]=y; echo ${x[0]}); echo ${x[0]}",
+		"x\ny\nx\n",
+	},
+	{
+		`x["x"]=x; (x["x"]=y); echo ${x[0]}`,
+		"x\n",
+	},
+	{
+		"shopt -s expand_aliases; alias f='echo x'\nf\n(f\nalias f='echo y'\nf\n)\nf\n",
+		"x\nx\ny\nx\n",
+	},
+	{
+		"set -- a; echo $1; (echo $1; set -- b; echo $1); echo $1",
+		"a\na\nb\na\n",
+	},
 
 	// cd/pwd
 	{"[[ fo~ == 'fo~' ]]", ""},