RFC: lower a' as var"'"(a)

[simeonschaub]

As proposed by @JeffBezanson in #33683, this changes the lowering of a' to __adjoint__(a) var"'"(a), where var"'" can be shadowed by the user. This should be less breaking than that PR, so could someone trigger PkgEval?

[maleadt]

@nanosoldier runtests(ALL, vs = ":master")

[nanosoldier]

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

[simeonschaub]

Doesn't seem like any of the failures were caused by this PR, so that's definitely a good sign!

[JeffBezanson]

The implementation is correct. I think the main thing to do is decide whether we like this approach, and if so apply it consistently to this kind of syntax, and address it in the manual.

[KristofferC]

This would mean https://github.com/simonster/TwoBasedIndexing.jl could make a comeback? Let's go!

[yuyichao]

What's the point of letting a local name override a syntax? Overriding a global name in a local scope is certainly fine but this one seems to be all about syntax. If it was not about syntax then the user should have just picked a different function name instead, i.e. the user should just use my_adjoint(a) rather than a'. However, syntax customization was exactly what macro is for. A huge advantage of macros is that it is easy to spot where it is applied so it is very easy to realize when customized syntax rule applies. This change just completely changes things and seems to allow a global assignment to suddenly change the meaning of a syntax with absolutely no visual clue about it. Say a @innocent_macro that is expanded to :($(esc(__adjoint__)) = my_adjoint) or a global assignment in a different file.

It seems that the syntax customization can already be done by having a macro enclosing the part with customized syntax and the existing approach seems to be way clearer.

[JeffBezanson]

Yes, but I can already write + = print...

[yuyichao]

Right, that's fine and that's why I said.

Overriding a global name in a local scope is certainly fine but this one seems to be all about syntax.

+ isn't a syntax.

[yuyichao]

Also, the bigger difference is that for builtin functions, they aren't much better names than user defined ones so while you can do push! = my_push! or + = my_add, there are alternative names and operators that can be used so we are in no way encouraging people to actually override them. I wouldn't say the fact that a few very fundamental and commonly used names being overridable is a plus either, but since they aren't being overriden in real life frequent enough and there isn't enough reason for people to do it, it doesn't matter very much and are basically examples of "don't shot yourself in the foot". This is also how scope works right now so unless the rule of scope changes it doesn't make much sense to special case a few commonly used functions.

However, the whole point of this change seems to be that people actually want to override this where the syntax was originally not overridable without using a macro. If no one actually want to use this to make a implicit syntax change then I'm certainly fine with that but in that case the lowering might as well not be changed.

[JeffBezanson]

Making a distinction between ' being syntax and ! and + not feels a little arbitrary to me. Should we make x' call var"'" instead?

[simeonschaub]

Are you talking about applying this to every call, that currently gets lowered to a Base function, or would we draw a line somewhere? Out of the top of my head, I can think of getindex, setindex!, getproperty, setproperty!, the whole broadcasting machinery and cat, hcat and vcat, but I'm sure there are others.

[yuyichao]

Making a distinction between ' being syntax and ! and + not feels a little arbitrary to me. Should we make x' call var"'" instead?

If the argument is consistency, I'm certainly fine with lowering a' to a call of var"'"(a). The fact that this allow someone to override things and write confusing code is unfortunate but that's OK. Edit: though this argument works better if there are more generic postfix operators. The fact that ' is already special in this regard makes the distinction not as arbitrary anymore...

This however shouldn't be applied to other syntaxes, especially getindex and setindex!. Edit: and basically all the other syntaxes mentioned above.

[simeonschaub]

Thinking about it some more, I agree that lowering to var"'" makes more sense than __adjoint__, because if we decide that we want to support unicode modifiers for ', as described in #34507, it would be inconsistent, if one was lowered as __adjoint__ and the other as var"'ᵀ".

[simeonschaub]

There seems to be consensus, that it makes more sense to lower to var"'", so I made that change.

[simeonschaub]

Would it make sense to merge the docstrings for ' and adjoint into one?

[c42f]

I agree that declaring a' more syntaxy than a + b seems pretty arbitrary so I think we should make this change to lowering for a little extra consistency. Unfortunately it doesn't actually bring the main benefits in consistency without parsing a' as Expr(:call, Symbol("'"), a) but I guess that's a julia-2.0 thing.

I reviewed the translation table in lowering. I was somewhat surprised, but adjoint seems to be the only "clearly function-like" syntax we should apply this treatment to.

[c42f]

Would it make sense to merge the docstrings for ' and adjoint into one?

Yes I think so. See the docstring for => as an analogy. [edit: also ≈]

[simeonschaub]

I merged the two docstrings and added another example. I also thought, I'd add a sentence, that explains how adjoint behaves for number types, because that was missing before.

[simeonschaub]

I think this should be mostly done now. Or are there still issues that need to be addressed?

[c42f]

Personally I think this is a small improvement of consistency and it's worth doing.

[c42f]

I wasn't on the triage call though so I asked about it last week in #triage on slack. Jeff said the following:

We decided that it's ok, but the main motivation seems to be adding unicode suffixes to ', which is a bit breaking.

So I'm not sure whether triage was in favor of just merging this, only merging if unicode suffixes go ahead, or just ambivalent.

[simeonschaub]

Any news on this?

[MasonProtter]

Regarding

We decided that it's ok, but the main motivation seems to be adding unicode suffixes to ', which is a bit breaking.

that can come later if we worry about it being breaking. As it stands, I'd argue that shadowing is good enough, i.e. allowing

julia> let var"'" = transpose, v = rand(ComplexF64, 5)
         v'v
      end
1.6027508097762602 + 3.5821371834839146im

[JeffBezanson]

Ok, fine, I guess I have a bit of a soft spot for local operators 😛 This can be rebased and merged.

[simeonschaub]

Should be ready to merge now