remove unnecessary true && part from at-view macro

[aviatesk]

The modification that expands @view A[i] to true && view(A, i) appears to go back as far as #20247. However, I'm not entirely sure why this is necessary. Considering that just expanding it to view(A, i) still seems to pass the base test suite, I wonder if it might be just better to get rid of that part.

[giordano]

true && view(A, i) appears to go back as far as #20247.

That was already present in #16564 by @simonbyrne, and the motivation was given in #16564 (comment)

I've also changed it so that it expands to true && view(X, inds...), to prevent people redefining view by doing @view(X[inds...]) = ...

julia> @view(A[1:2]) = 2
ERROR: syntax: invalid assignment location "true && Base.view(A, 1:2)" around REPL[2]:1

If anything, this needs a test, and maybe an explicit comment in the code, to avoid further confusion.

[stevengj]

I traced back through the git blame, and true && ... ultimately arises from #16564 (comment), which applied to @view but maybe is not necessary for @views? (In contrast, if you look carefully at #20247, you'll see that the previous version had similar Expr(:&&, true, esc(...)) code but in a different place.)

Update: Oh, I see that @giordano also pointed out the same thing.

But it seems that you are making a bunch of other changes as well?

[aviatesk]

Thank you. Certainly there is a difference from the previous code in cases like @view(A[idx]) = x. Currently, errors arise unless Base.view is imported by the user, so I think the present method may be acceptable? Or, perhaps a more careful approach would be to raise an explicit error when the expression provided by the user is is_function_def(rhs).

[giordano]

Using @view(A[idx]) = x looks a very convoluted and unintuitive way to define a method, I'd be more comfortable with keeping it a syntax error and have a test for that (I couldn't see any for it, but maybe I missed it).

[aviatesk]

FWIW, it results in a syntax error even on this PR (unless we import Base.view explicitly):

julia> let A = [1,2], idx = (1,2)
           @view(A[idx]) = 2
           view(A, 1:2)
       end
ERROR: syntax: invalid function name "Base.view" around REPL[18]:2
Stacktrace:
 [1] top-level scope
   @ REPL[18]:1

[giordano]

(unless we import Base.view explicitly)

I really don't think anyone would expect @view(A[idx]) = 2 to be a method definition, regardless of whether Base.view is imported or not.

[mbauman]

(unless we import Base.view explicitly)

I don't think that changes anything (anymore?). The macro (now?) plops the function itself into the AST. The error message talks about a "function name" and uses the stringified name, but it's not getting a name — it's getting a function object. It's the same as:

julia> function f end
f (generic function with 0 methods)

julia> @eval ($f)(x) = x
ERROR: syntax: invalid function name "Main.f"

Edit: Ah, yes, the original definition from #16564 generated an AST like Expr(:call, :(Base.view),...), but we now do Expr(:call, Base.view, ...). The true && is necessary in the former, but not the latter.

[aviatesk]

Thanks for your input, Bauman. Embedding the function object directly allows for triggering a syntax error, independent of imports. I've kept comments and tests as a future reference. I think now this PR is ready to go.