Skip to content

Fix escaping bugs in UseTextBlocks #215

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
May 3, 2023
Merged

Fix escaping bugs in UseTextBlocks #215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3528f94
fix backslashes not escaped in UseTextBlocks
aksh1618 Apr 26, 2023
1d09657
fix triple quotes inside string not escaped in UseTextBlocks
aksh1618 Apr 26, 2023
20173f6
add test for ending quote handling in UseTextBlocks
aksh1618 Apr 26, 2023
ac5610a
add clarifying comments for different cases handled in UseTextBlocks
aksh1618 Apr 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/main/java/org/openrewrite/java/migrate/lang/UseTextBlocks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,14 +141,27 @@ private J.Literal toTextBlock(J.Binary binary, String content, List<J.Literal> s
String indentation = getIndents(concatenation.toString(), useTab, tabSize);

boolean isEndsWithNewLine = content.endsWith("\n");

// references:
// - https://docs.oracle.com/en/java/javase/14/docs/specs/text-blocks-jls.html
// - https://javaalmanac.io/features/textblocks/

// escape backslashes
content = content.replace("\\", "\\\\");
// escape triple quotes
content = content.replace("\"\"\"", "\"\"\\\"");
Copy link
Contributor

@kunli2 kunli2 May 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

do you know why the triple quotes are transformed to \"\"\\\" but not \\\"\"\", the difference is the 1st or 3rd quote is escaped.
I found for an 8 quotes case:

        String s1 = "\"\"\"\"\"\"\"\"";
        String s2 = """
                                ""\"""\"""\
                                """;
        String s3 = """
                                \"""\"""\""\
                                """;

s1 and s2 are equivalent, however,

        String s3 = """
                                \"""\"""""\
                                """;

doesn't compile unless change it to

        String s3 = """
                                \"""\"""\""\
                                """;

which makes me think to escape the 3rd quote instead of 1st is like magic.
Do you have any insight of this?

Copy link
Contributor Author

@aksh1618 aksh1618 May 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@kunli2 I went for escaping the third quote instead of the first to optimize for the number of escapes needed in case of n number of continuous quotes (For example: 2 escapes instead of 3 in case of 8 quotes). Another thing I noticed was that IDEA by default escapes the third quote as well, for instance if you copy-paste a series of quotes inside a text block.

In your examples, I can't see any difference between s2 & s3 (both), did GitHub formatting probably make them look the same? Could you share it in some other form such as a gist so that I can try to understand it better, maybe then I could answer it better?.

Copy link
Contributor

Choose a reason for hiding this comment

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

hi @aksh1618 , yes, I also found IDE by default escapes the 3rd quote.
what I noticed is below, the first statement in the code doesn't compile (\"""\""""" doesn't compile but \"""\"""\""\ does). I guess that is the reason why IDEA escapes the 3rd quote instead of 1st.

String s3 = """
                                \"""\"""""\
                                """;
String s3 = """
                                \"""\"""\""\
                                """;

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for the confusion in my 1st comment, I have revised the code in the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kunli2 thanks for clarifying, it's a very good example! So to summarize, in this case if we're replacing programmatically successively, escaping the first one would not work as it leads to code that doesn't compile due to 4 quotes left together. So now it makes sense why they went with the third one as well instead of having to write a more complex replacement algorithm for handling leftover quotes 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@aksh1618 yeah, it makes sense! thanks for fixing and contribution again!

// preserve trailing spaces
content = content.replace(" \n", "\\s\n");
// handle preceding indentation
content = content.replace("\n", "\n" + indentation);
// handle line continuations
content = content.replace(passPhrase, "\\\n" + indentation);

// add first line
content = "\n" + indentation + content;

// add last line to ensure the closing delimiter is in a new line to manage indentation
// add last line to ensure the closing delimiter is in a new line to manage indentation & remove the
// need to escape ending quote in the content
if (!isEndsWithNewLine) {
content = content + "\\\n" + indentation;
}
Expand Down
171 changes: 171 additions & 0 deletions src/test/java/org/openrewrite/java/migrate/lang/UseTextBlocksTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,177 @@ class Test {
);
}

/**
* Single escaping a quote in a string literal provides: " -> \"
* <p>
* On converting this to a text block, we can let go of the escaping for the double quote: \" -> "
*/
@Test
void singleEscapedQuote() {
rewriteRun(
//language=java
java(
// Before:
// String json = "{" +
// "\"key\": \"value\"" +
// "}";
"""
class Test {
String json = "{" +
"\\"key\\": \\"value\\"" +
"}";
}
""",
// After:
// String json = """
// {\
// "key": "value"\
// }\
// """;
"""
class Test {
String json = ""\"
{\\
"key": "value"\\
}\\
""\";
}
"""
)
);
}

/**
* Double escaping a quote in a string literal provides: " -> \" -> \\\"
* <p>
* On converting this to a text block, the escaped backslash should remain, but we can let go of the
* escaping for the double quote: \\\" -> \\"
*/
@Test
void doubleEscapedQuote() {
rewriteRun(
//language=java
java(
// Before:
// String stringifiedJson = "{" +
// "\\\"key\\\": \\\"value\\\"" +
// "}";
"""
class Test {
String stringifiedJson = "{" +
"\\\\\\"key\\\\\\": \\\\\\"value\\\\\\"" +
"}";
}
""",
// After:
// String stringifiedJson = """
// {\
// \\"key\\": \\"value\\"\
// }\
// """;
"""
class Test {
String stringifiedJson = ""\"
{\\
\\\\"key\\\\": \\\\"value\\\\"\\
}\\
""\";
}
"""
)
);
}

/**
* Triple quotes in a string literal are escaped as: """ -> \"\"\"
* <p>
* On converting this to a text block, only one of the quotes needs to be escaped: \"\"\" -> ""\"
*/
@Test
void tripleQuotes() {
rewriteRun(
//language=java
java(
// Before:
// String myFaceInASCII = "\"\"\"\"\"\"\"\"\n" +
// "| o o |\n" +
// "| == |\n" +
// "\\------/\n";
"""
class Test {
String myFaceInASCII = "\\"\\"\\"\\"\\"\\"\\"\\"\\n" +
"| o o |\\n" +
"| == |\\n" +
"\\\\------/\\n";
}
""",
// After:
// String myFaceInASCII = """
// ""\"""\"""
// | o o |
// | == |
// \\------/
// """;
"""
class Test {
String myFaceInASCII = ""\"
""\\""\"\\""\"
| o o |
| == |
\\\\------/
""\";
}
"""
)
);
}

/**
* Quote in a string literal is escaped as: " -> \"
* <p>
* On converting this to a text block, it needs to be escaped if it is the last character:
* <pre>
* """
* "test\"" -> test\""""
* </pre>
* However, this is not required in case we use the newline escape (\) to put the ending delimiter on the next line:
* <pre>
* """
* "test\"" -> test"\
* """
* </pre>
*/
@Test
void endingQuote() {
rewriteRun(
//language=java
java(
// Before:
// String myPlay = "Alice: \"Hi Bob!\"\n" +
// "Bob: \"Don't use plaintext Alice!\"";
"""
class Test {
String myPlay = "Alice: \\"Hi Bob!\\"\\n" +
"Bob: \\"Don't use plaintext Alice!\\"";
}
""",
// After:
// String myPlay = """
// Alice: "Hi Bob!"
// Bob: "Don't use plaintext Alice!"\
// """;
"""
class Test {
String myPlay = ""\"
Alice: "Hi Bob!"
Bob: "Don't use plaintext Alice!"\\
""\";
}
"""
)
);
}

@Disabled
@Test
void grouping() {
Expand Down