Syntax error when multiple comments in braceless IF

[gregcaban]

Parser fix for a small bug for when setRecordingComments(true) and if 2 consecutive comments in a braceless if statement could lead to EvaluatorException (syntax error) due to bad AST being produced. For example

if (x)
    // comment 1
    // comment 2
    doSomething();
else
    doSomethingElse();

This was happening due to the second comment node being consumed instead of the if's body and as the original body node ( doSomething() above) is found where 'else' keyword was expected, leading to a syntax error.

The fix is to just iterate on comment nodes until the real body expression is found and saving last comment as InlineComment.

That means that if .toSource() was called on that AST, we'd lose the first comment in example above. I decided against that as it would require changes in AstNode to allow collecting a list of Inline Comment and use it toSource, which felt like to big of a change for this arguably edge case bug. Happy to explore it further if you feel it's worth it.

[andreabergia]

This looks fine to me.

A possible alternative could have been to modify the parsing to "merge" the two comment nodes into just one, with an embedded newline, but I don't mind this fix either.

[rbri]

A possible alternative could have been to modify the parsing to "merge" the two comment nodes into just one, with an embedded newline, but I don't mind this fix either.

like that suggestion

[gregcaban]

A possible alternative could have been to modify the parsing to "merge" the two comment nodes into just one, with an embedded newline, but I don't mind this fix either.

like that suggestion

That's clever, I like the idea. Implemented in 8b0f109

[aardvark179]

Merging comments makes absolute sense in our parser, because we don't generally need to care about them exceed in terms of their spans.

It might be a problem if anybody is using our parse tree for analysis, but it's probably not a big problem.

[gregcaban]

Merging comments makes absolute sense in our parser, because we don't generally need to care about them exceed in terms of their spans.

It might be a problem if anybody is using our parse tree for analysis, but it's probably not a big problem.

Just to clarify, the inline comments that we're merging here are only used to emit string when .toSource() is called. The original Comment nodes are untouched by proposed change, so it shouldn't affect any AST based analysis.

[andreabergia]

I like this new approach even better.

[gbrail]

Thanks!

According to my AI code review friend:

  The original commits introduced a duplication issue in
  the AstRoot.getComments() collection. While they correctly merged
  consecutive comments for the inlineComment property, they left the
  original individual comment nodes in the global list. This would have
  caused tools that iterate over all AST comments to see the same content
  multiple times.

It suggested a test case:

@Test
public void testCommentContentAfterMerging() {
    String source =
            "if (x)\n" + "    // comment 1\n" + "    // comment 2\n" + "    doSomething();\n";

    CompilerEnvirons env = new CompilerEnvirons();
    env.setRecordingComments(true);

    Parser parser = new Parser(env);
    AstRoot ast = parser.parse(source, "test", 1);

    // Check how many comments are in the global list
    java.util.SortedSet<org.mozilla.javascript.ast.Comment> comments = ast.getComments();
    Assert.assertEquals(1, comments.size());

    java.util.Iterator<org.mozilla.javascript.ast.Comment> it = comments.iterator();
    org.mozilla.javascript.ast.Comment c1 = it.next();

    Assert.assertEquals("// comment 1\n// comment 2", c1.getValue());
}

and a fix:

 --- a/rhino/src/main/java/org/mozilla/javascript/Parser.java
+++ b/rhino/src/main/java/org/mozilla/javascript/Parser.java
@@ -1707,6 +1707,7 @@ public class Parser {
                 sb = new StringBuilder(mergedComment.getValue());
             } else {
                 sb.append('\n').append(((Comment) body).getValue());
+                scannedComments.remove(scannedComments.size() - 1);
             }
             body = statement();
         }

This all makes sense to me, actually, and I wouldn't have caught it myself. What do you think?

[gregcaban]

Thanks @gbrail, it's adressed in d84256b.

[gbrail]

Looks good now, thanks!