[PATCH] Fix for `submodules' in (ice-9 session) (closes #30062)

[PATCH] Fix for `submodules' in (ice-9 session) (closes #30062)

[Jose A. Ortega Ruiz]

* module/ice-9/session.scm (submodules): replace implementation to
  use `module-submodules' instead of `module-obarray' (the latter
  doesn't include submodules anymore).

Signed-off-by: Jose A. Ortega Ruiz <jao@gnu.org>
---
 module/ice-9/session.scm |   11 ++---------
 1 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/module/ice-9/session.scm b/module/ice-9/session.scm
index 10ce613..36aeb99 100644
--- a/module/ice-9/session.scm
+++ b/module/ice-9/session.scm
@@ -406,15 +406,8 @@ It is an image under the mapping EXTRACT."
 (define (root-modules)
   (submodules (resolve-module '() #f)))
 
-(define (submodules m)
-  (hash-fold (lambda (name var data)
-	       (let ((obj (and (variable-bound? var) (variable-ref var))))
-		 (if (and (module? obj)
-			  (eq? (module-kind obj) 'directory))
-		     (cons obj data)
-		     data)))
-	     '()
-	     (module-obarray m)))
+(define (submodules mod)
+  (hash-map->list (lambda (k v) v) (module-submodules mod)))
 
 (define apropos-fold-exported
   (make-fold-modules root-modules submodules module-public-interface))
-- 
1.7.1

Re: [PATCH] Fix for `submodules' in (ice-9 session) (closes #30062)

[Andy Wingo]

Hi,

Can you submit a test please, also? This patch is correct, but with
--enable-deprecated builds, it should be unnecessary.

Andy

On Tue 31 Aug 2010 05:19, "Jose A. Ortega Ruiz" <jao@gnu.org> writes:

> * module/ice-9/session.scm (submodules): replace implementation to
>   use `module-submodules' instead of `module-obarray' (the latter
>   doesn't include submodules anymore).
>
> Signed-off-by: Jose A. Ortega Ruiz <jao@gnu.org>
> ---
>  module/ice-9/session.scm |   11 ++---------
>  1 files changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/module/ice-9/session.scm b/module/ice-9/session.scm
> index 10ce613..36aeb99 100644
> --- a/module/ice-9/session.scm
> +++ b/module/ice-9/session.scm
> @@ -406,15 +406,8 @@ It is an image under the mapping EXTRACT."
>  (define (root-modules)
>    (submodules (resolve-module '() #f)))
>  
> -(define (submodules m)
> -  (hash-fold (lambda (name var data)
> -	       (let ((obj (and (variable-bound? var) (variable-ref var))))
> -		 (if (and (module? obj)
> -			  (eq? (module-kind obj) 'directory))
> -		     (cons obj data)
> -		     data)))
> -	     '()
> -	     (module-obarray m)))
> +(define (submodules mod)
> +  (hash-map->list (lambda (k v) v) (module-submodules mod)))
>  
>  (define apropos-fold-exported
>    (make-fold-modules root-modules submodules module-public-interface))

-- 
http://wingolog.org/

Re: [PATCH] Fix for `submodules' in (ice-9 session) (closes #30062)

[Jose A. Ortega Ruiz]

On Tue, Aug 31 2010, Andy Wingo wrote:

> Hi,
>
> Can you submit a test please, also?

I'm not quite sure what you want the test to be: the procedure
`submodules' is not exported by `(ice-9 session)', and there's no test
suite for the latter, so testing this change properly would mean
creating such a test suite, checking all directly affected exported
procedures. Those are `apropos-fold-exported' and
`apropos-fold-accessible'.

Testing them is a bit involved, due to the cycles in the module tree
that we have discussed in the past, and that imply that any fold that
does not keep track of visited modules and escapes somehow upon
detecting a cycle, will enter an infinite loop. Even when escaping,
one's not guaranteed to visit all modules. IIRC, those cycles cannot be
eliminated for backwards compatibility reasons.

IMHO, given the cycles, the apropos-fold* functions are broken (one
cannot fold over cyclic structures), and maybe should be fixed instead
of applying only this partial fix. A possible solution is making
apropos-fold-* work on a real DAG, by prunning cycles during the
traversal.

IIRC, the modules causing cycles where among those having gensym's as
names: if they were catchable by a predicate somehow, and easy solution
would be to filter them out in `submodules' -- if for some reason a user
needs a gensym module (i don't know what they are or whether they're
part of the public module interface), she can access it via
`module-submodules'.

jao

Re: [PATCH] Fix for `submodules' in (ice-9 session) (closes #30062)

[Jose A. Ortega Ruiz]

Hi again,

On Tue, Aug 31 2010, Jose A. Ortega Ruiz wrote:

> On Tue, Aug 31 2010, Andy Wingo wrote:
>
>> Hi,
>>
>> Can you submit a test please, also?
>
> I'm not quite sure what you want the test to be: the procedure
> `submodules' is not exported by `(ice-9 session)', and there's no test
> suite for the latter, so testing this change properly would mean
> creating such a test suite, checking all directly affected exported
> procedures. Those are `apropos-fold-exported' and
> `apropos-fold-accessible'.
>
> Testing them is a bit involved, due to the cycles in the module tree

[... blah blah blah ...]

Please, ignore all the above stupid chatter. The fold functions are
already testing for duplicates and there's no infinite loop anywhere.
Sorry about my big mouth. (I'm still curious about the meaning of the
modules with gensyms as names, though.)

So, i'll write indirect tests for `submodules' by testing
`apropos-fold-exported' and `apropos-fold-all' (i didn't even get the
name right!). Would that work for you?

Thanks,

jao
-- 
"I didn't do it, and I'll never do it again."
 -Derrik Weeks

Re: [PATCH] Fix for `submodules' in (ice-9 session) (closes #30062)

[Ludovic Courtès]

Hi!

"Jose A. Ortega Ruiz" <jao@gnu.org> writes:

> (I'm still curious about the meaning of the
> modules with gensyms as names, though.)

psyntax expects modules to have a name so that it can refer to them in
expanded code.  Thus, there can be no anonymous modules: modules are
always given a name, see ‘module-name’.  This allows things like the
“compile in fresh module” test to work.

Thanks,
Ludo’.

Re: [PATCH] Fix for `submodules' in (ice-9 session) (closes #30062)

[Jose A. Ortega Ruiz]

On Wed, Sep 01 2010, Ludovic Courtès wrote:

> Hi!
>
> "Jose A. Ortega Ruiz" <jao@gnu.org> writes:
>
>> (I'm still curious about the meaning of the
>> modules with gensyms as names, though.)
>
> psyntax expects modules to have a name so that it can refer to them in
> expanded code.  Thus, there can be no anonymous modules: modules are
> always given a name, see ‘module-name’.  This allows things like the
> “compile in fresh module” test to work.

I see. But then, aren't those modules something internal to psyntax's
workings? And if so, shouldn't they be filtered out from the return
value of module-submodules (or not be traversed by the apropos-fold)? As
a user of those procedures, i find the appearance of those modules a bit
confusing (the only use case in client code i can think of is when using
the return value of current-module). Am i missing something?

Thanks,
jao
-- 
Nature uses as little as possible of anything.
  -Johannes Kepler, astronomer (1571-1630)

Re: [PATCH] Fix for `submodules' in (ice-9 session) (closes #30062)

[Ludovic Courtès]

Hello!

"Jose A. Ortega Ruiz" <jao@gnu.org> writes:

> On Wed, Sep 01 2010, Ludovic Courtès wrote:
>
>> Hi!
>>
>> "Jose A. Ortega Ruiz" <jao@gnu.org> writes:
>>
>>> (I'm still curious about the meaning of the
>>> modules with gensyms as names, though.)
>>
>> psyntax expects modules to have a name so that it can refer to them in
>> expanded code.  Thus, there can be no anonymous modules: modules are
>> always given a name, see ‘module-name’.  This allows things like the
>> “compile in fresh module” test to work.
>
> I see. But then, aren't those modules something internal to psyntax's
> workings?

No, they’re not internal.  They’re just (pseudo-)anonymous modules that
ended up in the module hierarchy, like any other module.  Evaluate
(module-name (make-module)) and you’ve added another one.  :-)

> And if so, shouldn't they be filtered out from the return value of
> module-submodules (or not be traversed by the apropos-fold)? As a user
> of those procedures, i find the appearance of those modules a bit
> confusing (the only use case in client code i can think of is when
> using the return value of current-module). Am i missing something?

I agree that as users we’d rather not see these modules, especially from
Geiser.  But they have to be there.

So, unless I’m missing an elegant design trick to avoid this, I think
you’re bound to use heuristics to filter them out (e.g., get rid of
modules whose name contains white spaces.)

Thanks,
Ludo’.

Re: [PATCH] Fix for `submodules' in (ice-9 session) (closes #30062)

[Jose A. Ortega Ruiz]

Hi Ludo,

On Thu, Sep 02 2010, Ludovic Courtès wrote:

> Hello!
>

[...]

>>> psyntax expects modules to have a name so that it can refer to them in
>>> expanded code.  Thus, there can be no anonymous modules: modules are
>>> always given a name, see ‘module-name’.  This allows things like the
>>> “compile in fresh module” test to work.
>>
>> I see. But then, aren't those modules something internal to psyntax's
>> workings?
>
> No, they’re not internal.  They’re just (pseudo-)anonymous modules that
> ended up in the module hierarchy, like any other module.  Evaluate
> (module-name (make-module)) and you’ve added another one.  :-)

Oh, i see. (Then, i could/should probably test anonymous modules in
the new session.test too.)

>> And if so, shouldn't they be filtered out from the return value of
>> module-submodules (or not be traversed by the apropos-fold)? As a user
>> of those procedures, i find the appearance of those modules a bit
>> confusing (the only use case in client code i can think of is when
>> using the return value of current-module). Am i missing something?
>
> I agree that as users we’d rather not see these modules, especially from
> Geiser.  But they have to be there.
>
> So, unless I’m missing an elegant design trick to avoid this, I think
> you’re bound to use heuristics to filter them out (e.g., get rid of
> modules whose name contains white spaces.)

Well, something along those lines is what Geiser is actually doing right
now :)

Thanks for the clarification.

Cheers,
jao
-- 
Genius may have its limitations, but stupidity is not thus handicapped.
 -Elbert Hubbard

Re: [PATCH] Fix for `submodules' in (ice-9 session) (closes #30062)

[Andy Wingo]

On Wed 01 Sep 2010 15:23, ludo@gnu.org (Ludovic Courtès) writes:

> So, unless I’m missing an elegant design trick to avoid this, I think
> you’re bound to use heuristics to filter them out (e.g., get rid of
> modules whose name contains white spaces.)

We can add flags to modules to indicate that they are temporary; not
"user modules".

Andy
-- 
http://wingolog.org/

Re: [PATCH] Fix for `submodules' in (ice-9 session) (closes #30062)

[Jose A. Ortega Ruiz]

On Fri, Sep 03 2010, Andy Wingo wrote:

> On Wed 01 Sep 2010 15:23, ludo@gnu.org (Ludovic Courtès) writes:
>
>> So, unless I’m missing an elegant design trick to avoid this, I think
>> you’re bound to use heuristics to filter them out (e.g., get rid of
>> modules whose name contains white spaces.)
>
> We can add flags to modules to indicate that they are temporary; not
> "user modules".

That'd be great. Then, we could add a (keyword) parameter to the folding
functions to exclude (or, i think i would prefer, include) temporary
modules in the traversal (or create new functions doing that).

jao

Re: [PATCH] Fix for `submodules' in (ice-9 session) (closes #30062)

[Andy Wingo]

Hi,

On Fri 03 Sep 2010 18:13, "Jose A. Ortega Ruiz" <jao@gnu.org> writes:

> On Fri, Sep 03 2010, Andy Wingo wrote:
>
>> We can add flags to modules to indicate that they are temporary; not
>> "user modules".
>
> That'd be great. Then, we could add a (keyword) parameter to the folding
> functions to exclude (or, i think i would prefer, include) temporary
> modules in the traversal (or create new functions doing that).

You can already filter out modules whose "kind" is #f:

  scheme@(guile-user)> (module-kind (resolve-module '(ice-9 q)))
  $3 = directory
  scheme@(guile-user)> (module-kind (make-fresh-user-module))
  $4 = #f

I don't think we need an extra flag. However module "kinds" need some
retroactive design, I think...

Andy
-- 
http://wingolog.org/

Re: [PATCH] Fix for `submodules' in (ice-9 session) (closes #30062)

[Jose A. Ortega Ruiz]

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

On Tue, Aug 31 2010, Andy Wingo wrote:

> Hi,
>
> Can you submit a test please, also? This patch is correct, but with
> --enable-deprecated builds, it should be unnecessary.

Okay, test added (i'm not sure if there's something to do about
--enable-deprecated builds), and patch attached.

Cheers,
jao


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-for-submodules-in-ice-9-session-closes-30062.patch --]
[-- Type: text/x-diff, Size: 4497 bytes --]

From b29148d72882e5840fbe9242ccbd17be14f42545 Mon Sep 17 00:00:00 2001
From: Jose A. Ortega Ruiz <jao@gnu.org>
Date: Tue, 31 Aug 2010 14:13:43 +0200
Subject: [PATCH] Fix for `submodules' in (ice-9 session) (closes #30062)

* module/ice-9/session.scm (submodules): replace implementation to
  use `module-submodules' instead of `module-obarray' (the latter
  doesn't include submodules anymore).

* test-suite/tests/session.test: new test suite for session, checking
  the exported procedures that use `submodules'.

Signed-off-by: Jose A. Ortega Ruiz <jao@gnu.org>
---
 module/ice-9/session.scm      |   11 +-------
 test-suite/Makefile.am        |    1 +
 test-suite/tests/session.test |   50 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 9 deletions(-)
 create mode 100644 test-suite/tests/session.test

diff --git a/module/ice-9/session.scm b/module/ice-9/session.scm
index 10ce613..36aeb99 100644
--- a/module/ice-9/session.scm
+++ b/module/ice-9/session.scm
@@ -406,15 +406,8 @@ It is an image under the mapping EXTRACT."
 (define (root-modules)
   (submodules (resolve-module '() #f)))
 
-(define (submodules m)
-  (hash-fold (lambda (name var data)
-	       (let ((obj (and (variable-bound? var) (variable-ref var))))
-		 (if (and (module? obj)
-			  (eq? (module-kind obj) 'directory))
-		     (cons obj data)
-		     data)))
-	     '()
-	     (module-obarray m)))
+(define (submodules mod)
+  (hash-map->list (lambda (k v) v) (module-submodules mod)))
 
 (define apropos-fold-exported
   (make-fold-modules root-modules submodules module-public-interface))
diff --git a/test-suite/Makefile.am b/test-suite/Makefile.am
index eaa7512..c779eac 100644
--- a/test-suite/Makefile.am
+++ b/test-suite/Makefile.am
@@ -100,6 +100,7 @@ SCM_TESTS = tests/00-initial-env.test		\
 	    tests/reader.test			\
 	    tests/receive.test			\
 	    tests/regexp.test			\
+	    tests/session.test			\
 	    tests/signals.test			\
 	    tests/socket.test			\
 	    tests/srcprop.test			\
diff --git a/test-suite/tests/session.test b/test-suite/tests/session.test
new file mode 100644
index 0000000..5493209
--- /dev/null
+++ b/test-suite/tests/session.test
@@ -0,0 +1,50 @@
+;;;; session.test --- test suite for (ice-9 session)   -*- scheme -*-
+;;;; Jose Antonio Ortega Ruiz <jao@gnu.org> -- August 2010
+;;;;
+;;;; 	Copyright (C) 2010 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
+;;;; License as published by the Free Software Foundation; either
+;;;; version 3 of the License, or (at your option) any later version.
+;;;;
+;;;; This library is distributed in the hope that it will be useful,
+;;;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+;;;; Lesser General Public License for more details.
+;;;;
+;;;; You should have received a copy of the GNU Lesser General Public
+;;;; License along with this library; if not, write to the Free Software
+;;;; Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+;;;; 02110-1301 USA
+
+(define-module (test-suite session)
+  #:use-module (test-suite lib)
+  #:use-module (ice-9 session))
+
+(define (find-module mod-name)
+  (let ((mod (resolve-module mod-name #f #:ensure #f)))
+    (call/cc (lambda (k)
+               (apropos-fold-all (lambda (m _)
+                                   (and (not (module? m)) (k #f))
+                                   (and (eq? m mod) (k #t)))
+                                 #f)))))
+
+(with-test-prefix "apropos-fold-all"
+  (pass-if "a root module: ice-9" (find-module '(ice-9)))
+  (pass-if "a child of test-suite" (find-module '(test-suite lib)))
+  (pass-if "a non-module" (not (find-module '(ice-999-0))))
+  (pass-if "a childish non-module" (not (find-module '(ice-9 ice-999-0)))))
+
+(define (find-interface mod-name)
+  (let* ((mod (resolve-module mod-name #f #:ensure #f))
+         (ifc (and mod (module-public-interface mod))))
+    (and ifc
+         (call/cc (lambda (k)
+                    (apropos-fold-exported (lambda (i _)
+                                             (and (eq? i ifc) (k #t)))
+                                           #f))))))
+
+(with-test-prefix "apropos-fold-exported"
+  (pass-if "a child of test-suite" (find-interface '(test-suite lib)))
+  (pass-if "a child of ice-9" (find-interface '(ice-9 session))))
-- 
1.7.1

Re: [PATCH] Fix for `submodules' in (ice-9 session) (closes #30062)

[Jose A. Ortega Ruiz]

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


Heya again,

On Wed, Sep 01 2010, Jose A. Ortega Ruiz wrote:

> On Tue, Aug 31 2010, Andy Wingo wrote:
>
>> Hi,
>>
>> Can you submit a test please, also? This patch is correct, but with
>> --enable-deprecated builds, it should be unnecessary.
>
> Okay, test added (i'm not sure if there's something to do about
> --enable-deprecated builds), and patch attached.

Sorry for self-following-up, but, after the conversation with Ludo, i
added a new test using an anonymous module. The new version of the patch
is attached: does it look OK for pushing?

Thanks!
jao


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-for-submodules-in-ice-9-session-closes-30062.patch --]
[-- Type: text/x-diff, Size: 4606 bytes --]

From 58efa6f2f7ef6bd2d710e1b996b6752e9e4c5093 Mon Sep 17 00:00:00 2001
From: Jose A. Ortega Ruiz <jao@gnu.org>
Date: Tue, 31 Aug 2010 14:13:43 +0200
Subject: [PATCH] Fix for `submodules' in (ice-9 session) (closes #30062)

* module/ice-9/session.scm (submodules): replace implementation to
  use `module-submodules' instead of `module-obarray' (the latter
  doesn't include submodules anymore).

* test-suite/tests/session.test: new test suite for session, checking
  the exported procedures that use `submodules'.

Signed-off-by: Jose A. Ortega Ruiz <jao@gnu.org>
---
 module/ice-9/session.scm      |   11 +-------
 test-suite/Makefile.am        |    1 +
 test-suite/tests/session.test |   53 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 9 deletions(-)
 create mode 100644 test-suite/tests/session.test

diff --git a/module/ice-9/session.scm b/module/ice-9/session.scm
index 10ce613..36aeb99 100644
--- a/module/ice-9/session.scm
+++ b/module/ice-9/session.scm
@@ -406,15 +406,8 @@ It is an image under the mapping EXTRACT."
 (define (root-modules)
   (submodules (resolve-module '() #f)))
 
-(define (submodules m)
-  (hash-fold (lambda (name var data)
-	       (let ((obj (and (variable-bound? var) (variable-ref var))))
-		 (if (and (module? obj)
-			  (eq? (module-kind obj) 'directory))
-		     (cons obj data)
-		     data)))
-	     '()
-	     (module-obarray m)))
+(define (submodules mod)
+  (hash-map->list (lambda (k v) v) (module-submodules mod)))
 
 (define apropos-fold-exported
   (make-fold-modules root-modules submodules module-public-interface))
diff --git a/test-suite/Makefile.am b/test-suite/Makefile.am
index eaa7512..c779eac 100644
--- a/test-suite/Makefile.am
+++ b/test-suite/Makefile.am
@@ -100,6 +100,7 @@ SCM_TESTS = tests/00-initial-env.test		\
 	    tests/reader.test			\
 	    tests/receive.test			\
 	    tests/regexp.test			\
+	    tests/session.test			\
 	    tests/signals.test			\
 	    tests/socket.test			\
 	    tests/srcprop.test			\
diff --git a/test-suite/tests/session.test b/test-suite/tests/session.test
new file mode 100644
index 0000000..1697471
--- /dev/null
+++ b/test-suite/tests/session.test
@@ -0,0 +1,53 @@
+;;;; session.test --- test suite for (ice-9 session)   -*- scheme -*-
+;;;; Jose Antonio Ortega Ruiz <jao@gnu.org> -- August 2010
+;;;;
+;;;; 	Copyright (C) 2010 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
+;;;; License as published by the Free Software Foundation; either
+;;;; version 3 of the License, or (at your option) any later version.
+;;;;
+;;;; This library is distributed in the hope that it will be useful,
+;;;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+;;;; Lesser General Public License for more details.
+;;;;
+;;;; You should have received a copy of the GNU Lesser General Public
+;;;; License along with this library; if not, write to the Free Software
+;;;; Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+;;;; 02110-1301 USA
+
+(define-module (test-suite session)
+  #:use-module (test-suite lib)
+  #:use-module (ice-9 session))
+
+(define (find-module mod)
+  (call/cc (lambda (k)
+             (apropos-fold-all (lambda (m _)
+                                 (and (not (module? m)) (k #f))
+                                 (and (eq? m mod) (k #t)))
+                               #f))))
+(define (find-mod-name mod-name)
+  (find-module (resolve-module mod-name #f #:ensure #f)))
+
+
+(with-test-prefix "apropos-fold-all"
+  (pass-if "a root module: ice-9" (find-mod-name '(ice-9)))
+  (pass-if "a child of test-suite" (find-mod-name '(test-suite lib)))
+  (pass-if "a non-module" (not (find-mod-name '(ice-999-0))))
+  (pass-if "a childish non-module" (not (find-mod-name '(ice-9 ice-999-0))))
+  (pass-if "an anonymous module" (find-mod-name (module-name (make-module)))))
+
+(define (find-interface mod-name)
+  (let* ((mod (resolve-module mod-name #f #:ensure #f))
+         (ifc (and mod (module-public-interface mod))))
+    (and ifc
+         (call/cc (lambda (k)
+                    (apropos-fold-exported (lambda (i _)
+                                             (and (eq? i ifc) (k #t)))
+                                           #f))))))
+
+(with-test-prefix "apropos-fold-exported"
+  (pass-if "a child of test-suite" (find-interface '(test-suite lib)))
+  (pass-if "a child of ice-9" (find-interface '(ice-9 session))))
-- 
1.7.1