Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[java] Make binary operators left-recursive #1979

Merged
merged 9 commits into from
Sep 27, 2019

Conversation

oowekyala
Copy link
Member

@oowekyala oowekyala commented Aug 15, 2019

So this should fix #1661 definitively

The structure of infix expressions is now always the same: a left-hand side, an operator, a right hand side. Since having different node types for each of those adds no information that the operator constant doesn't already provide, I think it's useful to merge all those nodes (there's 10) into a single one. This decreases the size of the API, and is for example what Eclipse JDT does

I also experimented with generating nice tree images for our javadocs. For example:

Capture du 2019-08-15 02-51-07

Unfortunately rendering the images is not really portable for now, as the build machine needs a LaTeX distribution. So I prerendered them and committed them here.

Copy of the discussion on left-recursiveness

What to do about #1661 (operator nodes) ? Sensible solution would be to make AdditiveExpression be parsed completely left-recursively, but then, it looks weird that all other binary expressions are not left-recursive…

The problem also arises for operators &, |, ^, which may be boolean non-shortcut expressions or numeric bitwise expressions

Arguments for left recursiveness:

  • Preserve all intermediary type resolution results, because of numeric promotion -> the whole point of [java] RFC: new layer of abstraction atop PrimaryExpressions #497
  • AST is JLS-like, easier to document (we don’t have to explain “sometimes it’s left-recursive and sometimes not”)
    ((1 + 4) + returnString()) = “14” “5”
    ((1 + 4) + 4) == (1 + 4 + 4)
  • Easier mapping of type resolution algorithms from the JLS
  • Stronger contract for binary expressions: all can present the same interface:
getLhs() : ASTExpression
getRhs() : ASTExpression
getOperator() : BinaryOp
  • Classpath independent

Arguments against:

  • Not super intuitive
  • Breaks away from previous AST structure, needs to be relearned
  • Needs more tooling for some use cases:

Eg how to catch a string concat statement like a = … + a + …; (use case taken from #1932's AvoidConcatInLoop)

  • In Java we can imagine the following method for binary expressions which yields all operands that are added with the same operator: operandStream(): NodeStream<ASTExpression>
    Eg for <a + b + c * d>, operandStream returns Stream.of(<a>, <b>, <c * d>). So you can do eg:
 additiveExpr.operandStream()
                   .filter(ASTVariableAccess.class)
                   .any(ref -> ref.getName().equals(“a”))
  • However in XPath it’s less trivial.. We’d have to provide access to the node stream with a function:
    • Old way, with flat additive expr:
AdditiveExpression[@Operator=”+”]/VariableReference[@Image = “a”]
  • New way:
AdditiveExpression[@Operator=”+”]
     [pmd-java:operandsOf(.)/self::VariableReference[@Image = “a”]]

@oowekyala oowekyala added the in:ast About the AST structure or API, the parsing step label Aug 15, 2019
@oowekyala oowekyala added this to the 7.0.0 milestone Aug 15, 2019
@pmd-test
Copy link

1 Message
📖 No java rules are changed!

Generated by 🚫 Danger

@oowekyala oowekyala merged commit 8d47824 into pmd:java-grammar Sep 27, 2019
@adangel
Copy link
Member

adangel commented Sep 28, 2019

I've added doc to https://github.com/pmd/pmd/wiki/Java_clean_changes#binary-operators-are-left-recursive - please update it, if I didn't get it right, thanks!

@oowekyala oowekyala deleted the grammar-left-recursive-ops branch October 1, 2019 08:50
@adangel adangel mentioned this pull request Jan 23, 2023
55 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in:ast About the AST structure or API, the parsing step
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants