[PATCH] srfi-64: fix unused variable warnings

[PATCH] srfi-64: fix unused variable warnings

[Aleix Conchillo Flaqué]

* module/srfi/srfi-64/testing.scm: remove unused name variable and use
let instead of let*.

Signed-off-by: Aleix Conchillo Flaqué <aconchillo@gmail.com>
---
 module/srfi/srfi-64/testing.scm | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/module/srfi/srfi-64/testing.scm b/module/srfi/srfi-64/testing.scm
index 37792cd0f..d97797786 100644
--- a/module/srfi/srfi-64/testing.scm
+++ b/module/srfi/srfi-64/testing.scm
@@ -707,26 +707,24 @@
       (syntax-case (list x (list (syntax quote) (%test-source-line2 x))) ()
 	(((mac tname expr) line)
 	 (syntax
-	  (let* ((r (test-runner-get))
-		 (name tname))
+	  (let ((r (test-runner-get)))
 	    (test-result-alist! r (cons (cons 'test-name tname) line))
 	    (%test-comp1body r expr))))
 	(((mac expr) line)
 	 (syntax
-	  (let* ((r (test-runner-get)))
+	  (let ((r (test-runner-get)))
 	    (test-result-alist! r line)
 	    (%test-comp1body r expr)))))))
   (define (%test-comp2 comp x)
     (syntax-case (list x (list (syntax quote) (%test-source-line2 x)) comp) ()
       (((mac tname expected expr) line comp)
        (syntax
-	(let* ((r (test-runner-get))
-	       (name tname))
+	(let ((r (test-runner-get)))
 	  (test-result-alist! r (cons (cons 'test-name tname) line))
 	  (%test-comp2body r comp expected expr))))
       (((mac expected expr) line comp)
        (syntax
-	(let* ((r (test-runner-get)))
+	(let ((r (test-runner-get)))
 	  (test-result-alist! r line)
 	  (%test-comp2body r comp expected expr))))))
   (define-syntax test-eqv
@@ -740,13 +738,12 @@
       (syntax-case (list x (list (syntax quote) (%test-source-line2 x))) ()
       (((mac tname expected expr error) line)
        (syntax
-	(let* ((r (test-runner-get))
-	       (name tname))
+	(let ((r (test-runner-get)))
 	  (test-result-alist! r (cons (cons 'test-name tname) line))
 	  (%test-comp2body r (%test-approximate= error) expected expr))))
       (((mac expected expr error) line)
        (syntax
-	(let* ((r (test-runner-get)))
+	(let ((r (test-runner-get)))
 	  (test-result-alist! r line)
 	  (%test-comp2body r (%test-approximate= error) expected expr))))))))
  (else
@@ -759,23 +756,21 @@
   (define-syntax test-assert
     (syntax-rules ()
       ((test-assert tname test-expression)
-       (let* ((r (test-runner-get))
-	      (name tname))
+       (let ((r (test-runner-get)))
 	 (test-result-alist! r '((test-name . tname)))
 	 (%test-comp1body r test-expression)))
       ((test-assert test-expression)
-       (let* ((r (test-runner-get)))
+       (let ((r (test-runner-get)))
 	 (test-result-alist! r '())
 	 (%test-comp1body r test-expression)))))
   (define-syntax %test-comp2
     (syntax-rules ()
       ((%test-comp2 comp tname expected expr)
-       (let* ((r (test-runner-get))
-	      (name tname))
+       (let ((r (test-runner-get)))
 	 (test-result-alist! r (list (cons 'test-name tname)))
 	 (%test-comp2body r comp expected expr)))
       ((%test-comp2 comp expected expr)
-       (let* ((r (test-runner-get)))
+       (let ((r (test-runner-get)))
 	 (test-result-alist! r '())
 	 (%test-comp2body r comp expected expr)))))
   (define-syntax test-equal
@@ -895,18 +890,17 @@
       (syntax-case (list x (list (syntax quote) (%test-source-line2 x))) ()
 	(((mac tname etype expr) line)
 	 (syntax
-	  (let* ((r (test-runner-get))
-		 (name tname))
+	  (let ((r (test-runner-get)))
 	    (test-result-alist! r (cons (cons 'test-name tname) line))
 	    (%test-error r etype expr))))
 	(((mac etype expr) line)
 	 (syntax
-	  (let* ((r (test-runner-get)))
+	  (let ((r (test-runner-get)))
 	    (test-result-alist! r line)
 	    (%test-error r etype expr))))
 	(((mac expr) line)
 	 (syntax
-	  (let* ((r (test-runner-get)))
+	  (let ((r (test-runner-get)))
 	    (test-result-alist! r line)
 	    (%test-error r #t expr))))))))
  (else
-- 
2.31.1

Re: [PATCH] srfi-64: fix unused variable warnings

[Maxime Devos]

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

On Wed, 2021-03-31 at 23:11 -0700, Aleix Conchillo Flaqué wrote:
> * module/srfi/srfi-64/testing.scm: remove unused name variable and use
> let instead of let*.
> 

I don't think this is the correct approach with respect to side effects.
For example, in:

>  (define (%test-comp2 comp x)
>      (syntax-case (list x (list (syntax quote) (%test-source-line2 x)) comp) ()
>        (((mac tname expected expr) line comp)
>         (syntax
> -	(let* ((r (test-runner-get))
> -	       (name tname))
> +	(let ((r (test-runner-get)))
>  	  (test-result-alist! r (cons (cons 'test-name tname) line))
>  	  (%test-comp2body r comp expected expr))))

I would keep the let* (but reverse the binding order), but change 'tname'
with 'name' in the call to 'test-result-alist!', such that 'test-X' macros
behave somewhat more like procedure calls (except for installing exeption
handlers and having access to the s-expression of the code that will be run,
of course).  It's largely a matter of taste, though.

In any case, it is good that 'tname' is now evaluated only once, as per
SRFI-64 (notice ***It is evaluated only once.*** (markup mine)):

 (test-assert [test-name] expression)

 This evaluates the expression. The test passes if the result is true;
 if the result is false, a test failure is reported. The test also fails
 if an exception is raised, assuming the implementation has a way to catch
 exceptions. How the failure is reported depends on the test runner environment.
 The test-name is a string that names the test case. (Though the test-name is
 a string literal in the examples, it is an expression. ***It is evaluated only once.***)
 It is used when reporting errors, and also when skipping tests, as described below.
 It is an error to invoke test-assert if there is no current test runner.

(My suggestion would be to also evaluate 'test-name' at least once, even if there
is no test runner, which seems a bit stricter than SRFI-64 demands, but seems like
a nice property to have and easy to achieve.)

As this patch does not ‘merely’ fix a warnings, but fixes a bug, could you change
the patch message accordingly?  Something like

  srfi-64: fix double evaluation of test-name.

perhaps?

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: [PATCH] srfi-64: fix unused variable warnings

[Aleix Conchillo Flaqué]

Hi Maxime,

Thank you for your comments!

On Thu, Apr 1, 2021 at 4:37 AM Maxime Devos <maximedevos@telenet.be> wrote:

> For example, in:
>
> >  (define (%test-comp2 comp x)
> >      (syntax-case (list x (list (syntax quote) (%test-source-line2 x)) comp) ()
> >        (((mac tname expected expr) line comp)
> >         (syntax
> > -     (let* ((r (test-runner-get))
> > -            (name tname))
> > +     (let ((r (test-runner-get)))
> >         (test-result-alist! r (cons (cons 'test-name tname) line))
> >         (%test-comp2body r comp expected expr))))
>
> I would keep the let* (but reverse the binding order), but change 'tname'
> with 'name' in the call to 'test-result-alist!', such that 'test-X' macros
> behave somewhat more like procedure calls (except for installing exeption
> handlers and having access to the s-expression of the code that will be run,
> of course).  It's largely a matter of taste, though.
>

I've done this change. One thing I don't understand is the "reverse
the binding order", I've done it as suggested but is this change the
one you refer to as "matter of taste"?

> In any case, it is good that 'tname' is now evaluated only once, as per
> SRFI-64 (notice ***It is evaluated only once.*** (markup mine)):
>
>  (test-assert [test-name] expression)
>
>  This evaluates the expression. The test passes if the result is true;
>  if the result is false, a test failure is reported. The test also fails
>  if an exception is raised, assuming the implementation has a way to catch
>  exceptions. How the failure is reported depends on the test runner environment.
>  The test-name is a string that names the test case. (Though the test-name is
>  a string literal in the examples, it is an expression. ***It is evaluated only once.***)
>  It is used when reporting errors, and also when skipping tests, as described below.
>  It is an error to invoke test-assert if there is no current test runner.
>
> (My suggestion would be to also evaluate 'test-name' at least once, even if there
> is no test runner, which seems a bit stricter than SRFI-64 demands, but seems like
> a nice property to have and easy to achieve.)
>

Yes, this makes sense. Thanks again for pointing that out.

> As this patch does not ‘merely’ fix a warnings, but fixes a bug, could you change
> the patch message accordingly?  Something like
>
>   srfi-64: fix double evaluation of test-name.
>
> perhaps?
>

Sounds good to me.

Best,

Aleix

Re: [PATCH] srfi-64: fix unused variable warnings

[Maxime Devos]

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

On Thu, 2021-04-01 at 23:12 -0700, Aleix Conchillo Flaqué wrote:
> Hi Maxime,
> 
> Thank you for your comments!
> 
> On Thu, Apr 1, 2021 at 4:37 AM Maxime Devos <maximedevos@telenet.be> wrote:
> 
> > For example, in:
> > 
> > >  (define (%test-comp2 comp x)
> > >      (syntax-case (list x (list (syntax quote) (%test-source-line2 x)) comp) ()
> > >        (((mac tname expected expr) line comp)
> > >         (syntax
> > > -     (let* ((r (test-runner-get))
> > > -            (name tname))
> > > +     (let ((r (test-runner-get)))
> > >         (test-result-alist! r (cons (cons 'test-name tname) line))
> > >         (%test-comp2body r comp expected expr))))
> > 
> > I would keep the let* (but reverse the binding order), but change 'tname'
> > with 'name' in the call to 'test-result-alist!', such that 'test-X' macros
> > behave somewhat more like procedure calls (except for installing exeption
> > handlers and having access to the s-expression of the code that will be run,
> > of course).  It's largely a matter of taste, though.
> > 
> 
> I've done this change. One thing I don't understand is the "reverse
> the binding order", I've done it as suggested but is this change the
> one you refer to as "matter of taste"?

Yes, that's the change I was referring to.  As to why: a procedural
equivalent of 'test-assert would look more or less like

;; (possibly more arguments are required)
(define* (test-assert* name thunk expression)
  ;; THUNK: when called, return something that will be
  ;;   used as true/false.
  ;; EXPRESSION: S-expression representing the body
  ;;  of THUNK
  (let ((r (test-runner-get)))
    ;; evaluate (thunk) here within some exception
    ;; handlers and use r
    ...))

(Similar equivalents to test-equal, test-eq ... can be written
as well.)

Suppose  '(test-assert* NAME (lambda () EXP) 'EXP) is evaluated.

Then first NAME is evaluated, which can have side-effects.  The
lambda expression and (quote EXP) are evaluated as well, but no
side-effects are possible here (aside for allocating some memory
for the thunk, which can lead to a 'out-of-memory exception, but
that's usually simply ignored).

Only after the arguments are evaluated will '(test-runner-get)
be evaluated.

However, for the original test-assert macro, the evaluation
order is different.  From a REPL:

> (use-modules (srfi srfi-26))
> ,expand (test-assert NAME EXP)
;; output manually cleaned up
$6 = (let* ((r (test-runner-get))
            (name NAME))
       more code ....)

It should be clear that here 'NAME is evaluated *after*
'(test-runner-get) is evaluated, unlike for the 'test-assert*
procedure.

That said, SRFI-64 does not require NAME to be evaluated even
if trying to get the test runner fails for some reason, I
don't think anyone ever changes the test runner from within
a ‘call’ (not really a call as test-assert is a macro) to
test-assert, and in practice NAME is a constant, so in practice
it doesn't really matter in what order things are evaluated.

Also see next comment:

> > In any case, it is good that 'tname' is now evaluated only once, as per
> > SRFI-64 (notice ***It is evaluated only once.*** (markup mine)):
> > 
> > [...]
> Yes, this makes sense. Thanks again for pointing that out.

This is done correctly in the new patch (and the old patch IIRC).
Also, by reversing the binding order from

-       (let* ((r (test-runner-get))
-              (name tname))

to

+       (let* ((name tname)
+               (r (test-runner-get)))

the expression tname is also evaluated *at least* once,
thus TNAME is evaluated *exactly* once, which seems like
a nice property to have, though this is a bit stricter
than SRFI-64 demands IIUC.

Also, the formatting seems to have gone wrong.
Shouldn't this be

+       (let* ((name tname)
+              (r (test-runner-get)))

?  If in Emacs, I recommend scheme-mode, in which case pressing tab on the
second line would produce the desired formatting.  Alternatively, select
a region of text and presss tab.

> > As this patch does not ‘merely’ fix a warnings, but fixes a bug, could you change
> > the patch message accordingly?  Something like
> > 
> >   srfi-64: fix double evaluation of test-name.
> > 
> > perhaps?
> > 

The revised commit message looks good to me.

Greetings,
Maxime.
p.s: I'm not a guile maintainer so you will have to wait on
someone else to actually merge this.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]