From c101ee96bd281c3d61e46d795a0fbecf19772694 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Wed, 2 Oct 2024 07:52:59 -0700 Subject: [PATCH] Fix column numbers in diagnostics Despite documentation to the contrary, the column numbers are already 1-indexed. The comment was added in unknown commit when g-j-f was still implemented using ecj instead of javac, so maybe it was true then? PiperOrigin-RevId: 681451766 --- .../googlejavaformat/FormatterDiagnostic.java | 6 +-- .../java/FormatterException.java | 2 +- .../googlejavaformat/java/DiagnosticTest.java | 14 +++--- .../googlejavaformat/java/MainTest.java | 44 ++++++++++++++++--- 4 files changed, 48 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/com/google/googlejavaformat/FormatterDiagnostic.java b/core/src/main/java/com/google/googlejavaformat/FormatterDiagnostic.java index be7f8a6ca..252da5bdf 100644 --- a/core/src/main/java/com/google/googlejavaformat/FormatterDiagnostic.java +++ b/core/src/main/java/com/google/googlejavaformat/FormatterDiagnostic.java @@ -49,7 +49,7 @@ public int line() { } /** - * Returns the 0-indexed column number on which the error occurred, or {@code -1} if the error + * Returns the 1-indexed column number on which the error occurred, or {@code -1} if the error * does not have a column. */ public int column() { @@ -61,14 +61,14 @@ public String message() { return message; } + @Override public String toString() { StringBuilder sb = new StringBuilder(); if (lineNumber >= 0) { sb.append(lineNumber).append(':'); } if (column >= 0) { - // internal column numbers are 0-based, but diagnostics use 1-based indexing by convention - sb.append(column + 1).append(':'); + sb.append(column).append(':'); } if (lineNumber >= 0 || column >= 0) { sb.append(' '); diff --git a/core/src/main/java/com/google/googlejavaformat/java/FormatterException.java b/core/src/main/java/com/google/googlejavaformat/java/FormatterException.java index 0eb06461b..5ca939bbb 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/FormatterException.java +++ b/core/src/main/java/com/google/googlejavaformat/java/FormatterException.java @@ -69,7 +69,7 @@ public String formatDiagnostics(String path, String input) { if (line != -1 && column != -1) { sb.append(CharMatcher.breakingWhitespace().trimTrailingFrom(lines.get(line - 1))) .append(System.lineSeparator()); - sb.append(" ".repeat(column)).append('^').append(System.lineSeparator()); + sb.append(" ".repeat(column - 1)).append('^').append(System.lineSeparator()); } } return sb.toString(); diff --git a/core/src/test/java/com/google/googlejavaformat/java/DiagnosticTest.java b/core/src/test/java/com/google/googlejavaformat/java/DiagnosticTest.java index fc966fac3..e05a37264 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/DiagnosticTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/DiagnosticTest.java @@ -79,7 +79,7 @@ public void parseError() throws Exception { int result = main.format(path.toString()); assertThat(stdout.toString()).isEmpty(); - assertThat(stderr.toString()).contains("InvalidSyntax.java:2:29: error: expected"); + assertThat(stderr.toString()).contains("InvalidSyntax.java:2:28: error: expected"); assertThat(result).isEqualTo(1); } @@ -119,7 +119,7 @@ public void oneFileParseError() throws Exception { int result = main.format(pathOne.toString(), pathTwo.toString()); assertThat(stdout.toString()).isEqualTo(two); - assertThat(stderr.toString()).contains("One.java:1:13: error: reached end of file"); + assertThat(stderr.toString()).contains("One.java:1:12: error: reached end of file"); assertThat(result).isEqualTo(1); } @@ -141,7 +141,7 @@ public void oneFileParseErrorReplace() throws Exception { int result = main.format("-i", pathOne.toString(), pathTwo.toString()); assertThat(stdout.toString()).isEmpty(); - assertThat(stderr.toString()).contains("One.java:1:14: error: class, interface"); + assertThat(stderr.toString()).contains("One.java:1:13: error: class, interface"); assertThat(result).isEqualTo(1); // don't edit files with parse errors assertThat(Files.readAllLines(pathOne, UTF_8)).containsExactly("class One {}}"); @@ -164,7 +164,7 @@ public void parseError2() throws FormatterException, IOException, UsageException int exitCode = main.format(args); assertThat(exitCode).isEqualTo(1); - assertThat(err.toString()).contains("A.java:2:6: error: ';' expected"); + assertThat(err.toString()).contains("A.java:2:5: error: ';' expected"); } @Test @@ -179,7 +179,7 @@ public void parseErrorStdin() throws FormatterException, IOException, UsageExcep int exitCode = main.format(args); assertThat(exitCode).isEqualTo(1); - assertThat(err.toString()).contains(":2:6: error: ';' expected"); + assertThat(err.toString()).contains(":2:5: error: ';' expected"); } @Test @@ -198,7 +198,7 @@ public void lexError2() throws FormatterException, IOException, UsageException { int exitCode = main.format(args); assertThat(exitCode).isEqualTo(1); - assertThat(err.toString()).contains("A.java:2:5: error: unclosed character literal"); + assertThat(err.toString()).contains("A.java:2:4: error: unclosed character literal"); } @Test @@ -212,6 +212,6 @@ public void lexErrorStdin() throws FormatterException, IOException, UsageExcepti int exitCode = main.format(args); assertThat(exitCode).isEqualTo(1); - assertThat(err.toString()).contains(":2:5: error: unclosed character literal"); + assertThat(err.toString()).contains(":2:4: error: unclosed character literal"); } } diff --git a/core/src/test/java/com/google/googlejavaformat/java/MainTest.java b/core/src/test/java/com/google/googlejavaformat/java/MainTest.java index d7de40510..2d9364082 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/MainTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/MainTest.java @@ -307,7 +307,7 @@ public void importRemoveErrorParseError() throws Exception { new PrintWriter(err, true), new ByteArrayInputStream(joiner.join(input).getBytes(UTF_8))); assertThat(main.format("-")).isEqualTo(1); - assertThat(err.toString()).contains(":4:3: error: class, interface"); + assertThat(err.toString()).contains(":4:2: error: class, interface"); } finally { Locale.setDefault(backupLocale); @@ -508,7 +508,7 @@ public void assumeFilename_error() throws Exception { new PrintWriter(err, true), new ByteArrayInputStream(joiner.join(input).getBytes(UTF_8))); assertThat(main.format("--assume-filename=Foo.java", "-")).isEqualTo(1); - assertThat(err.toString()).contains("Foo.java:1:15: error: class, interface"); + assertThat(err.toString()).contains("Foo.java:1:14: error: class, interface"); } @Test @@ -637,11 +637,13 @@ public void reorderModifiersOptionTest() throws Exception { } @Test - public void badIdentifier() throws Exception { + public void syntaxError() throws Exception { Path path = testFolder.newFile("Test.java").toPath(); String[] input = { "class Test {", // - " void f(int package) {}", + " void f(int package) {", + " int", + " }", "}", "", }; @@ -653,9 +655,37 @@ public void badIdentifier() throws Exception { int errorCode = main.format(path.toAbsolutePath().toString()); assertWithMessage("Error Code").that(errorCode).isEqualTo(1); String[] expected = { - path + ":2:14: error: expected", // - " void f(int package) {}", - " ^", + path + ":2:13: error: expected", + " void f(int package) {", + " ^", + path + ":3:5: error: not a statement", + " int", + " ^", + path + ":3:8: error: ';' expected", + " int", + " ^", + "", + }; + assertThat(err.toString()).isEqualTo(joiner.join(expected)); + } + + @Test + public void syntaxErrorBeginning() throws Exception { + Path path = testFolder.newFile("Test.java").toPath(); + String[] input = { + "error", // + }; + String source = joiner.join(input); + Files.writeString(path, source, UTF_8); + StringWriter out = new StringWriter(); + StringWriter err = new StringWriter(); + Main main = new Main(new PrintWriter(out, true), new PrintWriter(err, true), System.in); + int errorCode = main.format(path.toAbsolutePath().toString()); + assertWithMessage("Error Code").that(errorCode).isEqualTo(1); + String[] expected = { + path + ":1:1: error: reached end of file while parsing", // + "error", + "^", "", }; assertThat(err.toString()).isEqualTo(joiner.join(expected));