Re: [PATCH] Fix #1757 by not merging module environment into the syntactic environments of reexported macros

2021-07-31 Thread Peter Bex
On Sat, Jul 31, 2021 at 04:05:52PM +1200, Evan Hanson wrote:
> I was wondering whether there are any implications in changing the order
> of the `sexports` field of the module, now that we're putting all
> "normal" syntax exports before any re-exported ones. However, after a
> bit of digging, I don't think this should matter?

Good call, I hadn't realised this was changing the order.  Although
you're right - the order shouldn't really matter.

Cheers,
Peter


signature.asc
Description: PGP signature


Re: [PATCH] Fix #1757 by not merging module environment into the syntactic environments of reexported macros

2021-07-30 Thread Evan Hanson
On 2021-07-14 12:45, Peter Bex wrote:
> Here's a reasonably straightforward patch (I hope) for #1757.
> Commit should speak for itself, even if the stuff itself is
> a bit tricky.

It really is, this took some time to understand... But it does seem like
the right thing, now that you've spelled it out.

I was wondering whether there are any implications in changing the order
of the `sexports` field of the module, now that we're putting all
"normal" syntax exports before any re-exported ones. However, after a
bit of digging, I don't think this should matter?

Anyway, applied. Thank you Peter!

Evan



[PATCH] Fix #1757 by not merging module environment into the syntactic environments of reexported macros

2021-07-14 Thread Peter Bex
Hi all,

Here's a reasonably straightforward patch (I hope) for #1757.
Commit should speak for itself, even if the stuff itself is
a bit tricky.

I'm not 100% sure about the other places where "sexps" is used,
if we should use the local macros only there, or not.  All the
tests seem to work if we keep it referring to only local macros,
and I think we shouldn't mess too much with the environments
where that's not needed, so I think it's fine.

Cheers,
Peter
From 96827b77554e8b56838b0e47363326011b6e Mon Sep 17 00:00:00 2001
From: Peter Bex 
Date: Wed, 14 Jul 2021 12:18:56 +0200
Subject: [PATCH] Don't merge syntactic environment for reexported macros
 (#1757)

Separate the syntactic exports into local and reexported macros,
so we can merge the syntactic environment into the local macros.

The original code would merge the reexporting module's syntactic
environment into every macro it exports, but this is incorrect for
reexported macros - those should stick with their original syntactic
environment, as it was in the defining module.
---
 NEWS   |  4 
 distribution/manifest  |  2 ++
 modules.scm| 14 --
 tests/reexport-m7.scm  | 17 +
 tests/reexport-m8.scm  |  8 
 tests/reexport-tests-2.scm |  6 ++
 tests/runtests.bat |  4 
 tests/runtests.sh  |  2 ++
 8 files changed, 51 insertions(+), 6 deletions(-)
 create mode 100644 tests/reexport-m7.scm
 create mode 100644 tests/reexport-m8.scm

diff --git a/NEWS b/NEWS
index e65f885a..e0e2147d 100644
--- a/NEWS
+++ b/NEWS
@@ -33,6 +33,10 @@
   - Made topological-sort behave better when dependency target is listed
 multiple times by concatenating dependencies (fixes #1185).
 
+- Module system
+  - Reexported macros now work when the reexporting module redefines
+identifiers from the original (fixes #1757, reported by Sandra Snan).
+
 - Runtime system
   - Sleeping primordial thread doesn't forget mutations made to
 parameters in interrupt handlers anymore. (See #1638. Fix
diff --git a/distribution/manifest b/distribution/manifest
index 5416eff7..53aeaa4f 100644
--- a/distribution/manifest
+++ b/distribution/manifest
@@ -196,6 +196,8 @@ tests/reexport-m3.scm
 tests/reexport-m4.scm
 tests/reexport-m5.scm
 tests/reexport-m6.scm
+tests/reexport-m7.scm
+tests/reexport-m8.scm
 tests/reexport-tests.scm
 tests/reexport-tests-2.scm
 tests/ec.scm
diff --git a/modules.scm b/modules.scm
index 0ba1df49..77c9af52 100644
--- a/modules.scm
+++ b/modules.scm
@@ -385,16 +385,18 @@
 	   'import "cannot find implementation of re-exported syntax"
 	   name
   (let* ((sexps
-	  (map (lambda (se)
-		 (if (symbol? se)
-		 (find-reexport se)
-		 (list (car se) #f (##sys#ensure-transformer (cdr se) (car se)
-	   sexports))
+	  (filter-map (lambda (se)
+			(and (not (symbol? se))
+			 (list (car se) #f (##sys#ensure-transformer (cdr se) (car se)
+		  sexports))
+	 (reexp-sexps
+	  (filter-map (lambda (se) (and (symbol? se) (find-reexport se)))
+		  sexports))
 	 (nexps
 	  (map (lambda (ne)
 		 (list (car ne) #f (##sys#ensure-transformer (cdr ne) (car ne
 	   sdefs))
-	 (mod (make-module name lib '() vexports sexps iexports))
+	 (mod (make-module name lib '() vexports (append sexps reexp-sexps) iexports))
 	 (senv (if (or (not (null? sexps))  ; Only macros have an senv
 		   (not (null? nexps))) ; which must be patched up
 		   (merge-se
diff --git a/tests/reexport-m7.scm b/tests/reexport-m7.scm
new file mode 100644
index ..3c62fabf
--- /dev/null
+++ b/tests/reexport-m7.scm
@@ -0,0 +1,17 @@
+(module reexport-m7
+(reexported-reverse (reexported-local-reverse my-reverse))
+
+  (import scheme (chicken syntax))
+
+  (define (my-reverse lis)
+(reverse lis))
+
+  (define-syntax reexported-local-reverse
+(syntax-rules ()
+  ((_ lis)
+   (my-reverse lis
+
+  (define-syntax reexported-reverse
+(syntax-rules ()
+  ((_ lis)
+   (reverse lis)
diff --git a/tests/reexport-m8.scm b/tests/reexport-m8.scm
new file mode 100644
index ..2fe2b9ac
--- /dev/null
+++ b/tests/reexport-m8.scm
@@ -0,0 +1,8 @@
+(module reexport-m8 ()
+  ;; NOTE: Reexport only works properly if m7 is imported here, when
+  ;; the implementing library isn't required by the user of m8..
+  (import reexport-m7
+  (rename scheme (reverse reverse-og))
+	  (rename (chicken base) (identity reverse))
+	  (chicken module))
+  (reexport reexport-m7))
diff --git a/tests/reexport-tests-2.scm b/tests/reexport-tests-2.scm
index 14b51c5a..61d81993 100644
--- a/tests/reexport-tests-2.scm
+++ b/tests/reexport-tests-2.scm
@@ -6,3 +6,9 @@
 (import reexport-m6)
 (f:s1); expands to s2, which is reexported and refers to "s2", which is also visible in this context as "f:s2"
 (f:s2)
+
+;; reexport of syntax using shadowed identifiers in new module (#1757)
+(import reexport-m8)
+(assert (equal? '(d c b a)