jackfirth / resyntax

A Racket refactoring engine
Apache License 2.0
56 stars 10 forks source link

Don't replace syntax when output has shadowed ids #166

Closed AlexKnauth closed 1 year ago

AlexKnauth commented 1 year ago
#lang racket/base

(define (and x y) (list x y))

(define (member? e l)
  (if (member e l)
      #t
      #f))

should not be refactored to (and (member e l) #t) because and is shadowed.

AlexKnauth commented 1 year ago

I have added more tests. However, I am surprised that the final test I added seems to work, when I expected my changes to be insufficient to catch it:

test: "shadowed `define-syntax-rule`: single-clause syntax-rules macro"
------------------------------
(define-syntax define-syntax-rule
  (syntax-rules ()
    [(define-syntax-rule new old)
     (define-syntax new
       (syntax-rules ()
         [(new . args) (old . args)]))]))
(define-syntax my-or
  (syntax-rules ()
    [(my-or a b)
     (let ([tmp a])
       (if a a b))]))
------------------------------
AlexKnauth commented 1 year ago

After adding 2 more shadowed-output tests I have a guess about what's going on. I had been confused because it seemed to me to have information about the "future" expansion of the syntax object that it's looking at. But then I realized the way resyntax uses current-expand-observe observe-event!, it actually fully expands the program before doing any analyses, not just the "partial expansion" that I had imagined in my head from seeing un-expanded syntax objects. Since expansion continues after the expand-observe event, the expander state contains the information it needs to detect these conflicts.

AlexKnauth commented 1 year ago

Since expansion continues after the expand-observe event, the expander state contains the information it needs to detect these conflicts.

This same principle also allows it to detect the let-to-define tricky case