ClassNodes and ModuleNodes now always have ConstantPathNode children

[jemmaissroff]

If a ClassNode has a simple path, like "Foo", it will be represented as a ConstantPathNode with a ConstantReadNode parent, and a nil child. This is so that all ClassNodes and ModuleNodes have consistent path nodes, and not path nodes which could be either ConstantReadNodes or ConstantPathNodes.

This PR simplifies how any consumer of YARP has to handle ClassNodes or ModuleNodes. Before this, the logic for splitting on path / read node had to be both in the ConstantPathNode and in the ClassNode / ModuleNode. This commit minimizes the places where this splitting is duplicated, and makes the compiler (or any consumer of YARP) slightly simpler.

[kddnewton]

A constant path node with a nil parent is supposed to represent ::Foo, so I think we're losing the ability to represent that if we did this.

[enebo]

There is also an opportunity here to have something turn up red after this change since this PR was still green.

[enebo]

I guess lex-100 did fail but it is unclear it caught ::Foo case from that assertion.

[jemmaissroff]

A constant path node with a nil parent is supposed to represent ::Foo, so I think we're losing the ability to represent that if we did this.

I thought about this - that's why I made it a nil child (not parent) for the Foo case to distinguish it from the ::Foo case

[enebo]

Sorry to ask this question here but since you are both looking at this code here. For the ConstantPath I should be looking to see if parent is Nil and not null here right?

    Operand getContainerFromCPath(Node node) {

        if (node instanceof ConstantReadNode) {
            return findContainerModule();
        } else if (node instanceof ConstantPathNode) {
            ConstantPathNode path = (ConstantPathNode) node;

            if (path.parent == null) { // ::Foo
                return getManager().getObjectClass();
            } else {
                return build(path.parent);
            }
        }
        // FIXME: We may not need these based on whether there are more possible nodes.
        throw notCompilable("Unsupported node in module path", node);
    }

I am mildly surprised I have not been tripped up by this but I am only loading like the first 100 files of JRuby booting up.

[jemmaissroff]

Sorry to ask this question here but since you are both looking at this code here. For the ConstantPath I should be looking to see if parent is Nil and not null here right?

What is the difference between Nil and null?

[enebo]

Sorry to ask this question here but since you are both looking at this code here. For the ConstantPath I should be looking to see if parent is Nil and not null here right?

What is the difference between Nil and null?

I don't know either. I guess I am confused by this statement "nil parent is supposed to represent ::Foo". Is that just a null field? I was unclear if @kddnewton had meant an actual nil value as special value holder or just no value at all.

[jemmaissroff]

I don't know either. I guess I am confused by this statement "nil parent is supposed to represent ::Foo". Is that just a null field? I was unclear if @kddnewton had meant an actual nil value as special value holder or just no value at all.

I think he means no value at all here.

[enebo]

@jemmaissroff thanks. I guess I found it strange I could execute so much Ruby and not handle ::Foo if it had been a special nil holding value.

[kddnewton]

Sorry, here's the mapping today:

Foo        # ConstantReadNode("Foo")
::Foo      # ConstantPathNode(nil, ConstantReadNode("Foo"))
Foo::Bar   # ConstantPathNode(ConstantReadNode("Foo"), ConstantReadNode("Bar))
::Foo::Bar # ConstantPathNode(ConstantPathNode(nil, ConstantReadNode("Foo")), ConstantReadNode("Bar"))
foo::Bar   # ConstantPathNode(CallNode("foo"), ConstantReadNode("Bar"))

