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

Fix TextBlockLiteralExpr in LexicalDifferenceCalculator #3937

Merged

Conversation

blacelle
Copy link
Contributor

@blacelle blacelle commented Mar 2, 2023

Suggested fix for #3936.

@jlerbsc
Copy link
Collaborator

jlerbsc commented Mar 2, 2023

I'm not sure if this is the right solution. I need to take the time to look into this problem.

@blacelle
Copy link
Contributor Author

blacelle commented Mar 2, 2023

#3936 (comment)

We also see a comment:

// FIXME: csm should be CsmTextBlock -- See also #2677

And in CsmTextBlock, we see a \n after """:

@Override
public void prettyPrint(Node node, SourcePrinter printer) {
    // Note that values within TextBlocks ALWAYS have the \n line ending, per https://openjdk.java.net/jeps/378#1--Line-terminators
    printer.print("\"\"\"\n");
    // TODO: Confirm if we need to force this to use {@code \n} separators
    printer.print(property.getValueAsStringAttribute(node));
    printer.print("\"\"\"");
}

This made me quite confident some '\n' has been lost in the process.

@jlerbsc
Copy link
Collaborator

jlerbsc commented Mar 2, 2023

The pretty printer and LPP do not work the same way at all. I have no doubt that it is necessary to add a line break but it is more the solution raises some questions. I need to dig a little deeper into the subject.

@jlerbsc
Copy link
Collaborator

jlerbsc commented Mar 3, 2023

Since the parser is not very restrictive on text blocks and the definition of element syntax in the LPP does not categorize text block delimiters, your proposal can be accepted.

However, you should simplify your unit test and avoid referring to your project. Thanks.

@blacelle blacelle requested a review from jlerbsc March 3, 2023 09:38
@@ -290,9 +290,9 @@ private void calculatedSyntaxModelForNode(CsmElement csm, Node node, List<CsmEle
} else if ((csm instanceof CsmString) && (node instanceof TextBlockLiteralExpr)) {
// FIXME: csm should be CsmTextBlock -- See also #2677
if (change instanceof PropertyChange) {
elements.add(new CsmToken(GeneratedJavaParserConstants.TEXT_BLOCK_LITERAL, "\"\"\"" + ((PropertyChange) change).getNewValue() + "\"\"\""));
elements.add(new CsmToken(GeneratedJavaParserConstants.TEXT_BLOCK_LITERAL, "\"\"\"\n" + ((PropertyChange) change).getNewValue() + "\"\"\""));
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To set the most appropriate eof character, you can probably use Node.getLineEndingStyle().toString().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a matter of style, it is a matter of Java specification Just like prettyPrinter adds a \n.

On a system where EOL is '\r', Node.getLineEndingStyle().toString() would break TextBlockLiteralExpr

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to CsmTextBlock.prettyPrint(Node, SourcePrinter) not relying on printer.println()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.oracle.com/en/java/javase/14/docs/specs/text-blocks-jls.html

Reading https://www.vojtechruzicka.com/java-text-blocks/ makes me feel I overlooked part of the spec

Line terminators are normalized to the ASCII LF character, as follows:

1. An ASCII CR character followed by an ASCII LF character is translated to an ASCII LF character.
2. An ASCII CR character is translated to an ASCII LF character.

I'm less comfortable with dynamic EOL in this case, but you're the boss.

Should I do the same in CsmTextBlock.prettyPrint(Node, SourcePrinter) ?

Copy link
Contributor Author

@blacelle blacelle Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    // Note that values within TextBlocks ALWAYS have the \n line ending, per https://openjdk.java.net/jeps/378#1--Line-terminators

(CsmTextBlock author also felt '\n' were preferable)

@jlerbsc jlerbsc merged commit 42ecfab into javaparser:master Mar 3, 2023
@jlerbsc jlerbsc added this to the Next release milestone Mar 3, 2023
@jlerbsc jlerbsc added the PR: Fixed A PR that offers a fix or correction label Mar 3, 2023
@blacelle blacelle deleted the FixtextBlocksInLexicalPreservingPrinter branch March 4, 2023 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Fixed A PR that offers a fix or correction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants