Add back support for circular module dependency.

This fixes #1226.

pkg/eval/benchmarks_test.go

@@ -51,7 +51,8 @@ func benchmarkEval(b *testing.B, code string) {
 
 	for i := 0; i < b.N; i++ {
 		fm, cleanup := ev.prepareFrame(src, EvalCfg{Global: ev.Global()})
-		op.exec(fm)
+		_, exec := op.prepare(fm)
+		_ = exec()
 		cleanup()
 	}
 }

pkg/eval/builtin_special.go

@@ -381,11 +381,14 @@ func useFromFile(fm *Frame, spec, path string, r diag.Ranger) (*Ns, error) {
 
 // TODO: Make access to fm.Evaler.modules concurrency-safe.
 func evalModule(fm *Frame, key string, src parse.Source, r diag.Ranger) (*Ns, error) {
-	// Make an empty namespace before executing. This prevent circular use'es
+	ns, exec, err := fm.PrepareEval(src, r, new(Ns))
+	if err != nil {
+		return nil, err
+	}
+	// Installs the namespace before executing. This prevent circular use'es
 	// from resulting in an infinite recursion.
-	fm.Evaler.modules[key] = new(Ns)
-	ns, err := fm.Eval(src, r, new(Ns))
 	fm.Evaler.modules[key] = ns
+	err = exec()
 	if err != nil {
 		// Unload the namespace.
 		delete(fm.Evaler.modules, key)

pkg/eval/builtin_special_test.go

@@ -239,6 +239,25 @@ func TestUse_SetsVariableCorrectlyIfModuleCallsAddGlobal(t *testing.T) {
 	}
 }
 
+func TestUse_SupportsCircularDependency(t *testing.T) {
+	libdir, cleanup := InTestDir()
+	defer cleanup()
+
+	ApplyDir(Dir{
+		"a.elv": "var pre = apre; use b; put $b:pre $b:post; var post = apost",
+		"b.elv": "var pre = bpre; use a; put $a:pre $a:post; var post = bpost",
+	})
+
+	TestWithSetup(t, func(ev *Evaler) { ev.SetLibDir(libdir) },
+		That(`use a`).Puts(
+			// When b.elv is imported from a.elv, $a:pre is set but $a:post is
+			// not
+			"apre", nil,
+			// After a.elv imports b.elv, both $b:pre and $b:post are set
+			"bpre", "bpost"),
+	)
+}
+
 func TestUse(t *testing.T) {
 	libdir, cleanup := InTestDir()
 	defer cleanup()

pkg/eval/compiler.go

@@ -65,14 +65,9 @@ type nsOp struct {
 	template *staticNs
 }
 
-// Prepares a new local namespace before executing the inner effectOp. Replaces
-// fm.local.
-func (op nsOp) exec(fm *Frame) Exception {
-	op.prepareNs(fm)
-	return op.inner.exec(fm)
-}
-
-func (op nsOp) prepareNs(fm *Frame) {
+// Prepares the local namespace, and returns the namespace and a function for
+// executing the inner effectOp. Mutates fm.local.
+func (op nsOp) prepare(fm *Frame) (*Ns, func() Exception) {
 	if len(op.template.names) > len(fm.local.names) {
 		n := len(op.template.names)
 		newLocal := &Ns{make([]vars.Var, n), op.template.names, op.template.deleted}
@@ -86,7 +81,7 @@ func (op nsOp) prepareNs(fm *Frame) {
 		// variables deleted.
 		fm.local = &Ns{fm.local.slots, fm.local.names, op.template.deleted}
 	}
-
+	return fm.local, func() Exception { return op.inner.exec(fm) }
 }
 
 const compilationErrorType = "compilation error"

pkg/eval/eval.go

@@ -451,14 +451,13 @@ func (ev *Evaler) Eval(src parse.Source, cfg EvalCfg) error {
 	fm, cleanup := ev.prepareFrame(src, cfg)
 	defer cleanup()
 
-	op.prepareNs(fm)
+	newLocal, exec := op.prepare(fm)
 	if defaultGlobal {
-		ev.global = fm.local
+		ev.global = newLocal
 		ev.mu.Unlock()
 	}
 
-	exc := op.exec(fm)
-	return exc
+	return exec()
 }
 
 // CallCfg keeps configuration for the (*Evaler).Call method.

pkg/eval/frame.go

@@ -31,15 +31,19 @@ type Frame struct {
 	background bool
 }
 
-// Eval evaluates a piece of code in a copy. If r is not nil, it is added to the
-// traceback of the evaluation context. If ns is not nil, it is used in place
-// of the current local namespace as the namespace to evaluate the code in. It
-// returns the altered local namespace and any parse error, compilation error or
-// exception.
-func (fm *Frame) Eval(src parse.Source, r diag.Ranger, ns *Ns) (*Ns, error) {
+// PrepareEval prepares a piece of code for evaluation in a copy of the current
+// Frame. If r is not nil, it is added to the traceback of the evaluation
+// context. If ns is not nil, it is used in place of the current local namespace
+// as the namespace to evaluate the code in.
+//
+// If there is any parse error or compilation error, it returns a nil *Ns, nil
+// function and the error. If there is no parse error or compilation error, it
+// returns the altered local namespace, function that can be called to actuate
+// the evaluation, and a nil error.
+func (fm *Frame) PrepareEval(src parse.Source, r diag.Ranger, ns *Ns) (*Ns, func() Exception, error) {
 	tree, err := parse.ParseWithDeprecation(src, fm.ErrorFile())
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	local := fm.local
 	if ns != nil {
@@ -52,11 +56,23 @@ func (fm *Frame) Eval(src parse.Source, r diag.Ranger, ns *Ns) (*Ns, error) {
 	newFm := &Frame{
 		fm.Evaler, src, local, new(Ns), fm.intCh, fm.ports, traceback, fm.background}
 	op, err := compile(newFm.Evaler.Builtin().static(), local.static(), tree, fm.ErrorFile())
+	if err != nil {
+		return nil, nil, err
+	}
+	newLocal, exec := op.prepare(newFm)
+	return newLocal, exec, nil
+}
+
+// Eval evaluates a piece of code in a copy of the current Frame. It returns the
+// altered local namespace, and any parse error, compilation error or exception.
+//
+// See PrepareEval for a description of the arguments.
+func (fm *Frame) Eval(src parse.Source, r diag.Ranger, ns *Ns) (*Ns, error) {
+	newLocal, exec, err := fm.PrepareEval(src, r, ns)
 	if err != nil {
 		return nil, err
 	}
-	exc := op.exec(newFm)
-	return newFm.local, exc
+	return newLocal, exec()
 }
 
 // Close releases resources allocated for this frame. It always returns a nil

website/ref/language.md

@@ -2211,29 +2211,51 @@ In general, a module defined in namespace will be the same as the file name
 
 ### Circular dependencies
 
-Circular dependencies are legal but restricted. If a module `a` imports module
-`b`, which then also imports module `a`, module `b` won't be able to observe any
-bindings in module `a` when it it evaluated. Instead, it will observe an empty
-namespace.
+Circular dependencies are allowed but has an important restriction. If a module
+`a` contains `use b` and module `b` contains `use a`, the top-level statements
+in module `b` will only be able to access variables that are defined before the
+`use b` in module `a`; other variables will be `$nil`.
 
 On the other hand, functions in module `b` will have access to bindings in
 module `a` after it is fully evaluated.
 
 Examples:
 
-```elvish
-# a.elv
-foo = lorem
-use b
-# b.elv
-use a
-fn f { put $a:foo }
-put $a:foo # results in an exception
-# in REPL
-use a; use b
-b:f # puts "lorem"
+```elvish-transcript
+~> cat a.elv
+var before = before
+use ./b
+var after = after
+~> cat b.elv
+use ./a
+put $a:before $a:after
+fn f { put $a:before $a:after }
+~> use ./a
+▶ before
+▶ $nil
+~> use ./b
+~> b:f
+▶ before
+▶ after
 ```
 
+Note that this behavior can be different depending on whether the REPL imports
+`a` or `b` first. In the previous example, if the REPL imports `b` first, it
+will have access to all the variables in `a`:
+
+```elvish-transcript
+~> use ./b
+▶ before
+▶ after
+```
+
+**Note**: Elvish caches imported modules. If you are trying this locally, run a
+fresh Elvish instance with `exec` first.
+
+When you do need to have circular dependencies, it is best to avoid using
+variables from the modules in top-level statements, and only use them in
+functions.
+
 ### Relative imports
 
 The module spec may being with `./` or `../`, which introduce **relative