store strings instead of tokens in nodes

[iliabylich]

Per #5.

Also, I think the format of the AST is redundant in many places, for example there's no need to store actual value for constant tokens like while, or colon, because they are constant. It makse more sense to simply store their location in source input (just like whitequark/parser does).

Dynamic values should be stored of course, things like IDENTIFIER must be stored by value and by location too (whitequark/parser stores them as separate fields like name and name_l)

[iliabylich]

I'll take a look at CI failure a bit later today.

[iliabylich]

@kddnewton I understand that you are still busy with your other work, so no rush. PR is ready but it can wait of course.

I'm pretty sure that nodes shouldn't be based on tokens in general. It does make sense in many cases (especially for things like keywords or literals), but there are cases when location should be computed dynamically, for example here:

def foo(bar:)
end

^ the token is LABEL("bar:"), but the name of the argument is just bar and there are more cases like that, I can grab some if you want. Basically the whole AST builder of whitequark/parser (and lib-ruby-parser) is written manually from scratch to solve cases like this.

On the other side I understand that it's much easier to embed tokens, and maybe we could go with this approach at first and then slowly change it to be manual. I suppose right now there are more obvious things that must be solved first (like the absence of the whole parser lol). This is simply a thing that instantly caught my eye, but I don't know if we should focus on it rn.

[kddnewton]

So in general I'm keen to reduce the depth of the tree, which this accomplishes. So I'm definitely open to the idea of it. However, on the Ruby side of things, I definitely don't want to lose as much information as this PR would cause it to.

As a simple example, with the Binary node if you're consuming this from the Ruby side, you no longer know where the operator was in source relative to the left and right child nodes. This has caused problems for both syntax_tree and error_highlight. I'd rather we retain that information so it can be used by consumers.

That being said, yeah, we're definitely going to run into limits with the templating at some point. I don't think it's going to be a long term solution for all of our AST building. It gets especially hairy with the location information as things become more and more optional. I'm not sure of the right solution yet, but I definitely want to leave this open for a while so we can keep talking about it.

[iliabylich]

Sorry for not being clear. My goal is to keep all information about all locations as well.

This PR is somewhat more about changing rules of how nodes are constructed. Of course in its current form it introduces quality degradation, for example VariableReference no longer knows what is assigned (a local variable or an instance variable).

I've sent this PR to change templates and node constructors and I think it makes sense to replace tokens with strings + locations (similar to how other parsers do it), but not in a single PR 😄 . It's quite big as it is, and I'm not even sure it's possible to easily review it.

[kddnewton]

Yeah I definitely agree about tracking strings. We're going to need to do this eventually, I just don't think we can do it just yet.

[iliabylich]

Ok, I'm closing it for now. Honestly I think we can iterate much faster with tokens that are not 100% precise in a few case (like those mentioned above).

Later we can re-open it once we have the parser that works 😄