Skip to content

Commit

Permalink
Fix invalid let syntax from LHS views (#53108)
Browse files Browse the repository at this point in the history
When the frontend lowers an expression like `lhs .+= rhs`, it needs to
prevent evaluating the LHS more than once before re-writing to `lhs .=
lhs .+ rhs`. If the LHS was a `let` block — commonly generated by
`@views` and (since #53064) `@view` — the lowering pass had previously
been emitting an invalid `let` temporary. This very directly addresses
that case.

Fixes #53107.
Fixes #44356.
  • Loading branch information
mbauman authored Jan 30, 2024
1 parent fdb342c commit 9f643a0
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 2 deletions.
6 changes: 4 additions & 2 deletions src/julia-syntax.scm
Original file line number Diff line number Diff line change
Expand Up @@ -1653,8 +1653,10 @@
(let ((g (make-ssavalue)))
(begin (set! a (cons `(= ,g ,x) a))
g)))))
(cons (cons (car e) (map arg-to-temp (cdr e)))
(reverse a)))))
(if (eq? (car e) 'let)
(cons (arg-to-temp e) (reverse a))
(cons (cons (car e) (map arg-to-temp (cdr e)))
(reverse a))))))

(define (lower-kw-call f args)
(let* ((para (if (has-parameters? args) (cdar args) '()))
Expand Down
44 changes: 44 additions & 0 deletions test/subarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -637,6 +637,44 @@ end
@test foo == [X, X]
end

# Test as an assignment's left hand side
let x = [1,2,3,4]
@test Meta.@lower(@view(x[1]) = 1).head == :error
@test Meta.@lower(@view(x[1]) += 1).head == :error
@test Meta.@lower(@view(x[end]) = 1).head == :error
@test Meta.@lower(@view(x[end]) += 1).head == :error
@test Meta.@lower(@view(f(x)[end]) = 1).head == :error
@test Meta.@lower(@view(f(x)[end]) += 1).head == :error
@test (@view(x[1]) .+= 1) == fill(2)
@test x == [2,2,3,4]
@test (@view(reshape(x,2,2)[1,1]) .+= 10) == fill(12)
@test x == [12,2,3,4]
@test (@view(x[end]) .+= 1) == fill(5)
@test x == [12,2,3,5]
@test (@view(reshape(x,2,2)[end]) .+= 10) == fill(15)
@test x == [12,2,3,15]
@test (@view(reshape(x,2,2)[[begin],[begin,end]])::AbstractMatrix{Int} .+= [2]) == [14 5]
@test x == [14,2,5,15]

x = [1,2,3,4]
@test Meta.@lower(@views(x[[1]]) = 1).head == :error
@test Meta.@lower(@views(x[[1]]) += 1).head == :error
@test Meta.@lower(@views(x[[end]]) = 1).head == :error
@test Meta.@lower(@views(x[[end]]) += 1).head == :error
@test Meta.@lower(@views(f(x)[end]) = 1).head == :error
@test Meta.@lower(@views(f(x)[end]) += 1).head == :error
@test (@views(x[[1]]) .+= 1) == [2]
@test x == [2,2,3,4]
@test (@views(reshape(x,2,2)[[1],1]) .+= 10) == [12]
@test x == [12,2,3,4]
@test (@views(x[[end]]) .+= 1) == [5]
@test x == [12,2,3,5]
@test (@views(reshape(x,2,2)[[end]]) .+= 10) == [15]
@test x == [12,2,3,15]
@test (@views(reshape(x,2,2)[[begin],[begin,end]])::AbstractMatrix{Int} .+= [2]) == [14 5]
@test x == [14,2,5,15]
end

# test @views macro
@views let f!(x) = x[begin:end-1] .+= x[begin+1:end].^2
x = [1,2,3,4]
Expand All @@ -663,6 +701,12 @@ end
@test x == [5,8,12,9] && i == [4,3]
@. x[3:end] = 0 # make sure @. works with end expressions in @views
@test x == [5,8,0,0]
x[begin:end] .+= 1
@test x == [6,9,1,1]
x[[begin,2,end]] .-= [1,2,3]
@test x == [5,7,1,-2]
@. x[[begin,2,end]] .+= [1,2,3]
@test x == [6,9,1,1]
end
@views @test isa(X[1:3], SubArray)
@test X[begin:end] == @views X[begin:end]
Expand Down
18 changes: 18 additions & 0 deletions test/syntax.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3619,3 +3619,21 @@ end
@test p("public[7] = 5") == Expr(:(=), Expr(:ref, :public, 7), 5)
@test p("public() = 6") == Expr(:(=), Expr(:call, :public), Expr(:block, 6))
end

@testset "removing argument sideeffects" begin
# Allow let blocks in broadcasted LHSes, but only evaluate them once:
execs = 0
array = [1]
let x = array; execs += 1; x; end .+= 2
@test array == [3]
@test execs == 1
let; execs += 1; array; end .= 4
@test array == [4]
@test execs == 2
let x = array; execs += 1; x; end::Vector{Int} .+= 2
@test array == [6]
@test execs == 3
let; execs += 1; array; end::Vector{Int} .= 7
@test array == [7]
@test execs == 4
end

0 comments on commit 9f643a0

Please sign in to comment.