diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/CommandLineOptionsParser.java b/open-java-format/src/main/java/com/palantir/javaformat/java/CommandLineOptionsParser.java index d7c595227..638ed34b1 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/CommandLineOptionsParser.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/CommandLineOptionsParser.java @@ -14,9 +14,7 @@ package com.palantir.javaformat.java; -import static java.nio.charset.StandardCharsets.UTF_8; - -import com.google.common.base.CharMatcher; +import com.google.common.base.Preconditions; import com.google.common.base.Splitter; import com.google.common.collect.ImmutableRangeSet; import com.google.common.collect.Range; @@ -24,10 +22,13 @@ import java.io.UncheckedIOException; import java.nio.file.Files; import java.nio.file.Path; -import java.nio.file.Paths; +import java.util.ArrayDeque; import java.util.ArrayList; +import java.util.Deque; import java.util.Iterator; import java.util.List; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import javax.annotation.Nullable; /** A parser for {@link CommandLineOptions}. */ @@ -35,15 +36,29 @@ final class CommandLineOptionsParser { private static final Splitter COMMA_SPLITTER = Splitter.on(','); private static final Splitter COLON_SPLITTER = Splitter.on(':'); - private static final Splitter ARG_SPLITTER = - Splitter.on(CharMatcher.breakingWhitespace()).omitEmptyStrings().trimResults(); + + /** + * Splits the arguments of a parameter file on whitespace (including tabs and line breaks), and lets an argument be + * quoted so that it keeps the whitespace inside it unchanged. + * + *

The regex matches either a quoted string (single or double quotes are allowed) or a plain unquoted string. + * Double quotes may appear inside a single-quoted string and vice versa, and are then kept as they are. For + * simplicity, escaped quotes are not handled. + */ + private static final Pattern ARG_MATCHER = Pattern.compile( + "\"([^\"]*)(?:\"|$)" // group 1: string in double quotes (or until EOF), with whitespace allowed + + "|" // OR + + "'([^']*)(?:'|$)" // group 2: string in single quotes (or until EOF), with whitespace allowed + + "|" // OR + + "([^\\s\"']+)" // group 3: unquoted string, without whitespace and without any quotes + ); /** Parses {@link CommandLineOptions}. */ @SuppressWarnings("for-rollout:NullAway") static CommandLineOptions parse(Iterable options) { CommandLineOptions.Builder optionsBuilder = CommandLineOptions.builder(); List expandedOptions = new ArrayList<>(); - expandParamsFiles(options, expandedOptions); + expandParamsFiles(options, expandedOptions, new ArrayDeque<>()); Iterator it = expandedOptions.iterator(); while (it.hasNext()) { String option = it.next(); @@ -226,7 +241,7 @@ private static Range parseRange(String arg) { * Pre-processes an argument list, expanding arguments of the form {@code @filename} by reading the content of the * file and appending whitespace-delimited options to {@code arguments}. */ - private static void expandParamsFiles(Iterable args, List expanded) { + private static void expandParamsFiles(Iterable args, List expanded, Deque paramFilesStack) { for (String arg : args) { if (arg.isEmpty()) { continue; @@ -236,14 +251,35 @@ private static void expandParamsFiles(Iterable args, List expand } else if (arg.startsWith("@@")) { expanded.add(arg.substring(1)); } else { - Path path = Paths.get(arg.substring(1)); - try { - String sequence = new String(Files.readAllBytes(path), UTF_8); - expandParamsFiles(ARG_SPLITTER.split(sequence), expanded); - } catch (IOException e) { - throw new UncheckedIOException(path + ": could not read file: " + e.getMessage(), e); + String filename = arg.substring(1); + if (paramFilesStack.contains(filename)) { + throw new IllegalArgumentException("parameter file was included recursively: " + filename); + } + paramFilesStack.push(filename); + expandParamsFiles(getParamsFromFile(filename), expanded, paramFilesStack); + String finishedFilename = paramFilesStack.pop(); + Preconditions.checkState(filename.equals(finishedFilename)); + } + } + } + + /** Reads the parameters from a file, keeping quoted parameters whole. */ + private static List getParamsFromFile(String filename) { + String fileContent; + try { + fileContent = Files.readString(Path.of(filename)); + } catch (IOException e) { + throw new UncheckedIOException(filename + ": could not read file: " + e.getMessage(), e); + } + List paramsFromFile = new ArrayList<>(); + Matcher m = ARG_MATCHER.matcher(fileContent); + while (m.find()) { + for (int i = 1; i <= m.groupCount(); i++) { + if (m.group(i) != null) { // only one group matches: double quotes, single quotes or unquoted string. + paramsFromFile.add(m.group(i)); } } } + return paramsFromFile; } } diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/Main.java b/open-java-format/src/main/java/com/palantir/javaformat/java/Main.java index a05188668..5551b325d 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/Main.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/Main.java @@ -120,11 +120,18 @@ public int format(String... args) throws UsageException { } } - @SuppressWarnings("for-rollout:RedundantControlFlow") private int formatFiles(CommandLineOptions parameters, JavaFormatterOptions options) { int numThreads = Math.min(MAX_THREADS, parameters.files().size()); - ExecutorService executorService = Executors.newFixedThreadPool(numThreads); + // Closing the pool ends its threads, so that a tool that runs Main in-process does not keep them. The close + // waits for the submitted tasks, which formatFiles has already waited for. + try (ExecutorService executorService = Executors.newFixedThreadPool(numThreads)) { + return formatFiles(parameters, options, executorService); + } + } + @SuppressWarnings("for-rollout:RedundantControlFlow") + private int formatFiles( + CommandLineOptions parameters, JavaFormatterOptions options, ExecutorService executorService) { Map inputs = new LinkedHashMap<>(); Map> results = new LinkedHashMap<>(); boolean allOk = true; @@ -273,7 +280,7 @@ public static CommandLineOptions processArgs(String... args) throws UsageExcepti throw new UsageException("--assume-filename is only supported when formatting standard input"); } if (parameters.dryRun() && parameters.inPlace()) { - throw new UsageException("cannot use --dry-run and --in-place at the same time"); + throw new UsageException("cannot use --dry-run and --replace at the same time"); } return parameters; } diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineFlagsTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineFlagsTest.java index 7ce5ee956..2dea6c882 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineFlagsTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineFlagsTest.java @@ -102,11 +102,18 @@ public void inPlaceStdin() { @Test public void inPlaceDryRun() { + try { + Main.processArgs("--replace", "--dry-run", "A.java"); + fail("fail"); + } catch (UsageException e) { + assertThat(e).hasMessageThat().contains("cannot use --dry-run and --replace at the same time"); + } + try { Main.processArgs("-i", "-n", "A.java"); fail("fail"); } catch (UsageException e) { - assertThat(e).hasMessageThat().contains("cannot use --dry-run and --in-place at the same time"); + assertThat(e).hasMessageThat().contains("cannot use --dry-run and --replace at the same time"); } } diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineOptionsParserTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineOptionsParserTest.java index 344437e94..ced15a57c 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineOptionsParserTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineOptionsParserTest.java @@ -16,6 +16,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assertions.fail; import com.google.common.collect.Range; @@ -176,6 +177,61 @@ public void paramsFile() throws IOException { assertThat(options.files()).containsExactly("L", "M", "ℕ", "@O", "P", "Q"); } + @Test + public void paramsFileWithNesting() throws IOException { + Path outer = Files.createFile(testFolder.resolve("outer")); + Path exit = Files.createFile(testFolder.resolve("exit")); + Path nested1 = Files.createFile(testFolder.resolve("nested1")); + Path nested2 = Files.createFile(testFolder.resolve("nested2")); + Path nested3 = Files.createFile(testFolder.resolve("nested3")); + + String[] args = {"--dry-run", "@" + exit, "L", "@" + outer, "U"}; + + Files.write(exit, "--set-exit-if-changed".getBytes(UTF_8)); + Files.write(outer, ("M\n@" + nested1.toAbsolutePath() + "\nT").getBytes(UTF_8)); + Files.write(nested1, ("ℕ\n@" + nested2.toAbsolutePath() + "\nS").getBytes(UTF_8)); + Files.write(nested2, ("O\n@" + nested3.toAbsolutePath() + "\nR").getBytes(UTF_8)); + Files.write(nested3, "P\n\n \n@@Q\n".getBytes(UTF_8)); + + CommandLineOptions options = CommandLineOptionsParser.parse(Arrays.asList(args)); + assertThat(options.files()).containsExactly("L", "M", "ℕ", "O", "P", "@Q", "R", "S", "T", "U"); + } + + @Test + public void paramsFileWithRecursion() throws IOException { + Path outer = Files.createFile(testFolder.resolve("outer")); + Path exit = Files.createFile(testFolder.resolve("exit")); + Path nested1 = Files.createFile(testFolder.resolve("nested1")); + Path nested2 = Files.createFile(testFolder.resolve("nested2")); + + String[] args = {"--dry-run", "@" + exit, "L", "@" + outer, "U"}; + + Files.write(exit, "--set-exit-if-changed".getBytes(UTF_8)); + Files.write(outer, ("M\n@" + nested1.toAbsolutePath() + "\nT").getBytes(UTF_8)); + Files.write(nested1, ("ℕ\n@" + nested2.toAbsolutePath() + "\nS").getBytes(UTF_8)); + Files.write(nested2, ("O\n@" + nested1.toAbsolutePath() + "\nR").getBytes(UTF_8)); + + assertThatThrownBy(() -> CommandLineOptionsParser.parse(Arrays.asList(args))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageStartingWith("parameter file was included recursively: "); + } + + @Test + public void paramsFileWithQuotesAndWhitespaces() throws IOException { + Path outer = Files.createFile(testFolder.resolve("outer with whitespace")); + Path exit = Files.createFile(testFolder.resolve("exit with whitespace")); + Path nested = Files.createFile(testFolder.resolve("nested with whitespace")); + + String[] args = {"--dry-run", "@" + exit, "L +w", "@" + outer, "Q +w"}; + + Files.write(exit, "--set-exit-if-changed 'K +w".getBytes(UTF_8)); + Files.write(outer, ("\"'M' +w\"\n\"@" + nested.toAbsolutePath() + "\"\n'\"P\" +w'").getBytes(UTF_8)); + Files.write(nested, "\"ℕ +w\"\n\n \n\"@@O +w".getBytes(UTF_8)); + + CommandLineOptions options = CommandLineOptionsParser.parse(Arrays.asList(args)); + assertThat(options.files()).containsExactly("K +w", "L +w", "'M' +w", "ℕ +w", "@O +w", "\"P\" +w", "Q +w"); + } + @Test public void assumeFilename() { assertThat(CommandLineOptionsParser.parse(Arrays.asList("--assume-filename", "Foo.java")) diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/MainTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/MainTest.java index cbbccee6a..08fc5dabb 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/MainTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/MainTest.java @@ -35,6 +35,8 @@ import java.nio.file.attribute.PosixFilePermission; import java.util.EnumSet; import java.util.Locale; +import java.util.concurrent.FutureTask; +import java.util.concurrent.TimeUnit; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import org.junit.jupiter.api.parallel.Execution; @@ -91,6 +93,29 @@ public void version() throws UsageException { assertThat(err.toString()).contains("open-java-format: Version "); } + // Main used to leave its thread pool running after format returned. The command line does not notice, because it + // exits, but anything that runs Main in-process kept the idle threads (#40, from google/google-java-format#384). + @Test + public void formatLeavesNoPoolThreadRunning() throws Exception { + Path path = Files.writeString(testFolder.resolve("A.java"), "class A {}\n"); + Main main = new Main( + new PrintWriter(new StringWriter(), true), new PrintWriter(new StringWriter(), true), System.in); + // The pool's threads join the thread group of the thread that creates the pool. + ThreadGroup group = new ThreadGroup("formatLeavesNoPoolThreadRunning"); + FutureTask format = new FutureTask<>(() -> main.format(path.toString())); + new Thread(group, format).start(); + assertThat(format.get()).isEqualTo(0); + + Thread[] threads = new Thread[group.activeCount() + 16]; + int count = group.enumerate(threads); + for (int i = 0; i < count; i++) { + threads[i].join(TimeUnit.SECONDS.toMillis(10)); + assertWithMessage(threads[i].getName() + " is still running") + .that(threads[i].isAlive()) + .isFalse(); + } + } + @Test public void preserveOriginalFile() throws Exception { Path path = Files.createFile(testFolder.resolve("Test.java"));