Skip to content

Commit

Permalink
fix #17240, evaluate keyword argument defaults in successive scopes
Browse files Browse the repository at this point in the history 
This matches the behavior of optional positional arguments.
  • Loading branch information
@JeffBezanson
JeffBezanson committed Jul 25, 2017
1 parent 9186ae4 commit feaa6bf
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 59 deletions.
4 changes: 4 additions & 0 deletions NEWS.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ Language changes
* Declaring arguments as `x::ANY` to avoid specialization has been replaced
by `@nospecialize x`. ([#22666]).

* Keyword argument default values are now evaluated in successive scopes ---
the scope for each expression includes only previous keyword arguments, in
left-to-right order ([#17240]).

* The parsing of `1<<2*3` as `1<<(2*3)` is deprecated, and will change to
`(1<<2)*3` in a future version ([#13079]).

Expand Down
11 changes: 3 additions & 8 deletions doc/src/manual/functions.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -472,9 +472,8 @@ this example, `width` is certain to have the value `2`.

## Evaluation Scope of Default Values

Optional and keyword arguments differ slightly in how their default values are evaluated. When
optional argument default expressions are evaluated, only *previous* arguments are in scope. In
contrast, *all* the arguments are in scope when keyword arguments default expressions are evaluated.
When optional and keyword argument default expressions are evaluated, only *previous* arguments are in
scope.
For example, given this definition:

```julia
Expand All @@ -483,11 +482,7 @@ function f(x, a=b, b=1)
end
```

the `b` in `a=b` refers to a `b` in an outer scope, not the subsequent argument `b`. However,
if `a` and `b` were keyword arguments instead, then both would be created in the same scope and
the `b` in `a=b` would refer to the subsequent argument `b` (shadowing any `b` in an outer scope),
which would result in an undefined variable error (since the default expressions are evaluated
left-to-right, and `b` has not been assigned yet).
the `b` in `a=b` refers to a `b` in an outer scope, not the subsequent argument `b`.

## Do-Block Syntax for Function Arguments

Expand Down
90 changes: 41 additions & 49 deletions src/julia-syntax.scm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -367,12 +367,13 @@
`(block (method ,name) ,mdef (unnecessary ,name)) ;; return the function
mdef)))))

;; keyword default values that can be assigned right away. however, this creates
;; a quasi-bug (part of issue #9535) where it can be hard to predict when a
;; keyword argument will throw an UndefVarError.
(define (const-default? x)
(or (number? x) (string? x) (char? x) (and (pair? x) (memq (car x) '(quote inert)))
(eq? x 'true) (eq? x 'false)))
;; wrap expr in nested scopes assigning names to vals
(define (scopenest names vals expr)
(if (null? names)
expr
`(let (block
,(scopenest (cdr names) (cdr vals) expr))
(= ,(car names) ,(car vals)))))

(define empty-vector-any '(call (core AnyVector) 0))

Expand Down Expand Up @@ -434,24 +435,23 @@
(rkw (if (null? restkw) '() (symbol (string (car restkw) "..."))))
(mangled (symbol (string "#" (if name (undot-name name) 'call) "#"
(string (current-julia-module-counter)))))
(flags (map (lambda (x) (gensy)) vals)))
(tempnames (map (lambda (x) (gensy)) keynames)))
`(block
;; call with no keyword args
,(method-def-expr-
name positional-sparams (append pargl vararg)
`(block
,@prologue
,@(if (not ordered-defaults)
'()
(append! (map (lambda (kwname) `(local ,kwname)) keynames)
(map make-assignment keynames vals)))
;; call mangled(vals..., [rest_kw,] pargs..., [vararg]...)
(return (call ,mangled
,@(if ordered-defaults keynames vals)
,@(if (null? restkw) '() (list empty-vector-any))
,@(map arg-name pargl)
,@(if (null? vararg) '()
(list `(... ,(arg-name (car vararg))))))))
,(let (;; call mangled(vals..., [rest_kw,] pargs..., [vararg]...)
(ret `(return (call ,mangled
,@(if ordered-defaults keynames vals)
,@(if (null? restkw) '() (list empty-vector-any))
,@(map arg-name pargl)
,@(if (null? vararg) '()
(list `(... ,(arg-name (car vararg)))))))))
(if ordered-defaults
(scopenest keynames vals ret)
ret)))
#f)

;; call with keyword args pre-sorted - original method code goes here
Expand Down Expand Up @@ -483,14 +483,9 @@
,(if (any kwarg? pargl) (gensy) UNUSED)
(call (core kwftype) ,ftype)) (:: ,kw (core AnyVector)) ,@pargl ,@vararg)
`(block
;; initialize keyword args to their defaults, or set a flag telling
;; whether this keyword needs to be set.
,@(map (lambda (kwname) `(local ,kwname)) keynames)
,@(map (lambda (name dflt flag)
(if (const-default? dflt)
`(= ,name ,dflt)
`(= ,flag true)))
keynames vals flags)
;; temp variables that will be assigned if their corresponding keywords are passed.
;; `isdefined` is then used to check whether default values should be evaluated.
,@(map (lambda (v) `(local ,v)) tempnames)
,@(if (null? restkw) '()
`((= ,rkw ,empty-vector-any)))
;; for i = 1:(length(kw)>>1)
Expand All @@ -499,8 +494,8 @@
;; ii = i*2 - 1
(= ,ii (call (top -) (call (top *) ,i 2) 1))
(= ,elt (call (core arrayref) ,kw ,ii))
,(foldl (lambda (kvf else)
(let* ((k (car kvf))
,(foldl (lambda (kn else)
(let* ((k (car kn))
(rval0 `(call (core arrayref) ,kw
(call (top +) ,ii 1)))
;; note: if the "declared" type of a KW arg
Expand Down Expand Up @@ -528,13 +523,9 @@
,T)
T)))
rval0)))
;; if kw[ii] == 'k; k = kw[ii+1]::Type; end
`(if (comparison ,elt === (quote ,(decl-var k)))
(block
(= ,(decl-var k) ,rval)
,@(if (not (const-default? (cadr kvf)))
`((= ,(caddr kvf) false))
'()))
;; if kw[ii] == 'k; k_temp = kw[ii+1]::Type; end
`(if (comparison ,elt === (quote ,(cdr kn)))
(= ,(decl-var k) ,rval)
,else)))
(if (null? restkw)
;; if no rest kw, give error for unrecognized
Expand All @@ -547,21 +538,22 @@
,rkw (tuple ,elt
(call (core arrayref) ,kw
(call (top +) ,ii 1)))))
(map list vars vals flags))))
(map (lambda (k temp)
(cons (if (decl? k) `(,(car k) ,temp ,(caddr k)) temp)
(decl-var k)))
vars tempnames))))
;; set keywords that weren't present to their default values
,@(apply append
(map (lambda (name dflt flag)
(if (const-default? dflt)
'()
`((if ,flag (= ,name ,dflt)))))
keynames vals flags))
;; finally, call the core function
(return (call ,mangled
,@keynames
,@(if (null? restkw) '() (list rkw))
,@(map arg-name pargl)
,@(if (null? vararg) '()
(list `(... ,(arg-name (car vararg))))))))
,(scopenest keynames
(map (lambda (v dflt) `(if (isdefined ,v)
,v
,dflt))
tempnames vals)
`(return (call ,mangled ;; finally, call the core function
,@keynames
,@(if (null? restkw) '() (list rkw))
,@(map arg-name pargl)
,@(if (null? vararg) '()
(list `(... ,(arg-name (car vararg)))))))))
#f)
;; return primary function
,(if (not (symbol? name))
Expand Down
13 changes: 11 additions & 2 deletions test/keywordargs.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,10 +219,10 @@ f9948, getx9948 = let
getx() = x
h, getx
end
@test_throws UndefVarError f9948()
@test f9948() == 3
@test getx9948() == 3
@test f9948(x=5) == 5
@test_throws UndefVarError f9948()
@test f9948() == 3
@test getx9948() == 3

# issue #17785 - handle all sources of kwargs left-to-right
Expand Down Expand Up @@ -285,3 +285,12 @@ let a = 0
g21518()(; :kw=>1)
@test a == 2
end

# issue #17240 - evaluate default expressions in nested scopes
let a = 10
f17240(;a=a-1, b=2a) = (a,b)
@test f17240() == (9, 18)
@test f17240(a=2) == (2, 4)
@test f17240(b=3) == (9, 3)
@test f17240(a=2, b=1) == (2, 1)
end

0 comments on commit feaa6bf

Please sign in to comment.