bugMIT/GNU Scheme - Bugs: bug #63438, Macro hygiene is broken

 
 

bug #63438: Macro hygiene is broken

Submitter:  Taylor R. Campbell <riastradh>
Submitted:  Tue 29 Nov 2022 02:41:59 AM UTC
 
Category:  runtime Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect behavior
Status:  Fixed Privacy:  Public
Assigned to:  cph Originator Name: 
Open/Closed:  Closed
Keywords: 
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       No canned response available

 

Thu 01 Dec 2022 01:04:20 PM UTC, comment #5: 

I just noticed another problem with the renaming: it doesn't appear to do anything to guarantee the generated name didn't appear in the input.

(let ((.x.1-0 123))
  (define-syntax foo
    (syntax-rules ()
      ((foo y) (lambda (x) y))))
  ((foo .x.1-0) 456))
;Value: 456

Value should be 123.  Problem is that the macro expander blithely generates exactly the same symbol as the input was using, .x.1-0:

(pp (syntax '(let ((.x.1-0 123))
               (define-syntax foo
                 (syntax-rules ()
                   ((foo y) (lambda (x) y))))
               ((foo .x.1-0) 456))
            (nearest-repl/environment)))
(let ((.x.1-0 123))
  (let ((.x.1-0 456))
    .x.1-0))

Taylor R. Campbell <riastradh>
Project Member
Thu 01 Dec 2022 01:00:34 PM UTC, comment #4: 

The attached patch seems to fix this particular case.  I haven't tried a full build.  It seems to do the right thing for other cases involving shadowing like this too:

(pp (syntax
     '(let ()
        (define-syntax foo
          (syntax-rules ()
            ((foo 0)
             (foo 1 x))
            ((foo 1 y)
             (lambda (x y)
               (list (list x y)
                     (lambda (y) (list x y)))))))
        (foo 0))
     (nearest-repl/environment)))
(let ()
  (lambda (.x.1-0 .x.1-1)
    (list (list .x.1-0 .x.1-1) (lambda (.x.2-0) (list .x.1-0 .x.2-0)))))

The trouble with the code as is is that when two different values of original with the same underlying symbol are interned in uninterned-table, they will both have empty buckets in uninterned-table.  So the output of (symbol "." (identifier->symbol original) "." frame-id "-" (length (cdr bucket))) collides.

The patch reverses the indexing so that (length (cdr bucket)) counts the number of bindings in the same frame, meaning if two have the same symbol, they will be renamed differently.  I suspect that there's no danger of the opposite kind of collision happening from this reversal of indexing because frame ids correspond to distinct shadowings (but I don't understand this code well enough to be sure).

(file #54044)

Taylor R. Campbell <riastradh>
Project Member
Wed 30 Nov 2022 10:57:18 AM UTC, comment #3: 

I see. Thank you for the clarification.

This looks like a problem with the syntax renaming postpass.

Chris Hanson <cph>
Project Administrator
Wed 30 Nov 2022 09:50:34 AM UTC, comment #2: 

I don't think syntax-rules is the problem.  Same thing with er-macro-transformer:

(define-syntax foo
  (er-macro-transformer
   (lambda (f r c)
     (case (cadr f)
       ((0) `(,(car f) 1 ,(r 'x)))
       ((1) `(,(car f) 2 ,(caddr f) ,(r 'x)))
       ((2) `(lambda ,(cddr f) (list ,@(cddr f))))
       (else (syntax-error 'foo))))))
;Value: foo

(foo 0)
;duplicate parameters (.x.1-0) in (.x.1-0 .x.1-0)

Taylor R. Campbell <riastradh>
Project Member
Wed 30 Nov 2022 06:59:17 AM UTC, comment #1: 

I'm in the process of a complete rewrite of syntax-rules.
I'll make sure that there's a test case for this.

Chris Hanson <cph>
Project Administrator
Tue 29 Nov 2022 02:41:59 AM UTC, original submission:  

(define-syntax foo
  (syntax-rules ()
    ((foo 0)
     (foo 1 (x)))
    ((foo 1 (x0))
     (foo 2 (x0 x)))
    ((foo 2 (x0 x1))
     (lambda (x0 x1) (list x0 x1)))))
;Value: foo

(foo 0)
;duplicate parameters (.x.1-0) in (.x.1-0 .x.1-0)

The identifiers `x' generated by distinct macro invocation should not collide, but they do.

Taylor R. Campbell <riastradh>
Project Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #54044:  rename.patch added by riastradh (762B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by cph (Posted a comment)
  • -email is unavailable- added by riastradh (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

     

     

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-12-06 cph StatusNone Fixed
        Open/ClosedOpen Closed
    2022-12-01 riastradh Attached File- Added rename.patch, #54044

    Back to the top

    Powered by Savane 3.9