`use` creates a namespace even when no module was loaded

[nichtsundniemand]

Hey, so I was playing around with writing some scripts and stumbled across something weird:

When I try to use a module which does not exist - be it from a relative file or otherwise - the corresponding namespace will still be created.

Here are some examples:

~> use thismoduledoesnotexist
Exception: no such module: thismoduledoesnotexist
[tty 82]:1:1: use thismoduledoesnotexist
~> put $thismoduledoesnotexist:
▶ <ns 0xc000492810>

Or when trying to load a file (this happens quite often because of auto-completion not omitting the .elv-extension):

~> mkdir nil
~> use ./nil/nil.elv
Exception: no such module: ./nil/nil.elv
[tty 3]:1:1: use ./nil/nil.elv
~> put $'nil.elv:'
▶ <ns 0xc0002e2630>

As you can see the second example is even worse than my usual problem which is mistakenly having the extension for a module-file in the use-path. In the above example I tried to load an obviously inexistent file and the namespace was still created.

I haven't yet looked into elvish's source code to find the spot where this goes wrong. But I would be interested in fixing this bug myself.

EDIT: Ah - I forgot to do my due dilligence:

~> put $version
▶ 0.19.2

[nichtsundniemand]

Okay, so I think I'm starting to figure out what's going on. The problem appears to be that the global namespace is updated even before the statement is fully evaluated.

I've written some comments in the code while debugging. I hope this diff-output shows clearly what I believe to be the root of this issue:

diff --git a/pkg/eval/compiler.go b/pkg/eval/compiler.go
index 245efff4..c56b2ff9 100644
--- a/pkg/eval/compiler.go
+++ b/pkg/eval/compiler.go
@@ -66,14 +66,18 @@ func (op nsOp) prepare(fm *Frame) (*Ns, func() Exception) {
                copy(newLocal.slots, fm.local.slots)
                for i := len(fm.local.infos); i < n; i++ {
                        // TODO: Take readOnly into account too
+                       // Here we actually instantiate the broken namespace!
                        newLocal.slots[i] = MakeVarFromName(newLocal.infos[i].name)
                }
+               // Here we assign the new Namespace to the frame-locals
                fm.local = newLocal
        } else {
                // If no new variable has been created, there might still be some
                // existing variables deleted.
                fm.local = &Ns{fm.local.slots, op.template.infos}
        }
+       // Here we return the namespace which is then made the new global namespace
+       // in `(*Evaler).Eval()`
        return fm.local, func() Exception { return op.inner.exec(fm) }
 }

diff --git a/pkg/eval/eval.go b/pkg/eval/eval.go
index 6de58d72..56667e70 100644
--- a/pkg/eval/eval.go
+++ b/pkg/eval/eval.go
@@ -334,6 +334,7 @@ func (ev *Evaler) Eval(src parse.Source, cfg EvalCfg) error {
                ev.mu.Unlock()
        }

