Skip to content

Commit

Permalink
Implement proper macro scope resolution for generator/for
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 committed Mar 11, 2024
1 parent 1ba83f0 commit 4c158ce
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 8 deletions.
48 changes: 40 additions & 8 deletions src/macroexpand.scm
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,14 @@
(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 assgn)
(cons 'varlist (cadr assgn)))

;; macro definition
(pattern-lambda (macro (call name . argl) body)
`(-> (tuple ,@argl) ,body))
Expand Down Expand Up @@ -357,6 +365,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 +473,21 @@
`(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)))
(list (car e) body (resolve-letlike-assign (caddr e) env newenv m parent-scope inarg))))
((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
32 changes: 32 additions & 0 deletions test/syntax.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3650,3 +3650,35 @@ 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
end
let nnames = length(names(MacroHygieneFor; all=true))
@test (@MacroHygieneFor.for1) == 1
@test (@MacroHygieneFor.for2) == 2
@test (@MacroHygieneFor.for3) == 3
@test length(names(MacroHygieneFor; all=true)) == nnames
end

baremodule MacroHygieneGenerator
using ..Base: Any
macro gen1()
:(let a=Any[x for x in 1]; a; end)
end
end
let nnames = length(names(MacroHygieneGenerator; all=true))
@test (MacroHygieneGenerator.@gen1) == Any[x for x in 1]
@test length(names(MacroHygieneGenerator; all=true)) == nnames
end

0 comments on commit 4c158ce

Please sign in to comment.