Consideration of PM_INDEX_WRITE_NODE

[hasumikin]

As of 0.30.0, Prism.parse("a[0] = 1") and Prism.parse("a.[]=(0, 1)") will generate almost the same AST (I know we can distinguish them by the flags of CallNode though) like follows:

(Flags are omitted)
@ ProgramNode (location: (1,0)-(1,12))
+-- locals: []
+-- statements:
    @ StatementsNode (location: (1,0)-(1,12))
    +-- body: (length: 1)
        +-- @ CallNode (location: (1,0)-(1,12))
            +-- receiver:
            |   @ CallNode (location: (1,0)-(1,5))
            |   +-- receiver: nil
            |   +-- call_operator_loc: nil
            |   +-- name: :array
            +-- call_operator_loc: nil
            +-- name: :[]=
            +-- message_loc: (1,5)-(1,8) = "[0]"
            +-- opening_loc: (1,5)-(1,6) = "["
            +-- arguments:
            |   @ ArgumentsNode (location: (1,6)-(1,12))
            |   +-- arguments: (length: 2)
            |       +-- @ IntegerNode (location: (1,6)-(1,7))
            |       |   +-- value: 0
            |       +-- @ IntegerNode (location: (1,11)-(1,12))
            |           +-- value: 1
            +-- closing_loc: (1,7)-(1,8) = "]"
            +-- block: nil

The point is that a[0] = 1 does not generate an assignment node, a kind of PM_somehow_WRITE_NODE, despite being an assignment expression.

Matz says it is possibly a bug:
mruby/mruby#6303 (comment)

I guess, you may need a thing like PM_INDEX_WRITE_NODE for a[idx] = object.
What do you say?

[kddnewton]

We could potentially consider another node for this, but that's what the attribute_write flag on call nodes is supposed to indicate. a[0] = 1 will have the attribute_write flag and a.[]=(0, 1) will not. Similarly, foo.bar = 1 will have the attribute_write flag. You can see how we use it in the CRuby compiler (https://github.com/ruby/ruby/blob/371790165f65d195a1dd6bd615dc6fd42eed8f94/prism_compile.c#L3477).

Would that flag work for you, or do you think we should try a new node?

[hasumikin]

@kddnewton Thanks for the reply (and thank you for your all-time effort!)

Would that flag work for you,

Yes, technically, I can compile it correctly by looking at the flags.

My personal opinion about this case: a[idx] = object is that it feels a bit strange that the BIG structure of the AST (if I can say so) doesn't express that this is an assignment expression.
a[] = obj and a.[]=(obj) are fundamentally different in terms of grammar.

For me, a tricky part is knowing whether I need to look at the flags carefully all the time.
For example, I usually don't need to worry about whether the flags for an integer literal (PM_INTEGER_NODE) ​​are decimal or hexadecimal. On the other hand, I must not overlook the PM_REGULAR_EXPRESSION_NODE's flag.
But these two examples appear both natural because there is no fundamental grammatical difference while the flags express only the details in the same grammar.

Well, I don't have a strong thought. It might be a good idea to hear many people's opinions on this. How about making it an agenda for the Ruby core team?

[eregon]

Making the return value the same as the RHS is specifically what attribute_write is for, and since it affects multiple nodes I think it makes a lot of sense as a flag.
IMO we should keep this as-is.

Regarding whether to care or not about flags I would think for a compiler one should consider all flags on a node, most are necessary or useful for compilation, a few can be ignored. Maybe we could document flags which are expected to have no effect on compilation, OTOH it seems few of them and relatively clear already.

[kddnewton]

@hasumikin — I can definitely see your point. We do have quite a few different node types, so I can see why you would want another one here.

My reasoning for putting it into one node: one of the biggest complaints with the existing CRuby AST was that there were too many different call node types. (QCall, VCall, FCall, Command, CommandCall, Call, etc.) It made it very difficult for consumers of the AST on the static analysis side to know that they had covered all call sites. While we have already deviated from that a bit (because of control-flow calls which could represent many method calls and target calls which have different field sets) we've mostly tried to isolate all of the method calls into one node to make it easier on those consumers.

The other piece of this is that while making this into a new node will solve your problem for a[0] = 1, it will not solve your problem for a.b = 1 (you will still have to check the flag to get this right). So in this case adding a new node will cause everyone to have to handle it differently, but not fully solve the original problem of having to check the flags.

For these reasons, I'd like to keep it in the same node for now. But thank you very much for the thoughtful issue!