The nil in this case would map to null in Java. The idea here is that you can consistently compile ConstantPathNode by looking at the parent and compiling it (compiling in the const base if it's nil) and then compiling a constget with the child.

Sorry I didn't understand before that you had changed the parent. To me that's kind of confusing, because now you can't compile it in the same way as other ConstantPathNodes, you now have to check if the child is nil either in just this special case or in all cases.

I understand that you're trying to simplify this for consistency's sake. Is there another solution we could come to? For me it is very confusing to have a non-null parent but a null child, but also because it allocates a lot memory for every class and module definition now.

[eregon]

I think a ConstantPathNode with nil/null child is too confusing, it would be like Foo::nil or Foo:: intuitively which doesn't make much sense like that.

I think the logic to handle all 3 cases (Foo, Foo::Bar, ::Foo) can be extracted in a small helper method like here and here, where it doesn't matter much that there is 2 different node types.

We could have a common trait/interface for ConstantPathNode & ConstantReadNode to have a slightly more precise type, but I'm not sure it would give us much here, besides making it clearer when reading that this field can only be one of those two.

[eregon]

We could have a common trait/interface for ConstantPathNode & ConstantReadNode to have a slightly more precise type, but I'm not sure it would give us much here, besides making it clearer when reading that this field can only be one of those two.

Actually I noticed the JRuby AST has that and uses it to define getName() which is convenient here

[enebo]

I think null as first value is not that confusing. {null}::Foo is just not hard to reason out if you know Ruby already. You could argue it could be a special node type to remove that extra word of memory to store null. MRI and JRuby do differentiate this by having COLON3 but fwiw COLON3s are not common.

I echo @kddnewton that universally using constantpath would significantly increase the memory of class/module definitions.

[jemmaissroff]

What about having a required class name (ConstantReadNode) and optional class path (ConstantPathNode)? CRuby needs the class name isolated anyways. The class path could just have parents of the required class name. The only case I haven't figured out is ::Foo, but it might make sense to have a flag that indicates it's global (or another optional field).

Agreed, it is possible for each Ruby implementation using YARP to make helpers that do the same thing. Or YARP can expose those helpers (slightly better). But to me, either of these indicate a case where YARP's node structure is not reflecting what's helpful to consumers.

[eregon]

I think null as first value is not that confusing. {null}::Foo is just not hard to reason out if you know Ruby already.

Yes, agreed, I think that's fine, for a ConstantPathNode with nil/null child is too confusing I mean what is added in this PR, not what already exists (the child there refers to the second field not "a child field").

What about having a required class name (ConstantReadNode) and optional class path (ConstantPathNode)? CRuby needs the class name isolated anyways. The class path could just have parents of the required class name. The only case I haven't figured out is ::Foo, but it might make sense to have a flag that indicates it's global (or another optional field).

I like that idea. I think either a flag or a special node (like in #1143) to differentiate ::Foo and Foo would be good.
I think the special node might be less error-prone.
The flag sounds easy to forget to check, and then it would be incorrect handling. But also not the end of the world and likely good enough if the docs of ConstantReadNode mention it clearly.
And obviously the special node is worse in terms of footprint, at least for C.

[enebo]

What about having a required class name (ConstantReadNode) and optional class path (ConstantPathNode)? CRuby needs the class name isolated anyways. The class path could just have parents of the required class name. The only case I haven't figured out is ::Foo, but it might make sense to have a flag that indicates it's global (or another optional field).

As you say we have to always do this split in processing anyways so I like that part of this. I don't really have strong feelings on how to differentiate ::Foo from Foo but a special node means that knowledge would be contained in the class path and not in the class/module which could mean that special node processing would just be an if which returns Object. This would also make generic constant processing be able to use it as well as classes and modules.

As a special object then for modules/classes {cpath, name} would have cpath of special for :: and null for self. What special is I guess another piece of the puzzle.

[enebo]

Also if these end up being fields for class/module node I think you can omit the ConstantReadNode and just use Location since we know the rhs of the cpath has to be a constant identifier already.

[eregon]

Also if these end up being fields for class/module node I think you can omit the ConstantReadNode and just use Location since we know the rhs of the cpath has to be a constant identifier already.

By this you mean the RHS of a ConstantReadNode could use the constant pool, right? Pretty sure we agree we don't want to rely on location fields for compilation.

[enebo]

@eregon yeah constant pool entry.

[jemmaissroff]

Closing this. Discussed with Kevin, he implemented adding a name in #1325, and will implement another PR to add a new node type for the ::Foo case so we don't have to do a nil check there