Fix at-views for setindex expression

[yuyichao]

Fix #59383

[yuyichao]

Note that if this is to be backported some additional fix may be needed. #58905 removed an use of the replace_ref_begin_end_! function outside of views.jl and that would need to be fixed (by appending two false arguments). This depends on #59239

[vtjnash]

I don't believe this is quite right. I think the intended expansion of x[begin] = y is let x = x; x[length(x)] = y ==> e.g. that the let should move outside of the = so that the lowering expansion remains valid, but still does take the view

[yuyichao]

Updated.

Should also work for other flavors of assignments now .=, +=, .+= etc.

I kept the way in_quote_context is passed around as a variable even when it's value is known.

[yuyichao]

that the let should move outside of the = so that the lowering expansion remains valid, but still does take the view

I'm pretty sure the normal setindex! case should not use the view. And I don't think there is necessarily anything that needs to be done for expressions that would be lowered to just setindex!. This is the case even when no micro hygiene was involved on previous versions,

julia> @macroexpand @views x[a:b] = c
:(x[a:b] = c)

It seems that @views is only intended for anything that would be a getindex but not setindex! and it differentiates the two cases clearly when the expression lowers to both,

julia> @macroexpand @views x[a:b] += c
:(let var"#2#a" = x, var"#1#i1" = a:b
      var"#2#a"[var"#1#i1"] = (Base.maybeview)(var"#2#a", var"#1#i1") + c
  end)

julia> @macroexpand @views x[a:b] .+= c
:(let var"#4#a" = x, var"#3#i1" = a:b
      var"#4#a"[var"#3#i1"] .= (Base.maybeview)(var"#4#a", var"#3#i1") .+ c
  end)

That said, I wasn't perfectly happy with simply ignoring these cases as in the previous implementation because it's inconsistent with the function name. Also, the previous one didn't handle other flavors of assignments correctly and handling those does require the lowering style that you suggested.

[vtjnash]

FWIW, this error has also always been incorrect, if the intent of this function is to predict lowering. Lowering does not consider this to be invalid, and simply skips the replacement when withex === nothing:

julia> var"begin" = var"end" = 3
3

[vtjnash]

Seems an odd way to deal with a binary operator being to loop from 1 to 2 skipping 1

Suggested change

	for i = eachindex(ex.args)
	if i == 1
	# we'll deal with the ref expression later
	continue
	end
	let i = 2

[yuyichao]

I was going to handle only args[2] here. However, the code below treated it as if there could be more than 2 argument for this expression

julia/base/views.jl

Line 272 in cbea8cf

mapany(e -> esc(_views(e)), ex.args[2:end])...))), Base)

when it handles the generic assignment cases.

[vtjnash]

Suggested change

	elseif !in_quote_context && last(string(ex.head)) == '=' && Meta.isexpr(ex.args[1], :ref)
	elseif !in_quote_context && last(string(ex.head)) == '=' && Meta.isexpr(Meta.unescape(ex.args[1]), :ref)

[vtjnash]

Suggested change

	ex.args[i], used = replace_ref_begin_end_!(__module__, ex.args[i], withex, in_quote_context, escs)
	rhs, used = reescape(replace_ref_begin_end_!(__module__, unescape(ex.args[i]), withex, in_quote_context, escs)
	ex.args[i] = reescape(rhs, ex.args[i])

[yuyichao]

I assume the first reescape should not be there.

[yuyichao]

Also, why should the escape/unescape be done on this argument? This should just be a generic handling of the an expression just like the first argument of the :ref expression and exactly the same as the fallback case below. I assume replace_ref_begin_end_! is handling the escaping part correctly, else many more places would need to be fixed.

I could see though, that the first argument (the LHS) needs to be handled manually since we need to see through escape to figure out if the LHS of an assignment operator is an Expr(:ref). The implementation of @views below doesn't seem to handle this either though, and should probably be fixed at the same time,

julia> macro views_wrapper(lhs, rhs)
           quote
               @views $(esc(lhs)) += $(esc(rhs))
           end
       end
@views_wrapper (macro with 1 method)

julia> @macroexpand @views_wrapper x[a:b] 1
quote
    #= REPL[3]:3 =#
    (Base.maybeview)(x, a:b) += 1
end

[yuyichao]

And another issue seems to be that this function also straight up ignore escape expressions in general (unless they appears in hygienic-scope expressions). Shouldn't that logic only apply if the "begin"/"end" is escaped compared to the ref expression?

Basically, I feel like esc(:(x[begin:end])) should be processed whereas :(x[$(esc(:(var"begin":var"end")))]) shouldn't.

[vtjnash]

Yes, lots of code handles esc wrong. Including lowering, which considers begin/end to ignore scope (as we currently emulate here)

[yuyichao]

Right now x[esc(begin:end)] is being lowered to x[begin:end]. Would this be considered a bug in the long term or is it the job of this function to lift the begin/end out of the argument of ref?