Inconsistent 'return value' for definitions

Inconsistent 'return value' for definitions

[Mark H Weaver]

Hello all,

I think the following behavior is inconsistent and undesirable:

  scheme@(guile-user)> (list (compile '(define foo 3)))
  $1 = (#<unspecified>)
  scheme@(guile-user)> (list (primitive-eval '(define foo 3)))
  $2 = (#<variable 103b9c80 value: 3>)

It doesn't really make sense for 'define' to return a value, because it
cannot be used in expression context.  AFAICT, the only way to get this
'return value' is to call 'eval' or 'primitive-eval'.  This weirdness
has bitten Paul Smith in his efforts to port his code to Guile 2.

I think we should change 'define' to always "return" SCM_UNSPECIFIED, at
least in 2.2.  We might also consider nipping this in the bud and making
the same change in 2.0.4.

    Thanks,
      Mark

[PATCH] Fix primitive-eval to return #<unspecified> for definitions

[Mark H Weaver]

[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]

I wrote:

> Hello all,
>
> I think the following behavior is inconsistent and undesirable:
>
>   scheme@(guile-user)> (list (compile '(define foo 3)))
>   $1 = (#<unspecified>)
>   scheme@(guile-user)> (list (primitive-eval '(define foo 3)))
>   $2 = (#<variable 103b9c80 value: 3>)
>
> It doesn't really make sense for 'define' to return a value, because it
> cannot be used in expression context.  AFAICT, the only way to get this
> 'return value' is to call 'eval' or 'primitive-eval'.  This weirdness
> has bitten Paul Smith in his efforts to port his code to Guile 2.
>
> I think we should change 'define' to always "return" SCM_UNSPECIFIED, at
> least in 2.2.  We might also consider nipping this in the bud and making
> the same change in 2.0.4.

The more I think about it, the more convinced I am that this is simply a
bug in our Scheme evaluator.  Both our compiler and our bootstrap C
evaluator returns SCM_UNSPECIFIED for definitions.  Therefore, I would
like to nip this in the bud and push the following fix for 2.0.4.

What do you think?

    Mark



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] Fix primitive-eval to return #<unspecified> for definitions --]
[-- Type: text/x-patch, Size: 2073 bytes --]

From 3509a7f224bc6d7fa4e7eb629a416720eb7256fa Mon Sep 17 00:00:00 2001
From: Mark H Weaver <mhw@netris.org>
Date: Sun, 29 Jan 2012 17:43:13 -0500
Subject: [PATCH] Fix primitive-eval to return #<unspecified> for definitions

* module/ice-9/eval.scm (primitive-eval): Return #<unspecified> for
  definitions.  Previously the variable object was returned.

* test-suite/tests/eval.test (evaluator): Add test.

* NEWS: Add news entry.
---
 NEWS                       |    1 +
 module/ice-9/eval.scm      |    3 ++-
 test-suite/tests/eval.test |    4 ++++
 3 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/NEWS b/NEWS
index ce47686..02b824d 100644
--- a/NEWS
+++ b/NEWS
@@ -180,6 +180,7 @@ Search the manual for these identifiers and modules, for more.
 ** Fix erroneous check in `set-procedure-properties!'.
 ** Fix generalized-vector-{ref,set!} for slices.
 ** Fix error messages involving definition forms.
+** Fix primitive-eval to return #<unspecified> for definitions.
 ** HTTP: Extend handling of "Cache-Control" header.
 ** HTTP: Fix qstring writing of cache-extension values
 ** HTTP: Fix validators for various list-style headers.
diff --git a/module/ice-9/eval.scm b/module/ice-9/eval.scm
index c0fa64c..74b8532 100644
--- a/module/ice-9/eval.scm
+++ b/module/ice-9/eval.scm
@@ -428,7 +428,8 @@
          (let ((x (eval x env)))
            (if (and (procedure? x) (not (procedure-property x 'name)))
                (set-procedure-property! x 'name name))
-           (define! name x)))
+           (define! name x)
+           (if #f #f)))
       
         (('toplevel-set! (var-or-sym . x))
          (variable-set!
diff --git a/test-suite/tests/eval.test b/test-suite/tests/eval.test
index f532059..c3121c5 100644
--- a/test-suite/tests/eval.test
+++ b/test-suite/tests/eval.test
@@ -75,6 +75,10 @@
 
 (with-test-prefix "evaluator"
 
+  (pass-if "definitions return #<unspecified>"
+    (eq? (primitive-eval '(define test-var 'foo))
+         (if #f #f)))
+    
   (with-test-prefix "symbol lookup"
 
     (with-test-prefix "top level"
-- 
1.7.5.4

Re: [PATCH] Fix primitive-eval to return #<unspecified> for definitions

[Andy Wingo]

On Mon 30 Jan 2012 00:14, Mark H Weaver <mhw@netris.org> writes:

> What do you think?

Please push, thanks!

Andy
-- 
http://wingolog.org/