+       // This returns an `nsOp` struct with a template containing the unwanted namespace's name
        op, _, err := compile(b.static(), cfg.Global.static(), nil, tree, errFile)
        if err != nil {
                if defaultGlobal {
@@ -345,6 +346,7 @@ func (ev *Evaler) Eval(src parse.Source, cfg EvalCfg) error {
        fm, cleanup := ev.prepareFrame(src, cfg)
        defer cleanup()

+       // This returns a namespace containing the new - unwanted - namespace as a member (in `slots` and `infos`)
        newLocal, exec := op.prepare(fm)
        if defaultGlobal {
                ev.global = newLocal

So at some point after hitting the <Return>-key the shell enters the (*Evaler).Eval()-method. This method is responsible for parsing, compiling, "preparing" and executing the expression given on the command-line (or some other source, I guess).

The compile()-call returns an nsOp which in it's template-member already contains the name of the new namespace-variable in it's infos-slice. The corresponding element in the slots-slice has the value nil at this point.

Next op.prepare() is called which - as far as I understand - prepares the namespaces for the upcoming execution of the Operation?

Since the template of this nsOp contains the name for a new namespace it is instantiated with MakeVarFromName(). This new namespace containing both the name and pointer to the unwanted namespace is then returned from nsOp.prepare() and later assigned as the evaluator's new global namespace.

I don't know why it is necessary to do things this way around - it seems weird to create the names in the namespace before actually evaluating the statement.

And if for some reason it is necessary to do so (maybe so these names can actually be used later?) I suppose it would be sensible to have some sort of "roll-back" mechanism in case the evaluation fails after all.

[nichtsundniemand]

This also got me thinking if there were other cases this might occur. I've come up with one but I don't think it is that bad:

~> var iamexceptional = (fail foo)
Exception: foo
[tty 15]:1:23: var iamexceptional = (fail foo)
~> put $iamexceptional
▶ $nil

I'm really not sure if this should be allowed or not. On the one hand it kinda makes sense that the variable has a value of $nil OTOH maybe it shouldn't exist at all? I don't know...

Is the returned value of fail foo actually $nil? Does that statement even have a return value?

[krader1961]

Consider a related example:

elvish> put $v
Compilation error: variable $v not found
  [tty 2]:1:5: put $v
elvish> var v = (nop)
Exception: arity mismatch: assignment right-hand-side must be 1 value, but is 0 values
[tty 3]:1:1: var v = (nop)
elvish> put $v
▶ $nil

Don't confuse the declaration (var v) with the assignment op. In you example fail foo does not output any value. It raises an exception which can be caught:

elvish> var iamexceptional = ?(fail foo)
elvish> put $iamexceptional
▶ [&reason=[&content=foo &type=fail]]

[nichtsundniemand]

Thanks for the example :)

And yes - you are right - there is a distinction between declaration and assignment. EDIT: Also these are different code-paths, I believe... The name iamexceptional is maybe not introduced into the namespace the same way thismoduledoesnotexist is? SECONDEDIT: Well actually it is. Both are added to the template during compilation and instantiated during preparation.

Do you feel the same way about the use foo example?

[krader1961]

Put the following in a file named ~/.config/elvish/lib/mod.elv:

echo begin
var exception-occurred = $false
try {
    use nosuchmodule
} catch {
    echo use exception
    set exception-occurred = $true
}
use math
echo done

Then, at the interactive REPL prompt:

elvish> use mod
begin
use exception
done
elvish> pprint $mod:
<ns 0xc000599320>
elvish> keys $mod:
▶ exception-occurred
▶ math:
elvish> put $mod:exception-occurred
▶ $true

Notice that the $mod: namespace does not contain a nosuchmodule: key. The fact the interactive REPL namespace does when using a non-existent module is a quirk of the REPL. Whether that should be considered a bug is debatable. Unless changing the behavior is trivial I don't think it's worth it.

[nichtsundniemand]

Hm that is interesting that the two modes (REPL and use) behave differently. There is this check in Eval() for whether or not to modify the global namespace:

        newLocal, exec := op.prepare(fm)
        if defaultGlobal {
                ev.global = newLocal

I haven't spent a lot of time with the code but I assume that is what's different between evaluating a module at load-time and evaluating REPL commands?

This other example without exception handling also (as expected) leaves the identifier in the global namespace but does actually not result in a namespace containing any names - not even math::

echo begin
use math
use nosuchmodule
echo done

~> use ./anothermod
begin
Exception: no such module: nosuchmodule
Traceback:
  /home/wirklichniemand/anothermod.elv:4:1:
    use nosuchmodule
  [tty 19]:1:1:
    use ./anothermod
~> keys $anothermod:
~> 

I don't really care that this namespace which failed to be properly created doesn't contain any names at all since in my mind it's invalid as it resulted from a failed attempt to load a module.

But still - that there is different behaviour in this regard between REPL and non-REPL modes of evaluation is at least interesting to me...

In general I find it a bit weird that the compilation-step modifies the namespace at all. When thinking about the examples we have discussed so far (except maybe your last one) I would feel that the modification of the namespace - so creating a variable or adding a module - should happen during execution of the command and not before.

But when I think about compiling a module it makes more sense. The result of module-compilation is a new namespace so it makes sense to modify it during that stage (because again, in other words, compiling a module means compiling a namespace).

Now to the question of whether or not this is a bug I'd say: To me it was definitely unexpected behaviour.

Of course having some useless names in the REPL's namespaces isn't really a big deal since they can also just be del'ed if they annoy me. I don't really see this causing any problems either since another - maybe successful - use-call would shadow the old name.

Where I first noticed it and where I also feel like it is a bit problematic is during auto-complete where I wouldn't want to have a lot of actually unusable names being suggested.

My ideal solution would be that the name is not created afterall. I would be fine with it being created but initialized to $nil for example - more clearly communicating that there is something wrong.

Just trying out a var ns: = $nil gives me an Exception: wrong type: need ns, got nil though, so I guess that is not an acceptable solution.

I've tried to come up with a quick fix for my problem:

diff --git a/pkg/eval/eval.go b/pkg/eval/eval.go
index 6de58d72..9481e31d 100644
--- a/pkg/eval/eval.go
+++ b/pkg/eval/eval.go
@@ -347,8 +347,19 @@ func (ev *Evaler) Eval(src parse.Source, cfg EvalCfg) error {

        newLocal, exec := op.prepare(fm)
        if defaultGlobal {
-               ev.global = newLocal
+               // The Evaler-lock needs to be unlocked before the Op can be executed
                ev.mu.Unlock()
+
+               err := exec()
+               if err == nil {
+                       // Now if the Op was executed successfully we can modify the Evaler's
+                       // global namespace
+                       ev.mu.Lock()
+                       ev.global = newLocal
+                       ev.mu.Unlock()
+               }
+
+               return err
        }

        return exec()

But introducing this change makes some of the unit-tests fail and also changes the behaviour of var a = (nop) to now also not create the name a in the global namespace.

// And also: I hope I am not coming off as nitpicky or anything. I really like this project and would love for elvish to become my default-shell - or at least that the idea of a more modern shell gains more traction. // I am just having fun digging through the code and trying to understand why elvish behaves the way it does.

[nichtsundniemand]

The problem in #1692 might be related? If the use-statement creates the name edit: in the global namespace it would problably shadow the builtin edit:-NS?