Skip to content

Commit

Permalink
Implement proper macro scope resolution for generator/for (#53674)
Browse files Browse the repository at this point in the history
I'm not super familiar with this code, but it appears to me that these
need to be treated as equivalent to implicit let blocks for the purposes
of macro hygiene, so add appropriate logic to the macroexpander. Fixes
#53673.
  • Loading branch information
Keno authored Mar 13, 2024
1 parent b1dd26a commit 9ae7eab
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 8 deletions.
57 changes: 49 additions & 8 deletions src/macroexpand.scm
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,16 @@
(else '())))
(else '()))))))

;; for/generator
(pattern-lambda (for assgn body)
(if (eq? (car assgn) 'block)
`(varlist ,@(map cadr (cdr assgn)))
(cons 'varlist (cadr assgn))))
(pattern-lambda (generator body (filter filt . assgn))
(cons 'varlist (map (lambda (x) (cadr x)) assgn)))
(pattern-lambda (generator body . assgn)
(cons 'varlist (map (lambda (x) (cadr x)) assgn)))

;; macro definition
(pattern-lambda (macro (call name . argl) body)
`(-> (tuple ,@argl) ,body))
Expand Down Expand Up @@ -357,6 +367,25 @@
(else
(resolve-expansion-vars-with-new-env x env m parent-scope inarg)))))

(define (resolve-letlike-assign bind env newenv m parent-scope inarg)
(if (assignment? bind)
(make-assignment
;; expand binds in newenv with dummy RHS
(cadr (resolve-expansion-vars- (make-assignment (cadr bind) 0)
newenv m parent-scope inarg))
;; expand initial values in old env
(resolve-expansion-vars- (caddr bind) env m parent-scope inarg))
;; Just expand everything else that's not an assignment. N.B.: This includes
;; assignments inside escapes, which probably need special handling (TODO).
(resolve-expansion-vars- bind newenv m parent-scope inarg)))

(define (for-ranges-list ranges)
(if (eq? (car ranges) 'escape)
(map (lambda (range) `(escape ,range)) (for-ranges-list (cadr ranges)))
(if (eq? (car ranges) 'block)
(cdr ranges)
(list ranges))))

(define (resolve-expansion-vars- e env m parent-scope inarg)
(cond ((or (eq? e 'begin) (eq? e 'end) (eq? e 'ccall) (eq? e 'cglobal) (underscore-symbol? e))
e)
Expand Down Expand Up @@ -446,16 +475,28 @@
`(let (block
,@(map
(lambda (bind)
(if (assignment? bind)
(make-assignment
;; expand binds in old env with dummy RHS
(cadr (resolve-expansion-vars- (make-assignment (cadr bind) 0)
newenv m parent-scope inarg))
;; expand initial values in old env
(resolve-expansion-vars- (caddr bind) env m parent-scope inarg))
(resolve-expansion-vars- bind newenv m parent-scope inarg)))
(resolve-letlike-assign bind env newenv m parent-scope inarg))
binds))
,body)))
((for)
(let* ((newenv (new-expansion-env-for e env))
(body (resolve-expansion-vars- (caddr e) newenv m parent-scope inarg))
(expanded-ranges (map (lambda (range)
(resolve-letlike-assign range env newenv m parent-scope inarg)) (for-ranges-list (cadr e)))))
(if (length= expanded-ranges 1)
`(for ,@expanded-ranges ,body))
`(for (block ,@expanded-ranges) ,body)))
((generator)
(let* ((newenv (new-expansion-env-for e env))
(body (resolve-expansion-vars- (cadr e) newenv m parent-scope inarg))
(filt? (eq? (car (caddr e)) 'filter))
(range-exprs (if filt? (cddr (caddr e)) (cddr e)))
(filt (if filt? (resolve-expansion-vars- (cadr (caddr e)) newenv m parent-scope inarg)))
(expanded-ranges (map (lambda (range)
(resolve-letlike-assign range env newenv m parent-scope inarg)) range-exprs)))
(if filt?
`(generator ,body (filter ,filt ,@expanded-ranges))
`(generator ,body ,@expanded-ranges))))
((hygienic-scope) ; TODO: move this lowering to resolve-scopes, instead of reimplementing it here badly
(let ((parent-scope (cons (list env m) parent-scope))
(body (cadr e))
Expand Down
45 changes: 45 additions & 0 deletions test/syntax.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3624,3 +3624,48 @@ end
end
@test (@MyMacroModule.mymacro) == 1
@test (@MyMacroModule.mymacro(a)) == 2

# Issue #53673 - missing macro hygiene for for/generator
baremodule MacroHygieneFor
import ..Base
using Base: esc, Expr, +
macro for1()
:(let a=(for i=10; end; 1); a; end)
end
macro for2()
:(let b=(for j=11, k=12; end; 2); b; end)
end
macro for3()
:(let c=($(Expr(:for, esc(Expr(:block, :(j=11), :(k=12))), :())); 3); c; end)
end
macro for4()
:(begin; local j; let a=(for outer j=10; end; 4); j+a; end; end)
end
end
let nnames = length(names(MacroHygieneFor; all=true))
@test (@MacroHygieneFor.for1) == 1
@test (@MacroHygieneFor.for2) == 2
@test (@MacroHygieneFor.for3) == 3
@test (@MacroHygieneFor.for4) == 14
@test length(names(MacroHygieneFor; all=true)) == nnames
end

baremodule MacroHygieneGenerator
using ..Base: Any, !
my!(x) = !x
macro gen1()
:(let a=Any[x for x in 1]; a; end)
end
macro gen2()
:(let a=Bool[x for x in (true, false) if my!(x)]; a; end)
end
macro gen3()
:(let a=Bool[x for x in (true, false), y in (true, false) if my!(x) && my!(y)]; a; end)
end
end
let nnames = length(names(MacroHygieneGenerator; all=true))
@test (MacroHygieneGenerator.@gen1) == Any[x for x in 1]
@test (MacroHygieneGenerator.@gen2) == Bool[false]
@test (MacroHygieneGenerator.@gen3) == Bool[false]
@test length(names(MacroHygieneGenerator; all=true)) == nnames
end

0 comments on commit 9ae7eab

Please sign in to comment.