From ff1296738cf1f0a2f2e59dc2a402a87b8aa7ffa8 Mon Sep 17 00:00:00 2001 From: Jeff Bezanson Date: Sun, 2 Jul 2017 00:09:30 -0600 Subject: [PATCH] deprecate `for` loop vars that overwrite outer vars (#22314) provide the old behavior via `for outer i = ...` --- NEWS.md | 5 ++ base/deprecated.jl | 4 +- base/docs/utils.jl | 4 +- base/inference.jl | 20 ++++---- base/intfuncs.jl | 5 +- base/linalg/triangular.jl | 10 ++-- base/markdown/Markdown.jl | 4 +- base/markdown/render/latex.jl | 4 +- base/markdown/render/rst.jl | 4 +- base/multidimensional.jl | 4 +- base/printf.jl | 2 +- base/sparse/linalg.jl | 4 +- doc/src/manual/variables-and-scoping.md | 16 +++--- src/julia-parser.scm | 17 ++++++- src/julia-syntax.scm | 66 ++++++++++++++++++++++--- test/read.jl | 6 ++- test/sparse/sparse.jl | 8 +-- 17 files changed, 128 insertions(+), 55 deletions(-) diff --git a/NEWS.md b/NEWS.md index d0ac72076cf430..ae91a03613029c 100644 --- a/NEWS.md +++ b/NEWS.md @@ -70,6 +70,11 @@ Language changes warning, so that this syntax can be disallowed or given a new meaning in a future version ([#5148]). + * In `for i = ...`, if a local variable `i` already existed it would be overwritten + during the loop. This behavior is deprecated, and in the future `for` loop variables + will always be new variables local to the loop ([#22314]). + The old behavior of overwriting an existing variable is available via `for outer i = ...`. + * In `for i in x`, `x` used to be evaluated in a new scope enclosing the `for` loop. Now it is evaluated in the scope outside the `for` loop. diff --git a/base/deprecated.jl b/base/deprecated.jl index 3428b14900e8ca..65a5cf71729c13 100644 --- a/base/deprecated.jl +++ b/base/deprecated.jl @@ -88,7 +88,7 @@ function firstcaller(bt::Array{Ptr{Void},1}, funcsyms) lkup = StackTraces.UNKNOWN for frame in bt lkups = StackTraces.lookup(frame) - for lkup in lkups + for outer lkup in lkups if lkup == StackTraces.UNKNOWN continue end @@ -1067,7 +1067,7 @@ function partial_linear_indexing_warning_lookup(nidxs_remaining) caller = StackTraces.UNKNOWN for frame in bt lkups = StackTraces.lookup(frame) - for caller in lkups + for outer caller in lkups if caller == StackTraces.UNKNOWN continue end diff --git a/base/docs/utils.jl b/base/docs/utils.jl index 9f7a54f110f5b6..7cf5b4f0324624 100644 --- a/base/docs/utils.jl +++ b/base/docs/utils.jl @@ -289,7 +289,7 @@ function levsort(search, candidates) scores = map(cand -> (levenshtein(search, cand), -fuzzyscore(search, cand)), candidates) candidates = candidates[sortperm(scores)] i = 0 - for i = 1:length(candidates) + for outer i = 1:length(candidates) levenshtein(search, candidates[i]) > 3 && break end return candidates[1:i] @@ -328,7 +328,7 @@ printmatches(args...; cols = displaysize(STDOUT)[2]) = printmatches(STDOUT, args function print_joined_cols(io::IO, ss, delim = "", last = delim; cols = displaysize(io)[2]) i = 0 total = 0 - for i = 1:length(ss) + for outer i = 1:length(ss) total += length(ss[i]) total + max(i-2,0)*length(delim) + (i>1 ? 1 : 0)*length(last) > cols && (i-=1; break) end diff --git a/base/inference.jl b/base/inference.jl index 0bfc01993cc2cd..a9574ad95301c6 100644 --- a/base/inference.jl +++ b/base/inference.jl @@ -3618,9 +3618,9 @@ end function type_annotate!(sv::InferenceState) # remove all unused ssa values gt = sv.src.ssavaluetypes - for i = 1:length(gt) - if gt[i] === NF - gt[i] = Union{} + for j = 1:length(gt) + if gt[j] === NF + gt[j] = Union{} end end @@ -3671,9 +3671,9 @@ function type_annotate!(sv::InferenceState) end # finish marking used-undef variables - for i = 1:nslots - if undefs[i] - src.slotflags[i] |= Slot_UsedUndef + for j = 1:nslots + if undefs[j] + src.slotflags[j] |= Slot_UsedUndef end end nothing @@ -4650,10 +4650,10 @@ function inlineable(@nospecialize(f), @nospecialize(ft), e::Expr, atypes::Vector if !isempty(stmts) && !propagate_inbounds # avoid redundant inbounds annotations s_1, s_end = stmts[1], stmts[end] - i = 2 - while length(stmts) > i && ((isa(s_1,Expr)&&s_1.head===:line) || isa(s_1,LineNumberNode)) - s_1 = stmts[i] - i += 1 + si = 2 + while length(stmts) > si && ((isa(s_1,Expr)&&s_1.head===:line) || isa(s_1,LineNumberNode)) + s_1 = stmts[si] + si += 1 end if isa(s_1, Expr) && s_1.head === :inbounds && s_1.args[1] === false && isa(s_end, Expr) && s_end.head === :inbounds && s_end.args[1] === :pop diff --git a/base/intfuncs.jl b/base/intfuncs.jl index da4fa67d9f22a7..7c33bc407b027b 100644 --- a/base/intfuncs.jl +++ b/base/intfuncs.jl @@ -833,9 +833,8 @@ end function factorial(n::Integer) n < 0 && throw(DomainError(n, "`n` must be nonnegative.")) - local f::typeof(n*n), i::typeof(n*n) - f = 1 - for i = 2:n + f::typeof(n*n) = 1 + for i::typeof(n*n) = 2:n f *= i end return f diff --git a/base/linalg/triangular.jl b/base/linalg/triangular.jl index 12aaa748770f2b..616cb59855714a 100644 --- a/base/linalg/triangular.jl +++ b/base/linalg/triangular.jl @@ -1831,7 +1831,8 @@ function logm(A0::UpperTriangular{T}) where T<:Union{Float64,Complex{Float64}} d4 = norm(AmI^4, 1)^(1/4) alpha3 = max(d3, d4) if alpha3 <= theta[tmax] - for j = 3:tmax + local j + for outer j = 3:tmax if alpha3 <= theta[j] break end @@ -1851,7 +1852,7 @@ function logm(A0::UpperTriangular{T}) where T<:Union{Float64,Complex{Float64}} eta = min(alpha3, alpha4) if eta <= theta[tmax] j = 0 - for j = 6:tmax + for outer j = 6:tmax if eta <= theta[j] m = j break @@ -2030,7 +2031,8 @@ function invsquaring(A0::UpperTriangular, theta) d4 = norm(AmI^4, 1)^(1/4) alpha3 = max(d3, d4) if alpha3 <= theta[tmax] - for j = 3:tmax + local j + for outer j = 3:tmax if alpha3 <= theta[j] break elseif alpha3 / 2 <= theta[5] && p < 2 @@ -2054,7 +2056,7 @@ function invsquaring(A0::UpperTriangular, theta) eta = min(alpha3, alpha4) if eta <= theta[tmax] j = 0 - for j = 6:tmax + for outer j = 6:tmax if eta <= theta[j] m = j break diff --git a/base/markdown/Markdown.jl b/base/markdown/Markdown.jl index 40f8b8955b2731..9f2155fb2c3b50 100644 --- a/base/markdown/Markdown.jl +++ b/base/markdown/Markdown.jl @@ -56,8 +56,8 @@ macro doc_str(s::AbstractString, t...) docexpr(__source__, __module__, s, t...) end -function Base.display(d::Base.REPL.REPLDisplay, md::Vector{MD}) - for md in md +function Base.display(d::Base.REPL.REPLDisplay, mds::Vector{MD}) + for md in mds display(d, md) end end diff --git a/base/markdown/render/latex.jl b/base/markdown/render/latex.jl index fb0650c5d66037..1e885a578d5884 100644 --- a/base/markdown/render/latex.jl +++ b/base/markdown/render/latex.jl @@ -46,8 +46,8 @@ function latexinline(io::IO, code::Code) end function latex(io::IO, md::Paragraph) - for md in md.content - latexinline(io, md) + for mdc in md.content + latexinline(io, mdc) end println(io) println(io) diff --git a/base/markdown/render/rst.jl b/base/markdown/render/rst.jl index d9fadc4e7910ea..377aee73999261 100644 --- a/base/markdown/render/rst.jl +++ b/base/markdown/render/rst.jl @@ -90,8 +90,8 @@ end function rst(io::IO, l::LaTeX) println(io, ".. math::\n") - for l in lines(l.formula) - println(io, " ", l) + for line in lines(l.formula) + println(io, " ", line) end end diff --git a/base/multidimensional.jl b/base/multidimensional.jl index 7680923cdea973..64cf42b8b44c7a 100644 --- a/base/multidimensional.jl +++ b/base/multidimensional.jl @@ -841,9 +841,9 @@ end @noinline function _accumulate!(op, B, A, R1, ind, R2) # Copy the initial element in each 1d vector along dimension `axis` - i = first(ind) + ii = first(ind) @inbounds for J in R2, I in R1 - B[I, i, J] = A[I, i, J] + B[I, ii, J] = A[I, ii, J] end # Accumulate @inbounds for J in R2, i in first(ind)+1:last(ind), I in R1 diff --git a/base/printf.jl b/base/printf.jl index 47ade065971714..230ea2b983de6c 100644 --- a/base/printf.jl +++ b/base/printf.jl @@ -57,7 +57,7 @@ function parse(s::AbstractString) i = 1 while i < length(list) if isa(list[i],AbstractString) - for j = i+1:length(list) + for outer j = i+1:length(list) if !isa(list[j],AbstractString) j -= 1 break diff --git a/base/sparse/linalg.jl b/base/sparse/linalg.jl index 5ae8cdda4129c2..b3b78ecd50f130 100644 --- a/base/sparse/linalg.jl +++ b/base/sparse/linalg.jl @@ -301,8 +301,8 @@ function A_rdiv_B!(A::SparseMatrixCSC{T}, D::Diagonal{T}) where T if iszero(ddj) throw(LinAlg.SingularException(j)) end - for k in nzrange(A, j) - nonz[k] /= ddj + for i in nzrange(A, j) + nonz[i] /= ddj end end A diff --git a/doc/src/manual/variables-and-scoping.md b/doc/src/manual/variables-and-scoping.md index 22b1f2c8e75518..208a3f7d0a2515 100644 --- a/doc/src/manual/variables-and-scoping.md +++ b/doc/src/manual/variables-and-scoping.md @@ -435,7 +435,7 @@ julia> Fs[2]() 2 ``` -`for` loops will reuse existing variables for its iteration variable: +A `for` loop or comprehension iteration variable is always a new variable: ```jldoctest julia> i = 0; @@ -444,18 +444,20 @@ julia> for i = 1:3 end julia> i -3 +0 ``` -However, comprehensions do not do this, and always freshly allocate their iteration variables: +However, it is occasionally useful to reuse an existing variable as the iteration variable. +This can be done conveniently by adding the keyword `outer`: ```jldoctest -julia> x = 0; +julia> i = 0; -julia> [ x for x = 1:3 ]; +julia> for outer i = 1:3 + end -julia> x -0 +julia> i +3 ``` ## Constants diff --git a/src/julia-parser.scm b/src/julia-parser.scm index 9ca2ad558242c0..32281cca87b271 100644 --- a/src/julia-parser.scm +++ b/src/julia-parser.scm @@ -1611,7 +1611,18 @@ ;; as above, but allows both "i=r" and "i in r" (define (parse-iteration-spec s) - (let* ((lhs (parse-pipes s)) + (let* ((outer? (if (eq? (peek-token s) 'outer) + (begin + (take-token s) + (let ((nxt (peek-token s))) + (if (or (memq nxt '(= in ∈)) + (not (symbol? nxt)) + (operator? nxt)) + (begin (ts:put-back! s 'outer #t) + #f) + #t))) + #f)) + (lhs (parse-pipes s)) (t (peek-token s))) (cond ((memq t '(= in ∈)) (take-token s) @@ -1621,7 +1632,9 @@ ;; should be: (error "invalid iteration specification") (syntax-deprecation s (string "for " (deparse `(= ,lhs ,rhs)) " " t) (string "for " (deparse `(= ,lhs ,rhs)) "; " t))) - `(= ,lhs ,rhs))) + (if outer? + `(= (outer ,lhs) ,rhs) + `(= ,lhs ,rhs)))) ((and (eq? lhs ':) (closing-token? t)) ':) (else (error "invalid iteration specification"))))) diff --git a/src/julia-syntax.scm b/src/julia-syntax.scm index 040d4b04c9a2b4..b4f54037be6c90 100644 --- a/src/julia-syntax.scm +++ b/src/julia-syntax.scm @@ -1671,12 +1671,20 @@ (if ,g ,g ,(loop (cdr tail))))))))))) +;; If true, this will warn on all `for` loop variables that overwrite outer variables. +;; If false, this will try to warn only for uses of the last value after the loop. +(define *warn-all-loop-vars* #f) + (define (expand-for while lhs X body) ;; (for (= lhs X) body) - (let ((coll (make-ssavalue)) - (state (gensy))) + (let* ((coll (make-ssavalue)) + (state (gensy)) + (outer? (and (pair? lhs) (eq? (car lhs) 'outer))) + (lhs (if outer? (cadr lhs) lhs))) `(block (= ,coll ,(expand-forms X)) (= ,state (call (top start) ,coll)) + ;; TODO avoid `local declared twice` error from this + ;;,@(if outer? `((local ,lhs)) '()) ,(expand-forms `(,while (call (top !) (call (top done) ,coll ,state)) @@ -1684,6 +1692,9 @@ (block ;; NOTE: enable this to force loop-local var #;,@(map (lambda (v) `(local ,v)) (lhs-vars lhs)) + ,@(if (and (not outer?) (or *depwarn* *deperror*)) + (map (lambda (v) `(warn-if-existing ,v)) (lhs-vars lhs)) + '()) ,(lower-tuple-assignment (list lhs state) `(call (top next) ,coll ,state)) ,body))))))) @@ -2535,7 +2546,7 @@ ((eq? (car e) 'break-block) (unbound-vars (caddr e) bound tab)) ((eq? (car e) 'with-static-parameters) (unbound-vars (cadr e) bound tab)) (else (for-each (lambda (x) (unbound-vars x bound tab)) - (cdr e)) + (cdr e)) tab))) ;; local variable identification and renaming, derived from: @@ -2551,6 +2562,10 @@ ((eq? (car e) 'local) '(null)) ;; remove local decls ((eq? (car e) 'local-def) '(null)) ;; remove local decls ((eq? (car e) 'implicit-global) '(null)) ;; remove implicit-global decls + ((eq? (car e) 'warn-if-existing) + (if (or (memq (cadr e) outerglobals) (memq (cadr e) implicitglobals)) + `(warn-loop-var ,(cadr e)) + '(null))) ((eq? (car e) 'lambda) (let* ((lv (lam:vars e)) (env (append lv env)) @@ -2591,6 +2606,9 @@ vars)))) (need-rename (need-rename? vars)) (need-rename-def (need-rename? vars-def)) + (deprecated-loop-vars + (filter (lambda (v) (memq v env)) + (delete-duplicates (find-decls 'warn-if-existing blok)))) ;; new gensym names for conflicting variables (renamed (map named-gensy need-rename)) (renamed-def (map named-gensy need-rename-def)) @@ -2620,10 +2638,19 @@ (if lam ;; update in-place the list of local variables in lam (set-car! (cddr lam) (append real-new-vars real-new-vars-def (caddr lam)))) - (insert-after-meta ;; return the new, expanded scope-block - (blockify body) - (append! (map (lambda (v) `(local ,v)) real-new-vars) - (map (lambda (v) `(local-def ,v)) real-new-vars-def))))) + (let* ((warnings (map (lambda (v) `(warn-loop-var ,v)) deprecated-loop-vars)) + (body (if *warn-all-loop-vars* + body + (if (and (pair? body) (eq? (car body) 'block)) + (append body warnings) + `(block ,body ,@warnings))))) + (insert-after-meta ;; return the new, expanded scope-block + (blockify body) + (append! (map (lambda (v) `(local ,v)) real-new-vars) + (map (lambda (v) `(local-def ,v)) real-new-vars-def) + (if *warn-all-loop-vars* + (map (lambda (v) `(warn-loop-var ,v)) deprecated-loop-vars) + '())))))) ((eq? (car e) 'module) (error "module expression not at top level")) ((eq? (car e) 'break-block) @@ -3089,7 +3116,7 @@ f(x) = yt(x) ((atom? e) e) (else (case (car e) - ((quote top core globalref outerref line break inert module toplevel null meta) e) + ((quote top core globalref outerref line break inert module toplevel null meta warn-loop-var) e) ((=) (let ((var (cadr e)) (rhs (cl-convert (caddr e) fname lam namemap toplevel interp))) @@ -3335,6 +3362,11 @@ f(x) = yt(x) (else (for-each linearize (cdr e)))) e) +(define (deprecation-message msg) + (if *deperror* + (error msg) + (io.write *stderr* msg))) + ;; this pass behaves like an interpreter on the given code. ;; to perform stateful operations, it calls `emit` to record that something ;; needs to be done. in value position, it returns an expression computing @@ -3347,6 +3379,7 @@ f(x) = yt(x) (first-line #t) (current-loc #f) (rett #f) + (deprecated-loop-vars (table)) (arg-map #f) ;; map arguments to new names if they are assigned (label-counter 0) ;; counter for generating label addresses (label-map (table)) ;; maps label names to generated addresses @@ -3440,6 +3473,11 @@ f(x) = yt(x) (eq? (cadr e) '_)))) (syntax-deprecation #f (string "_ as an rvalue" (linenode-string current-loc)) "")) + (if (and (not *warn-all-loop-vars*) (has? deprecated-loop-vars e)) + (begin (deprecation-message + (string "Use of final value of loop variable \"" e "\"" (linenode-string current-loc) " " + "is deprecated. In the future the variable will be local to the loop instead." #\newline)) + (del! deprecated-loop-vars e))) (cond (tail (emit-return e1)) (value e1) ((or (eq? e1 'true) (eq? e1 'false)) #f) @@ -3477,6 +3515,8 @@ f(x) = yt(x) (lhs (if (and arg-map (symbol? lhs)) (get arg-map lhs lhs) lhs))) + (if (and (not *warn-all-loop-vars*) (has? deprecated-loop-vars lhs)) + (del! deprecated-loop-vars lhs)) (if value (let ((rr (if (or (atom? rhs) (ssavalue? rhs) (eq? (car rhs) 'null)) rhs (make-ssavalue)))) @@ -3670,6 +3710,16 @@ f(x) = yt(x) '(null)) (emit e))) ((isdefined) (if tail (emit-return e) e)) + ((warn-loop-var) + (if (or *warn-all-loop-vars* + (not (or (assq (cadr e) (car (lam:vinfo lam))) + (assq (cadr e) (cadr (lam:vinfo lam)))))) + (deprecation-message + (string "Loop variable \"" (cadr e) "\"" (linenode-string current-loc) " " + "overwrites a variable in an enclosing scope. " + "In the future the variable will be local to the loop instead." #\newline)) + (put! deprecated-loop-vars (cadr e) #t)) + '(null)) ;; top level expressions returning values ((abstract_type primitive_type struct_type thunk toplevel module) diff --git a/test/read.jl b/test/read.jl index c68076d9010f9f..0269746a61aeaf 100644 --- a/test/read.jl +++ b/test/read.jl @@ -138,6 +138,7 @@ verbose = false for (name, f) in l + local f io = ()->(s=f(text); push!(open_streams, s); s) write(filename, text) @@ -176,13 +177,14 @@ for (name, f) in l old_text = text cleanup() - for text in [ + for text_ in [ old_text, String(Char['A' + i % 52 for i in 1:(div(Base.SZ_UNBUFFERED_IO,2))]), String(Char['A' + i % 52 for i in 1:( Base.SZ_UNBUFFERED_IO -1)]), String(Char['A' + i % 52 for i in 1:( Base.SZ_UNBUFFERED_IO )]), String(Char['A' + i % 52 for i in 1:( Base.SZ_UNBUFFERED_IO +1)]) ] + text = text_ write(filename, text) verbose && println("$name read(io, String)...") @@ -311,7 +313,7 @@ function test_read_nbyte() fn = tempname() # Write one byte. One byte read should work once # but 2-byte read should throw EOFError. - f = open(fn, "w+") do f + open(fn, "w+") do f write(f, 0x55) flush(f) seek(f, 0) diff --git a/test/sparse/sparse.jl b/test/sparse/sparse.jl index 30e62f354afabf..e813328c835eeb 100644 --- a/test/sparse/sparse.jl +++ b/test/sparse/sparse.jl @@ -1095,14 +1095,14 @@ end N=2^3 Irand = randperm(M) Jrand = randperm(N) - I = sort([Irand; Irand; Irand]) + II = sort([Irand; Irand; Irand]) J = [Jrand; Jrand] SA = [sprand(M, N, d) for d in [1., 0.1, 0.01, 0.001, 0.0001, 0.]] for S in SA res = Any[1,2,3] for searchtype in [0, 1, 2] - res[searchtype+1] = test_getindex_algs(S, I, J, searchtype) + res[searchtype+1] = test_getindex_algs(S, II, J, searchtype) end @test res[1] == res[2] == res[3] @@ -1110,12 +1110,12 @@ end M = 2^14 N=2^4 - I = randperm(M) + II = randperm(M) J = randperm(N) Jsorted = sort(J) SA = [sprand(M, N, d) for d in [1., 0.1, 0.01, 0.001, 0.0001, 0.]] - IA = [I[1:round(Int,n)] for n in [M, M*0.1, M*0.01, M*0.001, M*0.0001, 0.]] + IA = [II[1:round(Int,n)] for n in [M, M*0.1, M*0.01, M*0.001, M*0.0001, 0.]] debug = false if debug @printf(" | | | times | memory |\n")