From c8ad5cf14c2d111943a531bb48c60965627c3403 Mon Sep 17 00:00:00 2001 From: Axel Howind Date: Fri, 29 Nov 2024 06:37:52 +0100 Subject: [PATCH 1/6] add enum value Check.THROW_IAE --- .../dua3/cabe/processor/Configuration.java | 41 ++++++++++++++++--- 1 file changed, 35 insertions(+), 6 deletions(-) diff --git a/cabe-processor/src/main/java/com/dua3/cabe/processor/Configuration.java b/cabe-processor/src/main/java/com/dua3/cabe/processor/Configuration.java index 6a8ffc6..0534b45 100644 --- a/cabe-processor/src/main/java/com/dua3/cabe/processor/Configuration.java +++ b/cabe-processor/src/main/java/com/dua3/cabe/processor/Configuration.java @@ -123,19 +123,48 @@ public enum Check { /** * Do not generate Checks. */ - NO_CHECK, + NO_CHECK(null, null), + /** * Generate standard assertions that can be controlled at runtime (i.e., g {@code -ea} and {@code -da} JVM flags). + * + *

If assertions are enabled, violations will result in throwing an {@link AssertionError}. + */ + + ASSERT("new AssertionError((Object) ", ")"), + + /** + * Throw a {@link NullPointerException} if parameter is null. + */ + THROW_NPE("new NullPointerException(",")"), + + /** + * Throw an {@link AssertionError} if parameter is null, regardless of the assertion setting. */ - ASSERT, + ASSERT_ALWAYS("new AssertionError((Object) ", ")"), + /** - * Throw a NullPointerException if parameter is null. + * Throw an {@link IllegalArgumentException} if parameter is null. */ - THROW_NPE, + THROW_IAE("new IllegalArgumentException(",")"); + + private final String prefix; + private final String suffix; + + Check(String prefix, String suffix) { + this.prefix = prefix; + this.suffix = suffix; + } + /** - * Throw an AssertionError if parameter is null, regardless of the assertion setting. + * Generate the code fragment that creates a new instance of this Check instance's throwable class. + * @param message the text to use as the message parameter of the throwable constructor. + * + * @return the code to create an instance of this Check instance's throwable */ - ASSERT_ALWAYS + public Optional getCodeForNewInstance(String message) { + return Optional.ofNullable(prefix).map(pre -> pre + message + suffix); + } } } From e5fe9b7176fa2cdda3974994bc5214ed5d72cf57 Mon Sep 17 00:00:00 2001 From: Axel Howind Date: Fri, 29 Nov 2024 07:53:04 +0100 Subject: [PATCH 2/6] refactor code generation and add support for THROW_IAE --- Writerside/v.list | 4 +- build.gradle.kts | 4 +- .../com/dua3/cabe/processor/ClassPatcher.java | 124 +++++++++--------- 3 files changed, 63 insertions(+), 69 deletions(-) diff --git a/Writerside/v.list b/Writerside/v.list index 5c5d78c..bd42acf 100644 --- a/Writerside/v.list +++ b/Writerside/v.list @@ -2,6 +2,6 @@ - - + + diff --git a/build.gradle.kts b/build.gradle.kts index 8b83653..2df6ceb 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -3,8 +3,8 @@ plugins { id("com.dorongold.task-tree") version "2.1.1" } -extra["plugin_version"] = "3.0.0" -extra["processor_version"] = "3.0.0" +extra["plugin_version"] = "3.0.1-SNAPSHOT" +extra["processor_version"] = "3.0.1-SNAPSHOT" subprojects { apply(plugin = "java") diff --git a/cabe-processor/src/main/java/com/dua3/cabe/processor/ClassPatcher.java b/cabe-processor/src/main/java/com/dua3/cabe/processor/ClassPatcher.java index 3400c6e..84f2099 100644 --- a/cabe-processor/src/main/java/com/dua3/cabe/processor/ClassPatcher.java +++ b/cabe-processor/src/main/java/com/dua3/cabe/processor/ClassPatcher.java @@ -406,8 +406,6 @@ private void instrumentMethod(ClassInfo ci, MethodInfo mi) throws ClassFileProce && mi.methodName().equals("equals") && mi.parameters().size() == 1; LOG.fine(() -> "instrumenting method " + methodName); - boolean hasStandardParameterAssertions = false; - boolean hasOtherParameterChecks = false; boolean hasStandardReturnValueAssertions = false; boolean hasOtherReturnValueChecks = false; try (Formatter standardParameterAssertions = new Formatter(); @@ -432,48 +430,24 @@ private void instrumentMethod(ClassInfo ci, MethodInfo mi) throws ClassFileProce NullnessOperator nullnessOperatorParameter = pi.nullnessOperator(); boolean isNonNull = (nullnessOperatorParameter == NullnessOperator.MINUS_NULL) || (!ignoreNonMethodNonNullAnnotation && ci.nullnessOperator().andThen(nullnessOperatorParameter) == NullnessOperator.MINUS_NULL); + if (isNonNull) { - Configuration.Check check = ci.isPublicApi() && mi.isPublic() ? configuration.publicApi() : configuration.privateApi(); - if (ci.isRecord() && check== Configuration.Check.ASSERT) { // issue: https://github.com/xzel23/cabe/issues/1 - LOG.info("cannot use assert in record " + ci.name() + " using THROW_NPE instead / https://github.com/xzel23/cabe/issues/1"); - check = Configuration.Check.THROW_NPE; - } - switch (check) { - case NO_CHECK -> { - // nop - } - case ASSERT -> { - standardParameterAssertions.format( - " if (%1$s==null) { throw new AssertionError((Object) \"%2$s is null\"); }%n", - pi.param(), parameterName - ); - hasStandardParameterAssertions = true; - } - case ASSERT_ALWAYS -> { - otherParameterChecks.format( - "if (%1$s==null) { throw new AssertionError((Object) \"%2$s is null\"); }%n", - pi.param(), parameterName - ); - hasOtherParameterChecks = true; - } - case THROW_NPE -> { - otherParameterChecks.format( - "if (%1$s==null) { throw new NullPointerException(\"%2$s is null\"); }%n", - pi.param(), parameterName - ); - hasOtherParameterChecks = true; - } - } + Configuration.Check check = getCheck(ci, mi); + check.getCodeForNewInstance("\"%2$s is null\"") + .map(createThrowableCode -> " if (%1$s==null) { throw " + createThrowableCode + "; }%n") + .ifPresent(checkCode -> { + if (check == Configuration.Check.ASSERT) { + standardParameterAssertions.format(checkCode, pi.param(), parameterName); + } else { + otherParameterChecks.format(checkCode, pi.param(), parameterName); + } + }); LOG.fine(() -> "adding null check for parameter " + parameterName + " in " + ci.name()); } } // modify class by injecting parameter checks - String codeParamChecks = (hasStandardParameterAssertions - ? "if (%1$s) {%n%2$s}%n".formatted(getAssertionEnabledExpression(ci), standardParameterAssertions) - : "") - + (hasOtherParameterChecks ? otherParameterChecks : ""); - + String codeParamChecks = getCheckCode(ci, standardParameterAssertions.toString(), otherParameterChecks.toString()); if (!codeParamChecks.isEmpty()) { LOG.fine(() -> "injecting code into: " + methodName + "\n" + codeParamChecks.indent(2).stripTrailing()); ctBehavior.insertBefore(codeParamChecks); @@ -489,37 +463,20 @@ private void instrumentMethod(ClassInfo ci, MethodInfo mi) throws ClassFileProce if (isNonNullRV) { Configuration.Check check = configuration.checkReturn(); - switch (check) { - case NO_CHECK -> { - // nop - } - case ASSERT -> { - standardReturnValueAssertions.format( - " if ($_==null) { throw new AssertionError((Object) \"invalid null return value\"); }%n" - ); - hasStandardReturnValueAssertions = true; - } - case ASSERT_ALWAYS -> { - otherReturnValueChecks.format( - " if ($_==null) { throw new AssertionError((Object) \"invalid null return value\"); }%n" - ); - hasOtherReturnValueChecks = true; - } - case THROW_NPE -> { - otherReturnValueChecks.format( - "if ($_==null) { throw new NullPointerException(\"invalid null return value\"); }%n" - ); - hasOtherReturnValueChecks = true; - } - } + check.getCodeForNewInstance("\"invalid null return value\"") + .map(createThrowableCode -> " if ($_==null) { throw " + createThrowableCode + "; }%n") + .ifPresent(checkCode -> { + if (check == Configuration.Check.ASSERT) { + standardReturnValueAssertions.format(checkCode); + } else { + otherReturnValueChecks.format(checkCode); + } + }); LOG.fine(() -> "adding null check for return value in " + ci.name()); } // modify class by injecting return value checks - String codeReturnValueChecks = (hasStandardReturnValueAssertions - ? "if (%1$s) {%n%2$s}%n".formatted(getAssertionEnabledExpression(ci), standardReturnValueAssertions) - : "") - + (hasOtherReturnValueChecks ? otherReturnValueChecks : ""); + String codeReturnValueChecks = getCheckCode(ci, standardReturnValueAssertions.toString(), otherReturnValueChecks.toString()); if (!codeReturnValueChecks.isEmpty()) { LOG.fine(() -> "injecting code into: " + methodName + "\n" + codeReturnValueChecks.indent(2).stripTrailing()); @@ -535,6 +492,43 @@ private void instrumentMethod(ClassInfo ci, MethodInfo mi) throws ClassFileProce } } + /** + * Constructs a string of code that includes checks for parameter or return value assertions. + * + * @param ci the ClassInfo object representing the class being processed + * @param standardAssertionsCode the code for standard assertions to be included in the check + * @param otherAssertionsCode additional assertion code to be included in the check + * @return a String representing the complete check code + * @throws CannotCompileException if the code compilation fails + * @throws NotFoundException if a required class or method is not found + */ + private String getCheckCode(ClassInfo ci, String standardAssertionsCode, String otherAssertionsCode) throws CannotCompileException, NotFoundException { + return (!standardAssertionsCode.isEmpty() + ? "if (%1$s) {%n%2$s}%n".formatted(getAssertionEnabledExpression(ci), standardAssertionsCode) + : "") + + otherAssertionsCode; + } + + /** + * Determines the appropriate check configuration for the given class and method. + * + *

For record constructors, standard assertions cannot be injected for technical reasons, so in that case + * {@code Check.THROW_NPE} will be returned instead of {@code Check.ASSERT}. + * + * @param ci the ClassInfo object representing the class being processed + * @param mi the MethodInfo object representing the method being processed + * @return the check configuration to be used, which could be a public API check, + * private API check, or adjusted if the class is a record + */ + private Configuration.Check getCheck(ClassInfo ci, MethodInfo mi) { + Configuration.Check check = ci.isPublicApi() && mi.isPublic() ? configuration.publicApi() : configuration.privateApi(); + if (ci.isRecord() && check == Configuration.Check.ASSERT) { // issue: https://github.com/xzel23/cabe/issues/1 + LOG.info("cannot use assert in record " + ci.name() + " using THROW_NPE instead / https://github.com/xzel23/cabe/issues/1"); + check = Configuration.Check.THROW_NPE; + } + return check; + } + /** * Retrieves a CtBehavior object representing a method or constructor from the given CtClass that matches the provided MethodInfo. * From e430132b6ca7eacfe275410767d9cf2626ff4065 Mon Sep 17 00:00:00 2001 From: Axel Howind Date: Fri, 29 Nov 2024 08:42:55 +0100 Subject: [PATCH 3/6] add test for THROW_IAE (currently fails, investigating) --- .../dua3/cabe/processor/ClassPatcherTest.java | 66 +++++++++++++++++-- 1 file changed, 59 insertions(+), 7 deletions(-) diff --git a/cabe-processor/src/test/java/com/dua3/cabe/processor/ClassPatcherTest.java b/cabe-processor/src/test/java/com/dua3/cabe/processor/ClassPatcherTest.java index dd803b8..543136e 100644 --- a/cabe-processor/src/test/java/com/dua3/cabe/processor/ClassPatcherTest.java +++ b/cabe-processor/src/test/java/com/dua3/cabe/processor/ClassPatcherTest.java @@ -206,16 +206,12 @@ void testInstrumentation(String className) { @ValueSource(strings = { "NO_CHECKS", "DEVELOPMENT", - "STANDARD" + "STANDARD", + "publicApi=THROW_IAE, privateApi=ASSERT, checkReturn=NO_CHECK" }) @Order(5) public void testConfiguration(String configName) throws IOException, ClassFileProcessingFailedException { - Configuration config = switch (configName) { - case "NO_CHECKS" -> Configuration.NO_CHECKS; - case "DEVELOPMENT" -> Configuration.DEVELOPMENT; - case "STANDARD" -> Configuration.STANDARD; - default -> throw new IllegalArgumentException("unknown configuration: " + configName); - }; + Configuration config = Configuration.parse(configName); // create directories Path unprocessedDir = testDir.resolve("classes-unprocessed-" + configName); @@ -420,6 +416,62 @@ public void testConfiguration(String configName) throws IOException, ClassFilePr publicNullableDefault: - publicNonNullDefault: java.lang.NullPointerException + """, + Configuration.parse("publicApi=THROW_IAE, privateApi=ASSERT, checkReturn=NO_CHECK"), """ + Config: Configuration[publicApi=THROW_IAE, privateApi=ASSERT, checkReturn=NO_CHECK] + =================================================================================== + Testing com/dua3/cabe/processor/test/config/TestClass.class with assertions false + --------------------------------------------------------------------------------- + assertions enabled : false + privateNullable : - + privateNonNull : - + publicNullable : - + publicNonNull : java.lang.IllegalArgumentException + + Testing com/dua3/cabe/processor/test/config/TestClass.class with assertions true + -------------------------------------------------------------------------------- + assertions enabled : true + privateNullable : - + privateNonNull : java.lang.AssertionError + publicNullable : - + publicNonNull : java.lang.IllegalArgumentException + + Testing com/dua3/cabe/processor/test/config/TestClassStdAssert.class with assertions false + ------------------------------------------------------------------------------------------ + assertions enabled : false + privateNullable : - + privateNonNull : - + publicNullable : - + publicNonNull : java.lang.IllegalArgumentException + + Testing com/dua3/cabe/processor/test/config/TestClassStdAssert.class with assertions true + ----------------------------------------------------------------------------------------- + assertions enabled : true + privateNullable : - + privateNonNull : java.lang.AssertionError + publicNullable : - + publicNonNull : java.lang.IllegalArgumentException + + Testing com/dua3/cabe/processor/test/config/TestInterface.class with assertions false + ------------------------------------------------------------------------------------- + assertions enabled : false + privateNullable : - + privateNonNull : - + publicNullable : - + publicNonNull : java.lang.IllegalArgumentException + publicNullableDefault: - + publicNonNullDefault: java.lang.IllegalArgumentException + + Testing com/dua3/cabe/processor/test/config/TestInterface.class with assertions true + ------------------------------------------------------------------------------------ + assertions enabled : true + privateNullable : - + privateNonNull : java.lang.AssertionError + publicNullable : - + publicNonNull : java.lang.IllegalArgumentException + publicNullableDefault: - + publicNonNullDefault: java.lang.IllegalArgumentException + """ ); From 3c54d72e64e9e49e357a5abad0d12dceaa6b9ff8 Mon Sep 17 00:00:00 2001 From: Axel Howind Date: Fri, 29 Nov 2024 10:02:37 +0100 Subject: [PATCH 4/6] fixed expected result for TestClassStdAssert - the class is not annotated --- .../test/java/com/dua3/cabe/processor/ClassPatcherTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cabe-processor/src/test/java/com/dua3/cabe/processor/ClassPatcherTest.java b/cabe-processor/src/test/java/com/dua3/cabe/processor/ClassPatcherTest.java index 543136e..68cb358 100644 --- a/cabe-processor/src/test/java/com/dua3/cabe/processor/ClassPatcherTest.java +++ b/cabe-processor/src/test/java/com/dua3/cabe/processor/ClassPatcherTest.java @@ -442,7 +442,7 @@ public void testConfiguration(String configName) throws IOException, ClassFilePr privateNullable : - privateNonNull : - publicNullable : - - publicNonNull : java.lang.IllegalArgumentException + publicNonNull : java.lang.NullPointerException Testing com/dua3/cabe/processor/test/config/TestClassStdAssert.class with assertions true ----------------------------------------------------------------------------------------- @@ -450,7 +450,7 @@ public void testConfiguration(String configName) throws IOException, ClassFilePr privateNullable : - privateNonNull : java.lang.AssertionError publicNullable : - - publicNonNull : java.lang.IllegalArgumentException + publicNonNull : java.lang.NullPointerException Testing com/dua3/cabe/processor/test/config/TestInterface.class with assertions false ------------------------------------------------------------------------------------- From 290a537314d8d6f377f2df7099bc89c8132e81f3 Mon Sep 17 00:00:00 2001 From: Axel Howind Date: Fri, 29 Nov 2024 10:02:48 +0100 Subject: [PATCH 5/6] remove unused import --- .../com/dua3/cabe/processor/test/config/TestClassStdAssert.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/cabe-processor/src/test/resources/testSrc/com/dua3/cabe/processor/test/config/TestClassStdAssert.java b/cabe-processor/src/test/resources/testSrc/com/dua3/cabe/processor/test/config/TestClassStdAssert.java index 0555d41..b09674e 100644 --- a/cabe-processor/src/test/resources/testSrc/com/dua3/cabe/processor/test/config/TestClassStdAssert.java +++ b/cabe-processor/src/test/resources/testSrc/com/dua3/cabe/processor/test/config/TestClassStdAssert.java @@ -1,7 +1,5 @@ package com.dua3.cabe.processor.test.config; -import org.jspecify.annotations.NonNull; - import java.util.Formatter; public class TestClassStdAssert { From 8d7c7e5cfeed2e04903606b8a479771b8c035e7c Mon Sep 17 00:00:00 2001 From: Axel Howind Date: Fri, 29 Nov 2024 11:51:40 +0100 Subject: [PATCH 6/6] update documentation --- CHANGES.md | 9 +++++++++ README.md | 2 +- Writerside/topics/cabe.md | 38 ++++++++++++++++++++------------------ 3 files changed, 30 insertions(+), 19 deletions(-) create mode 100644 CHANGES.md diff --git a/CHANGES.md b/CHANGES.md new file mode 100644 index 0000000..2c600f8 --- /dev/null +++ b/CHANGES.md @@ -0,0 +1,9 @@ +version 3.0.1 +============= + +- introduce Check.THROW_IAE to throw IllegalArgumentException instead of NullPointerException + +version 3.0.0 +============= + +- initial release with support for jspecify annotations diff --git a/README.md b/README.md index d2d323a..03d3f56 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ Usage - add the plugin in `build.gradle`: ``` plugins { - id 'com.dua3.cabe' version '3.0.0' + id 'com.dua3.cabe' version '3.0.1-SNAPSHOT' } ``` diff --git a/Writerside/topics/cabe.md b/Writerside/topics/cabe.md index e379a16..5a2e31f 100644 --- a/Writerside/topics/cabe.md +++ b/Writerside/topics/cabe.md @@ -185,12 +185,13 @@ When in doubt, profile your application when compiled using the different settin This depends on the configuration used. There are four types of checks. The examples assume a non-nullable parameter #named `p`. -| Check | Equivalent Java code | -|--------------------|-------------------------------------------------------------| -| NO_CHECK | [N/A] | -| STANDARD_ASSERTION | `assert p!=null : "p is null";` | -| ASSERT_ALWAYS | `if (p==null) throw new AssertionError("p is null");` | -| THROW_NPE | `if (p==null) throw new NullPointerException("p is null");` | +| Check | Equivalent Java code | +|--------------------|-----------------------------------------------------------------| +| NO_CHECK | [N/A] | +| STANDARD_ASSERTION | `assert p!=null : "p is null";` | +| ASSERT_ALWAYS | `if (p==null) throw new AssertionError("p is null");` | +| THROW_NPE | `if (p==null) throw new NullPointerException("p is null");` | +| THROW_IAE | `if (p==null) throw new IllegalArgumentException("p is null");` | ## Public vs Private API @@ -415,18 +416,19 @@ When using a configuration String, you can use either Examples: -| Configuration String | Public API | Private API | Return Value | -|------------------------------------------|---------------|---------------|----------------| -| "STANDARD" | THROW_NPE | ASSERT | NO_CHECK | -| "DEVELOPMENT" | ASSERT_ALWAYS | ASSERT_ALWAYS | ASSERT_ALWAYS | -| "NO_CHECKS" | NO_CHECK | NO_CHECK | NO_CHECK | -| "THROW_NPE" | THROW_NPE | THROW_NPE | THROW_NPE | -| "ASSERT" | ASSERT | ASSERT | ASSERT | -| "ASSERT_ALWAYS" | ASSERT_ALWAYS | ASSERT_ALWAYS | ASSERT_ALWAYS | -| "NO_CHECK" | NO_CHECK | NO_CHECK | NO_CHECK | -| "THROW_NPE" | THROW_NPE | THROW_NPE | THROW_NPE | -| "publicApi=THROW_NPE" | THROW_NPE | NO_CHECK | NO_CHECK | -| "publicApi=THROW_NPE:returnValue=ASSERT" | THROW_NPE | NO_CHECK | ASSERT | +| Configuration String | Public API | Private API | Return Value | +|------------------------------------------|---------------|---------------|---------------| +| "STANDARD" | THROW_NPE | ASSERT | NO_CHECK | +| "DEVELOPMENT" | ASSERT_ALWAYS | ASSERT_ALWAYS | ASSERT_ALWAYS | +| "NO_CHECKS" | NO_CHECK | NO_CHECK | NO_CHECK | +| "THROW_NPE" | THROW_NPE | THROW_NPE | THROW_NPE | +| "ASSERT" | ASSERT | ASSERT | ASSERT | +| "ASSERT_ALWAYS" | ASSERT_ALWAYS | ASSERT_ALWAYS | ASSERT_ALWAYS | +| "NO_CHECK" | NO_CHECK | NO_CHECK | NO_CHECK | +| "THROW_NPE" | THROW_NPE | THROW_NPE | THROW_NPE | +| "publicApi=THROW_NPE" | THROW_NPE | NO_CHECK | NO_CHECK | +| "publicApi=THROW_NPE:returnValue=ASSERT" | THROW_NPE | NO_CHECK | ASSERT | +| "publicApi=THROW_IAE:privateApi=ASSERT" | THROW_IAE | ASSERT | NO_CHECK | You can also use the standard record constructor of Configuration