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.

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

fix: escape sequences in text-blocks are kept #4409

Merged
merged 5 commits into from
Jan 11, 2022
Merged

fix: escape sequences in text-blocks are kept #4409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
95497c4
- add test case with escape sequences
xzel23 Jan 6, 2022
4f1c6ac
- change junit imports to junit.jupiter to match the @Test annotation
xzel23 Jan 6, 2022
92bcf51
fix LiteralHelper.getTextBlockToken() and enable unit test
xzel23 Jan 7, 2022
af0ae64
set line separator on windows
xzel23 Jan 7, 2022
e3c02df
use String.replace(CharSequence, CharSequence) instead of String.repl…
xzel23 Jan 10, 2022
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
3 changes: 1 addition & 2 deletions src/main/java/spoon/reflect/visitor/LiteralHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,11 @@ private static String getBasedString(Double value, LiteralBase base) {

/**
* @param literal CtTextBlock to be converted
* @param numTabs
* @return source code representation of the literal
*/
public static String getTextBlockToken(CtTextBlock literal) {
String token = "\"\"\"\n"
+ literal.getValue()
+ literal.getValue().replace("\\", "\\\\")
+ "\"\"\"";
return token;
}
Expand Down
52 changes: 43 additions & 9 deletions src/test/java/spoon/test/textBlocks/TextBlockTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
package spoon.test.textBlocks;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import spoon.Launcher;
import spoon.reflect.code.CtBlock;
import spoon.reflect.code.CtInvocation;
import spoon.reflect.code.CtLiteral;
import spoon.reflect.code.CtReturn;
import spoon.reflect.code.CtStatement;
import spoon.reflect.code.CtTextBlock;
Expand Down Expand Up @@ -44,7 +43,7 @@ public void testTextBlock1(){
CtStatement stmt1 = m1.getBody().getStatement(0);
assertTrue(stmt1.getValueByRole(CtRole.ASSIGNMENT) instanceof CtTextBlock);
CtTextBlock l1 = (CtTextBlock) stmt1.getValueByRole(CtRole.ASSIGNMENT);
assertEquals(l1.getValue(), "<html>\n <body>\n <p>Hello, कसौटी 🥲</p>\n </body>\n</html>\n");
assertEquals("<html>\n <body>\n <p>Hello, कसौटी 🥲</p>\n </body>\n</html>\n", l1.getValue());
}

@Test
Expand All @@ -57,9 +56,10 @@ public void testTextBlockqoute(){
CtStatement stmt2 = m2.getBody().getStatement(0);
assertTrue(stmt2.getValueByRole(CtRole.ASSIGNMENT) instanceof CtTextBlock);
CtTextBlock l2 = (CtTextBlock) stmt2.getValueByRole(CtRole.ASSIGNMENT);
assertEquals(l2.getValue(), "SELECT \"EMP_ID\", \"LAST_NAME\" FROM \"EMPLOYEE_TB\"\n"
assertEquals("SELECT \"EMP_ID\", \"LAST_NAME\" FROM \"EMPLOYEE_TB\"\n"
+ "WHERE \"CITY\" = 'INDIANAPOLIS'\n"
+ "ORDER BY \"EMP_ID\", \"LAST_NAME\";\n");
+ "ORDER BY \"EMP_ID\", \"LAST_NAME\";\n",
l2.getValue());
}

@Test
Expand All @@ -74,12 +74,13 @@ public void testTextBlockQouteWithinQoute(){
CtInvocation inv = (CtInvocation) stmt5.getDirectChildren().get(1);
assertTrue(inv.getArguments().get(0) instanceof CtTextBlock);
CtTextBlock l3 = (CtTextBlock) inv.getArguments().get(0);
assertEquals(l3.getValue(), "function hello() {\n"
assertEquals("function hello() {\n"
+ " print('\"Hello, world\"');\n"
+ "}\n"
+ "\n"
+ "hello();\n"
+ "");
+ "",
l3.getValue());
}

@Test
Expand All @@ -92,7 +93,7 @@ public void testTextBlockEmpty(){
CtStatement stmt1 = m4.getBody().getStatement(0);
assertTrue(stmt1.getValueByRole(CtRole.ASSIGNMENT) instanceof CtTextBlock);
CtTextBlock l1 = (CtTextBlock) stmt1.getValueByRole(CtRole.ASSIGNMENT);
assertEquals(l1.getValue(), "");
assertEquals("", l1.getValue());
}

@Test
Expand All @@ -118,4 +119,37 @@ public void testTextBlockCreation(){
c.toString()
);
}

@Test
@ExtendWith(LineSeperatorExtension.class)
public void testTextBlockEscapes(){
//contract: text-blocks should retain escape sequences in code
Launcher launcher = setUpTest();
launcher.getEnvironment().setAutoImports(true);

CtClass<?> allstmt = (CtClass<?>) launcher.getFactory().Type().get("textBlock.TextBlockTestClass");
CtMethod<?> m5 = allstmt.getMethod("m5");

CtStatement stmt5 = m5.getBody().getStatement(0);
assertTrue(stmt5.getValueByRole(CtRole.ASSIGNMENT) instanceof CtTextBlock);

// test text block value
CtTextBlock l1 = (CtTextBlock) stmt5.getValueByRole(CtRole.ASSIGNMENT);
assertEquals("no-break space: \\00a0\n" +
"newline: \\n\n" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Using replace("\\", "\\\\") would only escape the backslashes and not \n because the latter is treated as a single character and not a combination of \ + n. For example, if you run replace("\\", "\\\\") on \\\n, it would return \\\\\n. Notice that \n is not escaped. I think it would be better if we write a generic function to escape all characters or use one which already exists. I have something like this in mind:

public static String escape(String s){
  return s.replace("\\", "\\\\")
          .replace("\t", "\\t")
          .replace("\b", "\\b")
          .replace("\n", "\\n")
          .replace("\r", "\\r")
          .replace("\f", "\\f")
          .replace("\'", "\\'")
          .replace("\"", "\\\"");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as in the source code it's written as "\n" it works because at that time it's still two characters. It is not supposed to escape the newline at the end of a line in a textblock. The unit test for this passes and the original problem I had with spoon is also solved by this patch, so I am quote sure it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as in the source code it's written as "\n" it works because at that time it's still two characters.

Okay. So do we just want to escape backslashes when we put the literal inside a text block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, "normal" String literals are handled in LiteralHelper.getLiteralToken(), which in turn calls LiteralHelper.getStringLiteral(). But if we use that one, all newlines, tabs etc. are replaced by their respective escape sequences and all textblocks would be printed on a single line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. Your patch looks good to me in that case.

"tab: \\t ('\t')\n",
l1.getValue());

// escape sequences should be retained in code
String m5Text = m5.toString();
assertEquals("void m5() {\n" +
" String escape = \"\"\"\n" +
" no-break space: \\\\00a0\n" +
" newline: \\\\n\n" +
" tab: \\\\t ('\t')\n" +
" \"\"\";\n" +
"}",
m5Text);
}

}
8 changes: 8 additions & 0 deletions src/test/resources/textBlock/TextBlockTestClass.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,12 @@ void m4() {
String empty = """
""";
}

void m5() {
String escape = """
no-break space: \\00a0
newline: \\n
tab: \\t ('\t')
""";
}
}