CallNode needs to denote operator call type

[enebo]

Item for #807. CallNode.call_operatior is a token but we need to know whether the call is '.' or '&.'.

[eregon]

A boolean is_safe_navigation/safe_navigation field seems a good fit for that

[jemmaissroff]

Did we not want to split this into two different call nodes?

[enebo]

@jemmaissroff That works if CallNode does not become FCallNode, CallNode, and VCallNode. I would like to not have null fields for these 3 (for example all fcalls have a null receiver) but I am not sure if people are on board with that change. If there are 3 then the boolean makes sense. If not then the second type makes sense.

[eregon]

IMO it doesn't make sense to use a different node type for safe navigation, because if you already have different call nodes like FCallNode, CallNode, and VCallNode then you'd end up with combinations and 6 of them (as @enebo just said).

And if we don't --- like currently and by design as discussed with Kevin, it's just so much nicer to have to deal with a single CallNode type when using them, e.g. in compile.c --- then breaking that to save one byte seems hardly worth it.

Regarding VCallNode I think it's useless information for semantics (it just implies 0/no arguments, not really interesting, vcall just means it needed to be resolved between local var or call AFAIK, something relevant during parsing but not for execution and neither for source analysis/transformation/etc). FCallNode is just CallNode with no receiver, that's just saving one byte for lots of code duplication, IMO not worth it. Also BTW ignoring visibility is not simply just is it a FCallNode, because e.g. self.foo = ... also ignores visibility.

So a boolean seems best. And if we want to optimize size more we could have a byte for both the safe navigation flag and the ignore visibility flag (which would be convenient if YARP computes it, but is not necessary, usages can do it themselves based on the receiver node).

[enebo]

@eregon I will only push back a little bit on the notion that having 3 nodes is not useful. Semantically I know an fcall has an implicit self without performing a null check. I also know that if a call ever receives a null receiver during parsing it is a bug and not an fcall. To me 3 simple methods which call out for common parts like buildArguments() is preferable. Especially since we have other call types which are different nodes (like super -- so we will have those methods anyways).

For space considerations fcalls are pretty common. You are trading that space for approximately 11 lines of code (assuming you have one inner impl 3 - 3line methods + 2 lines for the begin end of a inner method). IMO this is not a good trade.

Having said all that if you both disagree about this I can accept things as they are.

[eregon]

Here are the all fields and boolean flags for a call node in TruffleRuby:

    private final RubyNode receiver;
    private final String methodName;
    private final RubyNode block;
    private final ArgumentsDescriptor descriptor;
    private final RubyNode[] arguments;

    private final boolean isSplatted;
    private final boolean ignoreVisibility;
    private final boolean isVCall;
    private final boolean isSafeNavigation;
    private final boolean isAttrAssign;

I think we should encode these 5 boolean flags as a bitmask, then it takes one byte and has all information easily available and there is no need to create our own helpers to share logic. With that I think it's clear there is no value to have things like FCallNode, it's not smaller in footprint (well, except a zero byte for no receiver for fcall), and FCallNode does not imply any value for e.g. isSafeNavigation or isSplatted (AFAIK).

Ah BTW we do need to know if it's a vcall (I forgot), because the error differs: it's NameError: undefined local variable or method '%s' for %s for vcall instead of NoMethodError: undefined method '%s' for %s.

[enebo]

" it's not smaller in footprint (well, except a zero byte for no receiver for fcall)"

That was my main point about footprint but it is ok.

The fact that we need a value for safe navigation means we need to use a byte and with that point we then have 7 more bits. This gets rid of other possible nodes like attr assign (in JRuby this is a specialized type). Those flags all look ok (we determine splat when processing argsnode but it is fine to know it ahead of time).

ignoreVisibility is put on fcalls, vcalls and calls with self? Is that right?

Not to beat a dead horse but in TR other than isSplatted you need no other fields for an fcall. As I said I am ok with what decision is arrived at but for fcalls if you just derived issplatted (vs a flag) you would be saving 2 bytes per fcall (since all other flags do not apply).

[eregon]

Now that we have flags, we should adds flags for CallNode

[kddnewton]

@enebo, @eregon we now have a safe navigation flag on call nodes. We can add vcall/fcall going forward as well. We could also potentially add some special serialization logic to cut down on fcall size when these flags are present.

[eregon]

@kddnewton I think for CallNode we need these flags: Done:

• isSafeNavigation

To do:

• isVCall -> necessary for NameError vs NoMethodError
• ignoreVisibility -> this would be very convenient for compilation, and avoid reimplementing the logic of ignore visibility if no receiver or if receiver is self (which sometimes isn't trivial, e.g., in self.foo += 42)
• isSplatted -> seems a bit more convenient than checking if child is SplatNode, and then for compilation we wouldn't need that SplatNode anymore and save that allocation. But probably we need SplatNode for tooling, so then this is probably best as-is for now.