Replace Value with Expression in the grammar

[cmnrd]

This makes a first step at implementing the spec in #986 by creating an extensible Expression syntax in replacement of the old Value rule. In consequence of the grammar changes, this PR also includes a ton of refactoring.

This PR is based on #1022 and includes its changes.

[edwardalee]

I'm not understanding this PR. It seems to be mostly about renaming the Value AST node to Expression without changing what it means. Is this meant to be a stepping stone towards supporting expressions?

[edwardalee]

I'm confused by this. I would expect an Expression to somehow different from a Value. This looks to me the same as what we used to call a Value.

[lhstrh]

I think the idea here is that @cmnrd plans to augment this rule later on. This PR is just meant to set the stage for that.

[cmnrd]

Yes, this a first step at introducing an expression syntax. This PR only converts what we now call a Value to an Expression. However, at the same time the resulting model is adjusted such that we have a base class Expressions and the different types of expressions (ParameterReferecen, Literal, Code, Time) are child classes. This enables easy addition of new expression rules in the future. Also, this allows for a better design of our code generators where we can use the Visitor pattern (in Java) or when blocks (in Kotlin) to differentiate the different expression types. This, however, is not part of this PR yet.

[petervdonovan]

The LSP tests have been a hindrance for this branch... sorry. I suspect that if we re-ran the tests, the LSP tests would be more likely to pass than not. This annoying inconsistency exists because I deliberately set them up to use a different random seed each time, on the grounds that as we run CI each day, we might randomly catch more edge cases over time.

This time, the LSP tests seem to be failing because the body of the STP handler in test/Python/src/federated/DistributedCountDecentralizedLateDownstream.lf is not being code-generated. This does not seem right to me. Maybe @Soroosh129 could explain what is going on?

[Soroosh129]

This time, the LSP tests seem to be failing because the body of the STP handler in test/Python/src/federated/DistributedCountDecentralizedLateDownstream.lf is not being code-generated. This does not seem right to me. Maybe @Soroosh129 could explain what is going on?

I can't seem to replicate this issue. The STP handler is being generated for me on master and this branch:

Python

C

[petervdonovan]

I was mistaken; thanks to Soroush for patiently explaining. The real reason for the most recent test failure in this branch is that instead of having toText insert tags while toUntaggedText does not insert tags, we have a toText function that does not insert tags and a toTaggedText function that does insert tags. This makes inserting tags more like an "opt-in" than an "opt-out."

I don't feel strongly about this, and I am sure there is a reason for this change. However, I am not convinced that it is the best choice. In ASTUtils.toUntaggedText (which is now deleted), I had left a comment stating that toUntaggedText only existed because we were analyzing and patching the contents of code blocks using regular expressions, which is a practice that probably should be discouraged. I think that verbatim code should nearly always have tags, so with the behavior of toText that is in this branch, I will probably have to do another pass over all the code generators to change most mentions of toText to toTaggedText.

[lhstrh]

I think I lean towards reverting the mechanism to opt-out instead of having opt-in.

[lhstrh]

This looks good to me! I guess the only minor aesthetic issue I see that not all developers might initially be aware of what tagging is and whether they should use toText or toUntaggedText. Perhaps we can come up with a better name for toUntaggedText that highlights its typical use case rather than the presence of tags? Maybe toOriginalText? Or toVerbatimText? IDK...

[cmnrd]

Thanks for finishing this up! With regard to the discussion about toText and toTaggedText, I found that toText is also used a lot outside of the generators and there the additional tags are pretty confusing. However, I like the new to toOriginalText name for the function and it should be pretty clear now.