Re: set-current-module broken in current Guile CVS version (anyone?)

[Dirk Herrmann]

Hello everybody!

First the good news: I have a patch for you.  You find it at the bottom of
this mail.  The bad news is that I will explain the idea behing the patch
in the mail, and I would like people to take a look and comment :-)


On 5 Feb 2001, Matthias Koeppe wrote:

> Thanks Dirk but this won't help.  I consider the module system to be
> completely broken.  Look at this example where I don't even back out
> to the repl and still everything is wrong:
> 
> guile> (begin (define-module (xyzzy))
>               (define xyz 3456)
>               (define-public (def) xyz))
> guile> (use-modules (xyzzy))
> guile> (def)

Did this example work in the 2000-12-07 CVS version?  I mean _exactly_ the
expression typed above, that is with the (begin...) around the define
expressions?

IMO, it should only have worked if typed without the surrounding begin.

> I don't really understand the module system but this behaviour is
> simply broken. For me this means that Guile has become non-interactive
> -- I can no longer change code reliably in run-time. I still have to
> run Guile in the 2000-12-07 CVS version, which works as one expects.

All these problems are just facettes of the underlying problem, namely
that there is a distinction between the top level environment (TLE) that
is used by the evaluator, and the (current-module).  Up to now, it seems,
the relationship between these two has not been cleanly defined.  The
problem with your example is the following:

* 'define' will add a definition to the evaluator's TLE. 
* 'define-public' adds a definition to the current-module. 
* 'define-module' changes current-module, but _not_ the evaluator's
  TLE.  This is only done indirectly through the repl, because the repl
  will use current-module for the _next_ evaluation.
* 'export' exports an identifier from the current-module.  If there is no
  such identifier defined in the current-module yet, a default one is
  created that binds the identifer to #f.

Let's take a look at how a session worked prior to 2000-12-15:

  guile> (define-module (xyzzy))
    ;; This changes the current-module and will thus be used by the repl
    ;; as the _next_ interaction environment.  Thus, the next expression
    ;; will be evaluated with a) current-module set to xyzzy and b) the
    ;; evaluator's TLE being xyzzy, too.
  guile> (define xyz 3456)
    ;; Since the evaluator's TLE is xyzzy, this binding will go into
    ;; module xyzzy.
  guile> (define-public (def) xyz)
    ;; defines def in the current-module, which is also xyzzy

Thus, everything seemed to work!

Now, compare this to the way the session with the 'begin' wrapper works.

  guile> (begin 
    ;; The whole evaluation is started with guile-user being the
    ;; evaluator's TLE, as well as the current-module.
                (define-module (xyzzy))
    ;; This changes the current-module to xyzzy, but the evaluator's TLE
    ;; is still guile-user.
                (define xyz 3456)
    ;; (!) This goes into guile-user, because the implementation of define
    ;; puts it into the evaluator's TLE
                (define-public (def) xyz))
    ;; (!) This goes into xyzzy, because define-public puts it into the
    ;; current-module.

Thus, it did not work, neither before, nor after my patch!

The right fix is, to make define and define-public to behave alike, namely
to place their definitions in the current-module.  This is what my patch
below does.

> I also don't really understand why the 2000-12-15 change (which I
> believe caused the problems) fixed the bug I reported regarding
> `export' via `eval', like this:

Before my patch, eval did _only_ set the environment's TLE to the given
environment.  After my patch, eval will guarantee that at the beginning of
the evaluation both the environment's TLE _and_ the current-module are set
to the environment given to eval.

However, within the repl, this did not cause any problems:  The repl
evaluates code in (interaction-environment), which is currently an alias
for current-module.  Thus, for expressions evaluated by the repl the
environment's TLE and the current-module were equal anyway.  The problems
did only occur for explicit calls to eval, where the environment argument
was different from the current-module.  Thus, all calls to eval with
(current-module) or with (interaction-environment) would also have worked.

> guile> (define-module (murks))
> #<directory (murks) 40222d38>
> guile> (define-module (guile-user))
> #<directory (guile-user) 401f6db0>
> guile> (eval-in-module '(define x 1) (resolve-module '(murks)))
    ;; Since define used the environments TLE, this definition
    ;; went into (resolve-module '(murks)).
> guile> x
> ERROR: Unbound variable: x        ; right
> guile> (eval-in-module '(export x) (resolve-module '(murks)))
    ;; export, however, uses current-module to export the identifier.
    ;; The current-module, however, is guile-user, since eval-in-module
    ;; calls eval, and eval did formerly not change current-module.
    ;; But, current-module (= guile-user) does not hold a binding
    ;; for x yet.  Therefore a dummy binding #f is created for x.
> guile> x
> #f                                ; in Guile 1.4 (wrong)
> ERROR: Unbound variable           ; in CVS Guile (right)
> guile> (use-modules (murks))
    ;; This adds module (murks) to the import list.  However, since there
    ;; already is a binding for x in guile-user, this has priority.
> guile> x
> #f                                ; in Guile 1.4 (wrong)
> 1                                 ; in CVS Guile (right)

> In any case, the change broke more than it fixed.  

The change itself is correct (*), but it brought up a discrepancy between
the behaviour of 'define' and 'define-public'.  (*): We are still
discussing whether the patch is correct in its entirety.  But, at least
the part that guarantees that current-module and the evaluator's TLE are
equal at the _beginning_ of the evaluation is correct.

Best regards,
Dirk Herrmann



Index: eval.c
===================================================================
RCS file: /cvs/guile/guile-core/libguile/eval.c,v
retrieving revision 1.193
diff -u -r1.193 eval.c
--- eval.c      2001/02/02 04:56:24     1.193
+++ eval.c      2001/02/05 16:42:21
@@ -836,6 +836,7 @@
 SCM 
 scm_m_define (SCM x, SCM env)
 {
+  SCM target_module;
   SCM proc, arg1 = x;
   x = SCM_CDR (x);
   SCM_ASSYNT (scm_ilength (x) >= 2, arg1, scm_s_expression, s_define);
@@ -869,7 +870,10 @@
            }
        }
 #endif
-      arg1 = scm_sym2vcell (proc, scm_env_top_level (env), SCM_BOOL_T);
+      target_module = scm_module_system_booted_p
+       ? SCM_MODULE_EVAL_CLOSURE (scm_selected_module ())
+       : SCM_BOOL_F;
+      arg1 = scm_sym2vcell (proc, target_module, SCM_BOOL_T);
       SCM_SETCDR (arg1, x);
 #ifdef SICP
       return scm_cons2 (scm_sym_quote, SCM_CAR (arg1), SCM_EOL);