From 5dc419ba56666553ec0a336ba910f00d39b66879 Mon Sep 17 00:00:00 2001 From: Elkhan Eminov Date: Sun, 14 Jan 2024 22:46:34 +0100 Subject: [PATCH 1/3] Throw on redundant options --- .../com/google/testing/compile/Compiler.java | 66 +++++- .../testing/compile/CompilationTest.java | 133 ------------ .../compile/EnclosedCompilationTest.java | 190 ++++++++++++++++++ 3 files changed, 248 insertions(+), 141 deletions(-) delete mode 100644 src/test/java/com/google/testing/compile/CompilationTest.java create mode 100644 src/test/java/com/google/testing/compile/EnclosedCompilationTest.java diff --git a/src/main/java/com/google/testing/compile/Compiler.java b/src/main/java/com/google/testing/compile/Compiler.java index f77d7e56..c716a5b2 100644 --- a/src/main/java/com/google/testing/compile/Compiler.java +++ b/src/main/java/com/google/testing/compile/Compiler.java @@ -35,9 +35,11 @@ import java.net.URL; import java.net.URLClassLoader; import java.util.LinkedHashSet; +import java.util.List; import java.util.Locale; import java.util.Optional; import java.util.Set; +import java.util.stream.Collectors; import javax.annotation.processing.Processor; import javax.tools.DiagnosticCollector; import javax.tools.JavaCompiler; @@ -53,6 +55,17 @@ @SuppressWarnings("JavaLangClash") public abstract class Compiler { + private static final Set REDUNDANT_OPTIONS = ImmutableSet.of( + "--add-exports", + "--add-opens", + "--add-reads" + ); + + @VisibleForTesting + static final String REDUNDANT_OPTIONS_ERROR_TEMPLATE = + "Following options were passed to the compiler: %s." + + "These options will have no effect and should instead be passed as VM options."; + /** Returns the {@code javac} compiler. */ public static Compiler javac() { return compiler(getSystemJavaCompiler()); @@ -61,7 +74,8 @@ public static Compiler javac() { /** Returns a {@link Compiler} that uses a given {@link JavaCompiler} instance. */ public static Compiler compiler(JavaCompiler javaCompiler) { return new AutoValue_Compiler( - javaCompiler, ImmutableList.of(), ImmutableList.of(), Optional.empty(), Optional.empty()); + javaCompiler, ImmutableList.of(), ImmutableList.of(), Optional.empty(), Optional.empty(), + false); } abstract JavaCompiler javaCompiler(); @@ -80,6 +94,12 @@ public static Compiler compiler(JavaCompiler javaCompiler) { */ public abstract Optional> annotationProcessorPath(); + /** + * Whether to ignore redundant options check. + * @see #REDUNDANT_OPTIONS + */ + public abstract boolean shouldIgnoreRedundantOptionsCheck(); + /** * Uses annotation processors during compilation. These replace any previously specified. * @@ -100,7 +120,11 @@ public final Compiler withProcessors(Processor... processors) { */ public final Compiler withProcessors(Iterable processors) { return copy( - ImmutableList.copyOf(processors), options(), classPath(), annotationProcessorPath()); + ImmutableList.copyOf(processors), + options(), + classPath(), + annotationProcessorPath(), + false); } /** @@ -122,7 +146,8 @@ public final Compiler withOptions(Iterable options) { processors(), FluentIterable.from(options).transform(toStringFunction()).toList(), classPath(), - annotationProcessorPath()); + annotationProcessorPath(), + false); } /** @@ -141,7 +166,8 @@ public final Compiler withClasspathFrom(ClassLoader classloader) { processors(), options(), Optional.of(getClasspathFromClassloader(classloader)), - annotationProcessorPath()); + annotationProcessorPath(), + false); } /** Uses the given classpath for the compilation instead of the system classpath. */ @@ -150,7 +176,8 @@ public final Compiler withClasspath(Iterable classPath) { processors(), options(), Optional.of(ImmutableList.copyOf(classPath)), - annotationProcessorPath()); + annotationProcessorPath(), + false); } /** @@ -162,7 +189,17 @@ public final Compiler withAnnotationProcessorPath(Iterable annotationProce processors(), options(), classPath(), - Optional.of(ImmutableList.copyOf(annotationProcessorPath))); + Optional.of(ImmutableList.copyOf(annotationProcessorPath)), + false); + } + + public final Compiler ignoreRedundantOptionsCheck() { + return copy( + processors(), + options(), + classPath(), + annotationProcessorPath(), + true); } /** @@ -180,6 +217,11 @@ public final Compilation compile(JavaFileObject... files) { * @return the results of the compilation */ public final Compilation compile(Iterable files) { + final List redundantOptions = getRedundantOptions(); + if (!shouldIgnoreRedundantOptionsCheck() && !redundantOptions.isEmpty()) { + throw new IllegalArgumentException( + String.format(REDUNDANT_OPTIONS_ERROR_TEMPLATE, redundantOptions)); + } DiagnosticCollector diagnosticCollector = new DiagnosticCollector<>(); try (StandardJavaFileManager standardFileManager = standardFileManager(diagnosticCollector); InMemoryJavaFileManager fileManager = new InMemoryJavaFileManager(standardFileManager)) { @@ -295,8 +337,16 @@ private Compiler copy( ImmutableList processors, ImmutableList options, Optional> classPath, - Optional> annotationProcessorPath) { + Optional> annotationProcessorPath, + boolean shouldIgnoreRedundantOptionsCheck) { return new AutoValue_Compiler( - javaCompiler(), processors, options, classPath, annotationProcessorPath); + javaCompiler(), processors, options, classPath, annotationProcessorPath, + shouldIgnoreRedundantOptionsCheck); + } + + private List getRedundantOptions() { + return options().stream() + .filter(option -> REDUNDANT_OPTIONS.stream().anyMatch(option::startsWith)) + .collect(Collectors.toList()); } } diff --git a/src/test/java/com/google/testing/compile/CompilationTest.java b/src/test/java/com/google/testing/compile/CompilationTest.java deleted file mode 100644 index 51f830ce..00000000 --- a/src/test/java/com/google/testing/compile/CompilationTest.java +++ /dev/null @@ -1,133 +0,0 @@ -/* - * Copyright (C) 2016 Google, Inc. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.google.testing.compile; - -import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth8.assertThat; -import static com.google.testing.compile.CompilationSubject.assertThat; -import static com.google.testing.compile.Compiler.javac; -import static javax.tools.StandardLocation.SOURCE_OUTPUT; -import static org.junit.Assert.fail; - -import com.google.common.collect.ImmutableList; -import javax.tools.JavaFileObject; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -@RunWith(JUnit4.class) -public class CompilationTest { - - private static final JavaFileObject source1 = - JavaFileObjects.forSourceLines( - "test.Source1", // format one per line - "package test;", - "", - "class Source1 {}"); - - private static final JavaFileObject source2 = - JavaFileObjects.forSourceLines( - "test.Source2", // format one per line - "package test;", - "", - "interface Source2 {}"); - - private static final JavaFileObject brokenSource = - JavaFileObjects.forSourceLines( - "test.BrokenSource", // format one per line - "package test;", - "", - "interface BrokenSource { what is this }"); - - @Test - public void compiler() { - Compiler compiler = compilerWithGenerator(); - Compilation compilation = compiler.compile(source1, source2); - assertThat(compilation.compiler()).isEqualTo(compiler); - assertThat(compilation.sourceFiles()).containsExactly(source1, source2).inOrder(); - assertThat(compilation.status()).isEqualTo(Compilation.Status.SUCCESS); - } - - @Test - public void compilerStatusFailure() { - Compiler compiler = compilerWithGenerator(); - Compilation compilation = compiler.compile(brokenSource); - assertThat(compilation.status()).isEqualTo(Compilation.Status.FAILURE); - assertThat(compilation.errors()).hasSize(1); - assertThat(compilation.errors().get(0).getLineNumber()).isEqualTo(3); - } - - @Test - public void generatedFilePath() { - Compiler compiler = compilerWithGenerator(); - Compilation compilation = compiler.compile(source1, source2); - assertThat(compilation.generatedFile(SOURCE_OUTPUT, "test/generated/Blah.java")).isPresent(); - } - - @Test - public void generatedFilePackage() { - Compiler compiler = compilerWithGenerator(); - Compilation compilation = compiler.compile(source1, source2); - assertThat(compilation.generatedFile(SOURCE_OUTPUT, "test.generated", "Blah.java")).isPresent(); - } - - @Test - public void generatedSourceFile() { - Compiler compiler = compilerWithGenerator(); - Compilation compilation = compiler.compile(source1, source2); - assertThat(compilation.generatedSourceFile("test.generated.Blah")).isPresent(); - } - - private static Compiler compilerWithGenerator() { - return javac().withProcessors(new GeneratingProcessor("test.generated")); - } - - @Test - public void generatedFiles_unsuccessfulCompilationThrows() { - Compilation compilation = - javac() - .compile( - JavaFileObjects.forSourceLines( - "test.Bad", "package test;", "", "this doesn't compile!")); - assertThat(compilation).failed(); - try { - ImmutableList unused = compilation.generatedFiles(); - fail(); - } catch (IllegalStateException expected) { - } - } - - @Test - public void describeFailureDiagnostics_includesWarnings_whenCompilingWerror() { - // Arrange - Compiler compiler = javac().withOptions("-Xlint:cast", "-Werror"); - JavaFileObject source = - JavaFileObjects.forSourceLines( - "test.CastWarning", // - "package test;", // - "class CastWarning {", // - " int i = (int) 0;", // - "}"); - - // Act - Compilation compilation = compiler.compile(source); - - // Assert - assertThat(compilation).failed(); - assertThat(compilation.describeFailureDiagnostics()).contains("[cast] redundant cast to int"); - } -} diff --git a/src/test/java/com/google/testing/compile/EnclosedCompilationTest.java b/src/test/java/com/google/testing/compile/EnclosedCompilationTest.java new file mode 100644 index 00000000..d5cf07f7 --- /dev/null +++ b/src/test/java/com/google/testing/compile/EnclosedCompilationTest.java @@ -0,0 +1,190 @@ +/* + * Copyright (C) 2016 Google, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.testing.compile; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth8.assertThat; +import static com.google.testing.compile.CompilationSubject.assertThat; +import static com.google.testing.compile.Compiler.REDUNDANT_OPTIONS_ERROR_TEMPLATE; +import static com.google.testing.compile.Compiler.javac; +import static javax.tools.StandardLocation.SOURCE_OUTPUT; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.fail; + +import com.google.common.collect.ImmutableList; +import com.google.common.truth.ExpectFailure; +import javax.tools.JavaFileObject; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.runners.Enclosed; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; + +@RunWith(Enclosed.class) +public class EnclosedCompilationTest { + + private static final JavaFileObject source1 = + JavaFileObjects.forSourceLines( + "test.Source1", // format one per line + "package test;", + "", + "class Source1 {}"); + + private static final JavaFileObject source2 = + JavaFileObjects.forSourceLines( + "test.Source2", // format one per line + "package test;", + "", + "interface Source2 {}"); + + private static final JavaFileObject brokenSource = + JavaFileObjects.forSourceLines( + "test.BrokenSource", // format one per line + "package test;", + "", + "interface BrokenSource { what is this }"); + + @RunWith(JUnit4.class) + public static class CompilationTest { + + private static Compiler compilerWithGenerator() { + return javac().withProcessors(new GeneratingProcessor("test.generated")); + } + + @Test + public void compiler() { + Compiler compiler = compilerWithGenerator(); + Compilation compilation = compiler.compile(source1, source2); + assertThat(compilation.compiler()).isEqualTo(compiler); + assertThat(compilation.sourceFiles()).containsExactly(source1, source2).inOrder(); + assertThat(compilation.status()).isEqualTo(Compilation.Status.SUCCESS); + } + + @Test + public void compilerStatusFailure() { + Compiler compiler = compilerWithGenerator(); + Compilation compilation = compiler.compile(brokenSource); + assertThat(compilation.status()).isEqualTo(Compilation.Status.FAILURE); + assertThat(compilation.errors()).hasSize(1); + assertThat(compilation.errors().get(0).getLineNumber()).isEqualTo(3); + } + + @Test + public void generatedFilePath() { + Compiler compiler = compilerWithGenerator(); + Compilation compilation = compiler.compile(source1, source2); + assertThat(compilation.generatedFile(SOURCE_OUTPUT, "test/generated/Blah.java")).isPresent(); + } + + @Test + public void generatedFilePackage() { + Compiler compiler = compilerWithGenerator(); + Compilation compilation = compiler.compile(source1, source2); + assertThat( + compilation.generatedFile(SOURCE_OUTPUT, "test.generated", "Blah.java")).isPresent(); + } + + @Test + public void generatedSourceFile() { + Compiler compiler = compilerWithGenerator(); + Compilation compilation = compiler.compile(source1, source2); + assertThat(compilation.generatedSourceFile("test.generated.Blah")).isPresent(); + } + + @Test + public void generatedFiles_unsuccessfulCompilationThrows() { + Compilation compilation = + javac() + .compile( + JavaFileObjects.forSourceLines( + "test.Bad", "package test;", "", "this doesn't compile!")); + assertThat(compilation).failed(); + try { + ImmutableList unused = compilation.generatedFiles(); + fail(); + } catch (IllegalStateException expected) { + } + } + + @Test + public void describeFailureDiagnostics_includesWarnings_whenCompilingWerror() { + // Arrange + Compiler compiler = javac().withOptions("-Xlint:cast", "-Werror"); + JavaFileObject source = + JavaFileObjects.forSourceLines( + "test.CastWarning", // + "package test;", // + "class CastWarning {", // + " int i = (int) 0;", // + "}"); + + // Act + Compilation compilation = compiler.compile(source); + + // Assert + assertThat(compilation).failed(); + assertThat(compilation.describeFailureDiagnostics()).contains("[cast] redundant cast to int"); + } + } + + @RunWith(Parameterized.class) + public static class RedundantOptionsTest { + + @Rule + public final ExpectFailure expectFailure = new ExpectFailure(); + + @Parameter + public ImmutableList redundantOptions; + + @Parameterized.Parameters + public static Iterable parameters() { + return ImmutableList.of( + new Object[]{ImmutableList.of("--add-exports=val/val=val")}, + new Object[]{ImmutableList.of("--add-reads=val/val=val")}, + new Object[]{ImmutableList.of("--add-opens=val/val=val")}, + new Object[]{ImmutableList.of("--add-opens=val/val=val")}, + new Object[]{ImmutableList.of( + "--add-opens=val/val=val", + "--add-exports=val/val=val", + "--add-reads=val/val=val")} + ); + } + + @Test + public void failsOnRedundantOptions() { + ImmutableList options = ImmutableList.builder() + .add("-Xlint:all") + .addAll(redundantOptions) + .build(); + Compiler compiler = javac().withOptions(options); + IllegalArgumentException exception = + assertThrows(IllegalArgumentException.class, () -> compiler.compile(source1, source2)); + assertThat(exception).hasMessageThat() + .contains(String.format(REDUNDANT_OPTIONS_ERROR_TEMPLATE, redundantOptions)); + } + + @Test + public void respectsIgnoreRedundantOptionsCheckFlag() { + Compiler compiler = javac().withOptions(redundantOptions).ignoreRedundantOptionsCheck(); + Compilation compilation = compiler.compile(source1, source2); + assertThat(compilation).succeeded(); + } + } + +} From dba8e6b666a192275692326fa700c43241f522b8 Mon Sep 17 00:00:00 2001 From: Elkhan Eminov Date: Fri, 19 Jan 2024 21:42:59 +0100 Subject: [PATCH 2/3] Remove unused `@Rule` --- .../com/google/testing/compile/EnclosedCompilationTest.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/test/java/com/google/testing/compile/EnclosedCompilationTest.java b/src/test/java/com/google/testing/compile/EnclosedCompilationTest.java index d5cf07f7..56cb1a1e 100644 --- a/src/test/java/com/google/testing/compile/EnclosedCompilationTest.java +++ b/src/test/java/com/google/testing/compile/EnclosedCompilationTest.java @@ -146,9 +146,6 @@ public void describeFailureDiagnostics_includesWarnings_whenCompilingWerror() { @RunWith(Parameterized.class) public static class RedundantOptionsTest { - @Rule - public final ExpectFailure expectFailure = new ExpectFailure(); - @Parameter public ImmutableList redundantOptions; From 7a8b0272b05861491e864754e2b8b441c43dc77e Mon Sep 17 00:00:00 2001 From: Elkhan Eminov Date: Fri, 26 Jan 2024 22:06:02 +0100 Subject: [PATCH 3/3] Remove unused imports --- .../com/google/testing/compile/EnclosedCompilationTest.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/test/java/com/google/testing/compile/EnclosedCompilationTest.java b/src/test/java/com/google/testing/compile/EnclosedCompilationTest.java index 56cb1a1e..7b36a5ab 100644 --- a/src/test/java/com/google/testing/compile/EnclosedCompilationTest.java +++ b/src/test/java/com/google/testing/compile/EnclosedCompilationTest.java @@ -26,9 +26,7 @@ import static org.junit.Assert.fail; import com.google.common.collect.ImmutableList; -import com.google.common.truth.ExpectFailure; import javax.tools.JavaFileObject; -import org.junit.Rule; import org.junit.Test; import org.junit.experimental.runners.Enclosed; import org.junit.runner.RunWith;