From 4ebec0db720845ff8e95b4eadb336c0a7e554d7a 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) --- base/deprecated.jl | 6 ++-- base/docs/utils.jl | 6 ++-- base/inference.jl | 20 ++++++------ base/intfuncs.jl | 5 ++- base/linalg/triangular.jl | 14 ++++++--- base/markdown/Markdown.jl | 4 +-- base/markdown/render/latex.jl | 4 +-- base/markdown/render/rst.jl | 4 +-- base/multidimensional.jl | 4 +-- base/printf.jl | 3 +- base/sparse/linalg.jl | 4 +-- src/julia-syntax.scm | 57 ++++++++++++++++++++++++++++++----- test/read.jl | 3 +- test/sparse/sparse.jl | 8 ++--- 14 files changed, 97 insertions(+), 45 deletions(-) diff --git a/base/deprecated.jl b/base/deprecated.jl index 828b3ce5c7538..d65c000b48ac5 100644 --- a/base/deprecated.jl +++ b/base/deprecated.jl @@ -90,7 +90,8 @@ function firstcaller(bt::Array{Ptr{Void},1}, funcsyms) lkup = StackTraces.UNKNOWN for frame in bt lkups = StackTraces.lookup(frame) - for lkup in lkups + for l in lkups + lkup = l if lkup == StackTraces.UNKNOWN continue end @@ -1067,7 +1068,8 @@ function partial_linear_indexing_warning_lookup(nidxs_remaining) caller = StackTraces.UNKNOWN for frame in bt lkups = StackTraces.lookup(frame) - for caller in lkups + for c_ in lkups + caller = c_ if caller == StackTraces.UNKNOWN continue end diff --git a/base/docs/utils.jl b/base/docs/utils.jl index a4e4be3bc5603..30177e77c411a 100644 --- a/base/docs/utils.jl +++ b/base/docs/utils.jl @@ -289,7 +289,8 @@ 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 i_ = 1:length(candidates) + i = i_ levenshtein(search, candidates[i]) > 3 && break end return candidates[1:i] @@ -328,7 +329,8 @@ 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 i_ = 1:length(ss) + i = i_ 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 c2fcdd04f04b6..ffe888e6f54af 100644 --- a/base/inference.jl +++ b/base/inference.jl @@ -3515,9 +3515,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 @@ -3568,9 +3568,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 @@ -4572,10 +4572,10 @@ function inlineable(f::ANY, ft::ANY, e::Expr, atypes::Vector{Any}, sv::Inference 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 82f5db4b2fa45..bc68b877bba04 100644 --- a/base/intfuncs.jl +++ b/base/intfuncs.jl @@ -801,9 +801,8 @@ end function factorial(n::Integer) n < 0 && throw(DomainError()) - 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 27a671abacf4c..2780dfad7b02e 100644 --- a/base/linalg/triangular.jl +++ b/base/linalg/triangular.jl @@ -1833,7 +1833,9 @@ 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 j_ = 3:tmax + j = j_ if alpha3 <= theta[j] break end @@ -1853,7 +1855,8 @@ 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 j_ = 6:tmax + j = j_ if eta <= theta[j] m = j break @@ -2032,7 +2035,9 @@ 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 j_ = 3:tmax + j = j_ if alpha3 <= theta[j] break elseif alpha3 / 2 <= theta[5] && p < 2 @@ -2056,7 +2061,8 @@ function invsquaring(A0::UpperTriangular, theta) eta = min(alpha3, alpha4) if eta <= theta[tmax] j = 0 - for j = 6:tmax + for j_ = 6:tmax + j = j_ if eta <= theta[j] m = j break diff --git a/base/markdown/Markdown.jl b/base/markdown/Markdown.jl index 93fb7a045f04c..f47e9c9fbc001 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 fb0650c5d6603..1e885a578d588 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 d9fadc4e7910e..377aee7399926 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 ffb5f61923db1..0c181a6cbd53a 100644 --- a/base/multidimensional.jl +++ b/base/multidimensional.jl @@ -812,9 +812,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 c2dea8210f4a5..398e066adbc79 100644 --- a/base/printf.jl +++ b/base/printf.jl @@ -57,7 +57,8 @@ function parse(s::AbstractString) i = 1 while i < length(list) if isa(list[i],AbstractString) - for j = i+1:length(list) + for j_ = i+1:length(list) + j = j_ if !isa(list[j],AbstractString) j -= 1 break diff --git a/base/sparse/linalg.jl b/base/sparse/linalg.jl index 2ef39f7ca6939..74c00079897c8 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/src/julia-syntax.scm b/src/julia-syntax.scm index 03821caa2abba..41694242c6c2a 100644 --- a/src/julia-syntax.scm +++ b/src/julia-syntax.scm @@ -1647,6 +1647,10 @@ (cons (car e) (map expand-forms (cdr e))))))) +;; 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)) @@ -1661,6 +1665,9 @@ (block ;; NOTE: enable this to force loop-local var #;,@(map (lambda (v) `(local ,v)) (lhs-vars lhs)) + ,@(if (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)))))))) @@ -2491,7 +2498,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: @@ -2507,6 +2514,7 @@ ((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) '(null)) ((eq? (car e) 'lambda) (let* ((lv (lam:vars e)) (env (append lv env)) @@ -2547,6 +2555,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)) @@ -2576,12 +2587,21 @@ (if lam ;; update in-place the list of local variables in lam (set-car! (cddr lam) (append! (caddr lam) real-new-vars real-new-vars-def))) - (insert-after-meta ;; return the new, expanded scope-block - (if (and (pair? body) (eq? (car body) 'block)) - body - `(block ,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 + (if (and (pair? body) (eq? (car body) 'block)) + body + `(block ,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) @@ -3035,7 +3055,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))) @@ -3282,6 +3302,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 @@ -3294,6 +3319,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 @@ -3387,6 +3413,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) @@ -3416,6 +3447,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)))) @@ -3602,6 +3635,14 @@ f(x) = yt(x) ((implicit-global) #f) ((const) (emit e)) ((isdefined) (if tail (emit-return e) e)) + ((warn-loop-var) + (if *warn-all-loop-vars* + (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 bits_type composite_type thunk toplevel module) diff --git a/test/read.jl b/test/read.jl index 6f794a84f59bd..4aa920fdf4f12 100644 --- a/test/read.jl +++ b/test/read.jl @@ -176,13 +176,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 readstring...") diff --git a/test/sparse/sparse.jl b/test/sparse/sparse.jl index a5e128b3da98e..362d5a6bbe72c 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")