[PATCH] Don't mix definitions and expressions in SRFI-9

[PATCH] Don't mix definitions and expressions in SRFI-9

[Andreas Rottmann]

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


As always, see the patch header for details.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: srfi-9-fix.diff --]
[-- Type: text/x-diff, Size: 2552 bytes --]

From: Andreas Rottmann <a.rottmann@gmx.at>
Subject: Don't mix definitions and expressions in SRFI-9

The expansion of `define-inlinable' contained an expression, which made
SRFI-9's `define-record-type' fail in non-toplevel contexts ("definition
used in expression context").

* module/srfi/srfi-9.scm (define-inlinable): Get rid of apparently
  useless expression in the expansion, so the expansion yields only
  definitions.
* test-suite/tests/srfi-9.test (non-toplevel): New test verifying that
  `define-record-type' works in non-toplevel context as well.

---
 module/srfi/srfi-9.scm       |    3 +--
 test-suite/tests/srfi-9.test |   12 +++++++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/module/srfi/srfi-9.scm b/module/srfi/srfi-9.scm
index 80c3b60..7c2b073 100644
--- a/module/srfi/srfi-9.scm
+++ b/module/srfi/srfi-9.scm
@@ -1,6 +1,6 @@
 ;;; srfi-9.scm --- define-record-type
 
-;; 	Copyright (C) 2001, 2002, 2006, 2009, 2010 Free Software Foundation, Inc.
+;; 	Copyright (C) 2001, 2002, 2006, 2009, 2010, 2011 Free Software Foundation, Inc.
 ;;
 ;; This library is free software; you can redistribute it and/or
 ;; modify it under the terms of the GNU Lesser General Public
@@ -81,7 +81,6 @@
          #`(begin
              (define (proc-name formals ...)
                body ...)
-             proc-name ;; unused
              (define-syntax name
                (lambda (x)
                  (syntax-case x ()
diff --git a/test-suite/tests/srfi-9.test b/test-suite/tests/srfi-9.test
index cf933a8..f8006c4 100644
--- a/test-suite/tests/srfi-9.test
+++ b/test-suite/tests/srfi-9.test
@@ -1,7 +1,7 @@
 ;;;; srfi-9.test --- Test suite for Guile's SRFI-9 functions. -*- scheme -*-
 ;;;; Martin Grabmueller, 2001-05-10
 ;;;;
-;;;; Copyright (C) 2001, 2006, 2007, 2010 Free Software Foundation, Inc.
+;;;; Copyright (C) 2001, 2006, 2007, 2010, 2011 Free Software Foundation, Inc.
 ;;;; 
 ;;;; This library is free software; you can redistribute it and/or
 ;;;; modify it under the terms of the GNU Lesser General Public
@@ -93,3 +93,13 @@
   ;; prior to guile 1.6.9 and 1.8.1 this wan't enforced
   (pass-if-exception "set-y! on bar" exception:wrong-type-arg
      (set-y! b 99)))
+
+(with-test-prefix "non-toplevel"
+    
+  (define-record-type :frotz (make-frotz a b) frotz?
+    (a frotz-a) (b frotz-b set-frotz-b!))
+
+  (pass-if "construction"
+    (let ((frotz (make-frotz 1 2)))
+      (and (= (frotz-a frotz) 1)
+           (= (frotz-b frotz) 2)))))
-- 
tg: (d59dd06..) t/srfi-9-fix (depends on: stable-2.0)

[-- Attachment #3: Type: text/plain, Size: 63 bytes --]


Regards, Rotty
-- 
Andreas Rottmann -- <http://rotty.yi.org/>

Re: [PATCH] Don't mix definitions and expressions in SRFI-9

[Ludovic Courtès]

Hi Andreas,

Andreas Rottmann <a.rottmann@gmx.at> writes:

> The expansion of `define-inlinable' contained an expression, which made
> SRFI-9's `define-record-type' fail in non-toplevel contexts ("definition
> used in expression context").

SRFI-9 says “Record-type definitions may only occur at top-level”, and
I’m inclined to stick to it.  If we diverge, then people could write
code thinking it’s portable SRFI-9 code while it’s not.

How about adding a ‘let-record-type’ or similar in (srfi srfi-9 gnu)?

Thanks,
Ludo’.

Re: [PATCH] Don't mix definitions and expressions in SRFI-9

[Alex Shinn]

2011/3/7 Ludovic Courtès <ludo@gnu.org>:
> Hi Andreas,
>
> Andreas Rottmann <a.rottmann@gmx.at> writes:
>
>> The expansion of `define-inlinable' contained an expression, which made
>> SRFI-9's `define-record-type' fail in non-toplevel contexts ("definition
>> used in expression context").
>
> SRFI-9 says “Record-type definitions may only occur at top-level”, and
> I’m inclined to stick to it.

FWIW, R7RS is lifting this restriction.

-- 
Alex

Re: [PATCH] Don't mix definitions and expressions in SRFI-9

[Ludovic Courtès]

Hi Alex,

Alex Shinn <alexshinn@gmail.com> writes:

> 2011/3/7 Ludovic Courtès <ludo@gnu.org>:
>> Hi Andreas,
>>
>> Andreas Rottmann <a.rottmann@gmx.at> writes:
>>
>>> The expansion of `define-inlinable' contained an expression, which made
>>> SRFI-9's `define-record-type' fail in non-toplevel contexts ("definition
>>> used in expression context").
>>
>> SRFI-9 says “Record-type definitions may only occur at top-level”, and
>> I’m inclined to stick to it.
>
> FWIW, R7RS is lifting this restriction.

But then it’s not SRFI-9.  :-)

Thanks,
Ludo’.

Re: [PATCH] Don't mix definitions and expressions in SRFI-9

[Andreas Rottmann]

ludo@gnu.org (Ludovic Courtès) writes:

> Hi Andreas,
>
> Andreas Rottmann <a.rottmann@gmx.at> writes:
>
>> The expansion of `define-inlinable' contained an expression, which made
>> SRFI-9's `define-record-type' fail in non-toplevel contexts ("definition
>> used in expression context").
>
> SRFI-9 says “Record-type definitions may only occur at top-level”, and
> I’m inclined to stick to it.  If we diverge, then people could write
> code thinking it’s portable SRFI-9 code while it’s not.
>
I can certainly relate to that, and agree that it's a good principle,
however, see below.

> How about adding a ‘let-record-type’ or similar in (srfi srfi-9 gnu)?
>
The issue is not that I'm explictly writing code that uses
`define-record-type' in a non-toplevel context, but that I have a
testing framework (built upon Riastradh's trc-testing), which uses R6RS
`eval' to load testcases.  Since `eval' does not allow for the code to
be evaluated to be in a top-level context, I'm using this:

(eval `(let () ,@code-to-be-tested) the-environment-for-the-code)

Now `code-to-be-tested' can't contain any SRFI-9 record definitions if
the literal interpretation of SRFI-9 is used, even if that code would
run perfectly fine if entered at the REPL or being executed as an R6RS
script (modulo the import statement, but the test runner can work around
that).  This is certainly a special use case, but I think it is
reasonable one.

Regards, Rotty
-- 
Andreas Rottmann -- <http://rotty.yi.org/>

Re: [PATCH] Don't mix definitions and expressions in SRFI-9

[Andy Wingo]

On Mon 07 Mar 2011 01:31, Andreas Rottmann <a.rottmann@gmx.at> writes:

> I have a testing framework (built upon Riastradh's trc-testing), which
> uses R6RS `eval' to load testcases.  Since `eval' does not allow for
> the code to be evaluated to be in a top-level context, I'm using this:
>
> (eval `(let () ,@code-to-be-tested) the-environment-for-the-code)

It would seem that if we followed the spec strictly -- not an approach
that I think is sensible -- you may not use R6RS `eval' to define record
types.

Regards,

Andy
-- 
http://wingolog.org/

Re: [PATCH] Don't mix definitions and expressions in SRFI-9

[Andy Wingo]

On Sun 06 Mar 2011 23:26, ludo@gnu.org (Ludovic Courtès) writes:

> Andreas Rottmann <a.rottmann@gmx.at> writes:
>
>> The expansion of `define-inlinable' contained an expression, which made
>> SRFI-9's `define-record-type' fail in non-toplevel contexts ("definition
>> used in expression context").
>
> SRFI-9 says “Record-type definitions may only occur at top-level”, and
> I’m inclined to stick to it.  If we diverge, then people could write
> code thinking it’s portable SRFI-9 code while it’s not.

Does anyone actually care about this?  We provide many compatible
extensions to standard interfaces.  It seems like this would be an
"unnecessary restriction which makes `let-record-type' seem necessary".
Especially given that the patch actually removes an unused line :)

Regards,

Andy
-- 
http://wingolog.org/

Re: [PATCH] Don't mix definitions and expressions in SRFI-9

[Ludovic Courtès]

Hi,

Andy Wingo <wingo@pobox.com> writes:

> On Sun 06 Mar 2011 23:26, ludo@gnu.org (Ludovic Courtès) writes:
>
>> Andreas Rottmann <a.rottmann@gmx.at> writes:
>>
>>> The expansion of `define-inlinable' contained an expression, which made
>>> SRFI-9's `define-record-type' fail in non-toplevel contexts ("definition
>>> used in expression context").
>>
>> SRFI-9 says “Record-type definitions may only occur at top-level”, and
>> I’m inclined to stick to it.  If we diverge, then people could write
>> code thinking it’s portable SRFI-9 code while it’s not.
>
> Does anyone actually care about this?  We provide many compatible
> extensions to standard interfaces.  It seems like this would be an
> "unnecessary restriction which makes `let-record-type' seem necessary".

OK, I lost.  ;-)

But, can we:

  1. Document the extension.

  2. Choose PROC-NAME such that -Wunused-toplevel won’t complain.
     There’s a trick for this: if it contains white space, then
     -Wunused-toplevel won’t complain; however, it has to be generated
     deterministically because it can appear in other compilation units,
     so we can’t use ‘generate-temporaries’ here.

Thanks,
Ludo’.

Re: [PATCH] Don't mix definitions and expressions in SRFI-9

[Andreas Rottmann]

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

ludo@gnu.org (Ludovic Courtès) writes:

> Hi,
>
> Andy Wingo <wingo@pobox.com> writes:
>
>> On Sun 06 Mar 2011 23:26, ludo@gnu.org (Ludovic Courtès) writes:
>>
>>> Andreas Rottmann <a.rottmann@gmx.at> writes:
>>>
>>>> The expansion of `define-inlinable' contained an expression, which made
>>>> SRFI-9's `define-record-type' fail in non-toplevel contexts ("definition
>>>> used in expression context").
>>>
>>> SRFI-9 says “Record-type definitions may only occur at top-level”, and
>>> I’m inclined to stick to it.  If we diverge, then people could write
>>> code thinking it’s portable SRFI-9 code while it’s not.
>>
>> Does anyone actually care about this?  We provide many compatible
>> extensions to standard interfaces.  It seems like this would be an
>> "unnecessary restriction which makes `let-record-type' seem necessary".
>
> OK, I lost.  ;-)
>
> But, can we:
>
>   1. Document the extension.
>
>   2. Choose PROC-NAME such that -Wunused-toplevel won’t complain.
>      There’s a trick for this: if it contains white space, then
>      -Wunused-toplevel won’t complain; however, it has to be generated
>      deterministically because it can appear in other compilation units,
>      so we can’t use ‘generate-temporaries’ here.
>
I think the attached version of the patch takes your suggestions into
account.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: srfi-9-fix.diff --]
[-- Type: text/x-diff, Size: 4589 bytes --]

From: Andreas Rottmann <a.rottmann@gmx.at>
Subject: Don't mix definitions and expressions in SRFI-9

The expansion of `define-inlinable' contained an expression, which made
SRFI-9's `define-record-type' fail in non-toplevel contexts ("definition
used in expression context").

* module/srfi/srfi-9.scm (define-inlinable): Get rid of apparently
  useless expression in the expansion, so the expansion yields only
  definitions.  At the same time, use a space in the generated names to
  lessen the chances of name conflicts, also avoiding -Wunused-toplevel
  warnings.
* test-suite/tests/srfi-9.test (non-toplevel): New test verifying that
  `define-record-type' works in non-toplevel context as well.
* doc/ref/srfi-modules.texi (SRFI-9 - define-record-type): Add
  subsubsection noting that Guile does not enforce top-levelness.

---
 doc/ref/srfi-modules.texi    |    9 ++++++++-
 module/srfi/srfi-9.scm       |    8 +++++---
 test-suite/tests/srfi-9.test |   12 +++++++++++-
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/doc/ref/srfi-modules.texi b/doc/ref/srfi-modules.texi
index bda7cbb..eab8779 100644
--- a/doc/ref/srfi-modules.texi
+++ b/doc/ref/srfi-modules.texi
@@ -1,6 +1,6 @@
 @c -*-texinfo-*-
 @c This is part of the GNU Guile Reference Manual.
-@c Copyright (C)  1996, 1997, 2000, 2001, 2002, 2003, 2004, 2006, 2007, 2008, 2009, 2010
+@c Copyright (C)  1996, 1997, 2000, 2001, 2002, 2003, 2004, 2006, 2007, 2008, 2009, 2010, 2011
 @c   Free Software Foundation, Inc.
 @c See the file guile.texi for copying conditions.
 
@@ -1927,6 +1927,13 @@ The functions created by @code{define-record-type} are ordinary
 top-level @code{define}s.  They can be redefined or @code{set!} as
 desired, exported from a module, etc.
 
+@unnumberedsubsubsec Non-toplevel Record Definitions
+
+The SRFI-9 specification explicitly disallows record definitions in a
+non-toplevel context, such as inside @code{lambda} body or inside a
+@var{let} block.  However, Guile's implementation does not enforce that
+restriction.
+
 @unnumberedsubsubsec Custom Printers
 
 You may use @code{set-record-type-printer!} to customize the default printing
diff --git a/module/srfi/srfi-9.scm b/module/srfi/srfi-9.scm
index 80c3b60..fad570b 100644
--- a/module/srfi/srfi-9.scm
+++ b/module/srfi/srfi-9.scm
@@ -1,6 +1,6 @@
 ;;; srfi-9.scm --- define-record-type
 
-;; 	Copyright (C) 2001, 2002, 2006, 2009, 2010 Free Software Foundation, Inc.
+;; 	Copyright (C) 2001, 2002, 2006, 2009, 2010, 2011 Free Software Foundation, Inc.
 ;;
 ;; This library is free software; you can redistribute it and/or
 ;; modify it under the terms of the GNU Lesser General Public
@@ -69,9 +69,12 @@
   ;; the macro expansion, whereas references in non-call contexts refer to
   ;; the procedure.  Inspired by the `define-integrable' macro by Dybvig et al.
   (lambda (x)
+    ;; Use a space in the prefix to avoid potential -Wunused-toplevel
+    ;; warning
+    (define prefix (string->symbol "% "))
     (define (make-procedure-name name)
       (datum->syntax name
-                     (symbol-append '% (syntax->datum name)
+                     (symbol-append prefix (syntax->datum name)
                                     '-procedure)))
 
     (syntax-case x ()
@@ -81,7 +84,6 @@
          #`(begin
              (define (proc-name formals ...)
                body ...)
-             proc-name ;; unused
              (define-syntax name
                (lambda (x)
                  (syntax-case x ()
diff --git a/test-suite/tests/srfi-9.test b/test-suite/tests/srfi-9.test
index cf933a8..f8006c4 100644
--- a/test-suite/tests/srfi-9.test
+++ b/test-suite/tests/srfi-9.test
@@ -1,7 +1,7 @@
 ;;;; srfi-9.test --- Test suite for Guile's SRFI-9 functions. -*- scheme -*-
 ;;;; Martin Grabmueller, 2001-05-10
 ;;;;
-;;;; Copyright (C) 2001, 2006, 2007, 2010 Free Software Foundation, Inc.
+;;;; Copyright (C) 2001, 2006, 2007, 2010, 2011 Free Software Foundation, Inc.
 ;;;; 
 ;;;; This library is free software; you can redistribute it and/or
 ;;;; modify it under the terms of the GNU Lesser General Public
@@ -93,3 +93,13 @@
   ;; prior to guile 1.6.9 and 1.8.1 this wan't enforced
   (pass-if-exception "set-y! on bar" exception:wrong-type-arg
      (set-y! b 99)))
+
+(with-test-prefix "non-toplevel"
+    
+  (define-record-type :frotz (make-frotz a b) frotz?
+    (a frotz-a) (b frotz-b set-frotz-b!))
+
+  (pass-if "construction"
+    (let ((frotz (make-frotz 1 2)))
+      (and (= (frotz-a frotz) 1)
+           (= (frotz-b frotz) 2)))))
-- 
tg: (d59dd06..) t/srfi-9-fix (depends on: stable-2.0)

[-- Attachment #3: Type: text/plain, Size: 62 bytes --]


Thanks, Rotty
-- 
Andreas Rottmann -- <http://rotty.yi.org/>

Re: [PATCH] Don't mix definitions and expressions in SRFI-9

[Ludovic Courtès]

Hi Andreas,

Andreas Rottmann <a.rottmann@gmx.at> writes:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>> Hi,
>>
>> Andy Wingo <wingo@pobox.com> writes:
>>
>>> On Sun 06 Mar 2011 23:26, ludo@gnu.org (Ludovic Courtès) writes:
>>>
>>>> Andreas Rottmann <a.rottmann@gmx.at> writes:
>>>>
>>>>> The expansion of `define-inlinable' contained an expression, which made
>>>>> SRFI-9's `define-record-type' fail in non-toplevel contexts ("definition
>>>>> used in expression context").
>>>>
>>>> SRFI-9 says “Record-type definitions may only occur at top-level”, and
>>>> I’m inclined to stick to it.  If we diverge, then people could write
>>>> code thinking it’s portable SRFI-9 code while it’s not.
>>>
>>> Does anyone actually care about this?  We provide many compatible
>>> extensions to standard interfaces.  It seems like this would be an
>>> "unnecessary restriction which makes `let-record-type' seem necessary".
>>
>> OK, I lost.  ;-)
>>
>> But, can we:
>>
>>   1. Document the extension.
>>
>>   2. Choose PROC-NAME such that -Wunused-toplevel won’t complain.
>>      There’s a trick for this: if it contains white space, then
>>      -Wunused-toplevel won’t complain; however, it has to be generated
>>      deterministically because it can appear in other compilation units,
>>      so we can’t use ‘generate-temporaries’ here.
>>
> I think the attached version of the patch takes your suggestions into
> account.

Thanks, applied!

Ludo’.