Skip to content

Commit 53ac7d8

Browse files
committed
Stop splitting long strings inside an escaped backslash
StringWrapper cuts a long string literal before whitespace and before \t, and ends a piece after \n and \r, but never asked whether the backslash of such an escape is escaped itself. In 'D:\\tempDb' the second backslash and the t looked like a tab, so a line break there left a lone backslash that escaped the closing quote, and the file failed with "unclosed string literal" (#32, from palantir#1680). \\n and \\r were misread the same way: they cut words such as C:\\new in two and forced a line break, which moved the break but still parsed. A backslash with an odd number of backslashes right before it now counts as escaped, both where pieces are cut and where a piece forces the line to end. Literals without an escaped backslash are cut as before. Of the JDK 21 and 25 sources, the same 8 files format differently: resource bundles whose messages contain \\n or \\r, which no longer break the line after them.
1 parent 39f69dc commit 53ac7d8

4 files changed

Lines changed: 95 additions & 3 deletions

File tree

‎open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -432,7 +432,7 @@ private static ImmutableList<String> stringComponents(
432432
}
433433

434434
static int hasEscapedWhitespaceAt(String input, int idx) {
435-
if (input.startsWith("\\t", idx)) {
435+
if (input.startsWith("\\t", idx) && !isEscaped(input, idx)) {
436436
return 2;
437437
}
438438
return -1;
@@ -446,7 +446,19 @@ static int hasEscapedNewlineAt(String input, int idx) {
446446
if (input.startsWith("\\n", idx)) {
447447
offset += 2;
448448
}
449-
return offset > 0 ? offset : -1;
449+
return offset > 0 && !isEscaped(input, idx) ? offset : -1;
450+
}
451+
452+
/**
453+
* Whether the character at {@code idx} is escaped by the backslashes before it. In {@code \\t} the second backslash
454+
* is, so it starts no escape sequence of its own and the {@code t} is an ordinary letter.
455+
*/
456+
private static boolean isEscaped(String input, int idx) {
457+
int backslashes = 0;
458+
while (idx - backslashes > 0 && input.charAt(idx - backslashes - 1) == '\\') {
459+
backslashes++;
460+
}
461+
return backslashes % 2 == 1;
450462
}
451463

452464
/**
@@ -486,7 +498,7 @@ private static String reflow(
486498
String text = input.removeFirst();
487499
line.add(text);
488500
length += text.length();
489-
if (text.endsWith("\\n") || text.endsWith("\\r")) {
501+
if (hasEscapedNewlineAt(text, text.length() - 2) != -1) {
490502
break;
491503
}
492504
}

‎open-java-format/src/test/java/com/palantir/javaformat/java/StringWrapperTest.java‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,23 @@
1515
package com.palantir.javaformat.java;
1616

1717
import static com.google.common.truth.Truth.assertThat;
18+
import static com.palantir.javaformat.java.JavaFormatterOptions.Style;
1819

1920
import com.google.common.base.Joiner;
21+
import com.google.common.collect.Iterables;
22+
import com.sun.source.tree.BinaryTree;
23+
import com.sun.source.tree.ClassTree;
24+
import com.sun.source.tree.CompilationUnitTree;
25+
import com.sun.source.tree.ExpressionTree;
26+
import com.sun.source.tree.LiteralTree;
27+
import com.sun.source.tree.VariableTree;
28+
import com.sun.source.util.JavacTask;
29+
import java.io.IOException;
30+
import java.net.URI;
31+
import java.util.List;
32+
import javax.tools.JavaFileObject;
33+
import javax.tools.SimpleJavaFileObject;
34+
import javax.tools.ToolProvider;
2035
import org.junit.jupiter.api.Test;
2136
import org.junit.jupiter.api.parallel.Execution;
2237
import org.junit.jupiter.api.parallel.ExecutionMode;
@@ -48,6 +63,60 @@ public void testAwkwardLineEndWrapping() throws Exception {
4863
assertThat(StringWrapper.wrap(100, input, Formatter.create())).isEqualTo(output);
4964
}
5065

66+
@Test
67+
public void wrapsAStringWithEscapedBackslashes() throws Exception {
68+
// In D:\\tempDb the second backslash and the t are not an escaped tab. Splitting there left a lone backslash
69+
// at the end of a piece, where it escaped the closing quote.
70+
String input = lines(
71+
"class C {",
72+
" void m(java.sql.Connection con) throws Exception {",
73+
" con.createStatement()",
74+
" .execute(\"ALTER DATABASE tempdb MODIFY FILE (NAME = 'tempdev',"
75+
+ " FILENAME = 'D:\\\\tempDb\\\\DATA\\\\tempdb.mdf')\");",
76+
" }",
77+
"}");
78+
Formatter formatter = Formatter.createFormatter(
79+
JavaFormatterOptions.builder().style(Style.OJF).build());
80+
81+
String output = formatter.formatSourceAndFixImports(input);
82+
83+
assertThat(formatter.formatSourceAndFixImports(output)).isEqualTo(output);
84+
}
85+
86+
@Test
87+
public void wrappingKeepsTheValueOfAStringWithEscapes() throws Exception {
88+
// At 40 columns the first line fills up right inside C:\\temp, where \\t is not an escaped tab.
89+
String input = lines("class T {", " String s = \"copy the file to C:\\\\temp and back\";", "}");
90+
91+
String output = StringWrapper.wrap(40, input, Formatter.create());
92+
93+
assertThat(output).isNotEqualTo(input);
94+
assertThat(stringValue(output)).isEqualTo(stringValue(input));
95+
}
96+
97+
/** The value javac gives the initializer of the only field: a string literal, or literals joined with +. */
98+
private static String stringValue(String source) throws IOException {
99+
JavaFileObject file = new SimpleJavaFileObject(URI.create("string:///T.java"), JavaFileObject.Kind.SOURCE) {
100+
@Override
101+
public CharSequence getCharContent(boolean ignoreEncodingErrors) {
102+
return source;
103+
}
104+
};
105+
JavacTask task =
106+
(JavacTask) ToolProvider.getSystemJavaCompiler().getTask(null, null, null, null, null, List.of(file));
107+
CompilationUnitTree unit = Iterables.getOnlyElement(task.parse());
108+
ClassTree type = (ClassTree) Iterables.getOnlyElement(unit.getTypeDecls());
109+
VariableTree field = (VariableTree) Iterables.getOnlyElement(type.getMembers());
110+
return concatenation(field.getInitializer());
111+
}
112+
113+
private static String concatenation(ExpressionTree expression) {
114+
if (expression instanceof BinaryTree plus) {
115+
return concatenation(plus.getLeftOperand()) + concatenation(plus.getRightOperand());
116+
}
117+
return (String) ((LiteralTree) expression).getValue();
118+
}
119+
51120
private static String lines(String... line) {
52121
return Joiner.on('\n').join(line) + '\n';
53122
}
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
class T {
2+
String s = "fields split on \\t, lines on \\r\\n or \\n in C:\\temp\\new\\reports and \\\\server\\share for caf\u00e9\tthen\ndone";
3+
}
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
class T {
2+
String s = "fields split on \\t,"
3+
+ " lines on \\r\\n or \\n in"
4+
+ " C:\\temp\\new\\reports and"
5+
+ " \\\\server\\share for"
6+
+ " caf\u00e9\tthen\n"
7+
+ "done";
8+
}

0 commit comments

Comments
 (0)