From 165dbbd775aae5eca5d207435a375874dddbc408 Mon Sep 17 00:00:00 2001 From: Peter Gafert Date: Sun, 20 Nov 2022 23:08:51 +0700 Subject: [PATCH 1/2] add rule not to use ASM from (most) domain classes It was a conscious decision to allow the access from `JavaClassDescriptor`, because using the ASM infrastructure there is just very convenient and duplicating the logic into ArchUnit seems like more pain than gain for now (since this one isolated place would also be easy to fix in the future if need arises). However, there are also some conceptual dependencies from `JavaModifier` to ASM which were accidental, because it was just a convenient design to code those opcodes into the enum constants. Furthermore, we can't easily fix this anymore now, because there is public API in `JavaModifier` that allows creating modifiers from "ASM access" (which already shows the design flaw here). Likely the idea was that this would allow to create `JavaModifier`s from Reflection class objects, etc., but in hindsight I think this makes no sense. I would also suspect that actually no user ever uses this API. And it breaks the principle that users never create ArchUnit domain objects (which is a guideline for all other places). Thus, we deprecate these methods for now so we can remove them at some point in the future. Signed-off-by: Peter Gafert --- .../com/tngtech/archunit/ImporterRules.java | 19 ++++++++++++++++- .../archunit/core/domain/JavaModifier.java | 21 +++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/archunit-integration-test/src/test/java/com/tngtech/archunit/ImporterRules.java b/archunit-integration-test/src/test/java/com/tngtech/archunit/ImporterRules.java index b26acd3cb0..50b9709b4b 100644 --- a/archunit-integration-test/src/test/java/com/tngtech/archunit/ImporterRules.java +++ b/archunit-integration-test/src/test/java/com/tngtech/archunit/ImporterRules.java @@ -9,8 +9,10 @@ import com.tngtech.archunit.lang.ArchRule; import static com.tngtech.archunit.ArchUnitArchitectureTest.THIRDPARTY_PACKAGE_IDENTIFIER; +import static com.tngtech.archunit.base.DescribedPredicate.doNot; import static com.tngtech.archunit.base.DescribedPredicate.not; import static com.tngtech.archunit.core.domain.JavaClass.Predicates.belongToAnyOf; +import static com.tngtech.archunit.core.domain.JavaClass.Predicates.resideOutsideOfPackage; import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses; public class ImporterRules { @@ -18,7 +20,22 @@ public class ImporterRules { @ArchTest public static final ArchRule domain_does_not_access_importer = noClasses().that().resideInAPackage("..core.domain..") - .should().accessClassesThat(belong_to_the_import_context()); + .should().dependOnClassesThat(belong_to_the_import_context()); + + @ArchTest + public static final ArchRule asm_is_only_used_in_importer_or_JavaClassDescriptor = + noClasses() + .that( + resideOutsideOfPackage("..core.importer..") + .and(doNot(belongToAnyOf(JavaClassDescriptor.class + // Conceptually there are also dependencies from JavaModifier to ASM. + // However, at the moment all those are inlined by the compiler (primitives). + // Whenever we get the chance to break the public API + // we should remove the dependencies from JavaModifier to ASM. + // Those dependencies crept in by accident at some point because the design was convenient. + ))) + ) + .should().dependOnClassesThat().resideInAPackage("org.objectweb.."); @ArchTest public static final ArchRule ASM_type_is_only_accessed_within_JavaClassDescriptor_or_JavaClassDescriptorImporter = diff --git a/archunit/src/main/java/com/tngtech/archunit/core/domain/JavaModifier.java b/archunit/src/main/java/com/tngtech/archunit/core/domain/JavaModifier.java index f4a777bc2b..f88744204f 100644 --- a/archunit/src/main/java/com/tngtech/archunit/core/domain/JavaModifier.java +++ b/archunit/src/main/java/com/tngtech/archunit/core/domain/JavaModifier.java @@ -64,16 +64,37 @@ public enum JavaModifier { this.asmAccessFlag = asmAccessFlag; } + /** + * @deprecated This seems like an unnecessary API for users of ArchUnit, but limits us to do internal refactorings. + * If you think you need this API, please reach out to us on GitHub by creating an issue at + * https://github.com/TNG/ArchUnit/issues. + * Otherwise, at some point in the future we will remove this API without any replacement. + */ + @Deprecated @PublicAPI(usage = ACCESS) public static Set getModifiersForClass(int asmAccess) { return getModifiersFor(ApplicableType.CLASS, asmAccess); } + /** + * @deprecated This seems like an unnecessary API for users of ArchUnit, but limits us to do internal refactorings. + * If you think you need this API, please reach out to us on GitHub by creating an issue at + * https://github.com/TNG/ArchUnit/issues. + * Otherwise, at some point in the future we will remove this API without any replacement. + */ + @Deprecated @PublicAPI(usage = ACCESS) public static Set getModifiersForField(int asmAccess) { return getModifiersFor(ApplicableType.FIELD, asmAccess); } + /** + * @deprecated This seems like an unnecessary API for users of ArchUnit, but limits us to do internal refactorings. + * If you think you need this API, please reach out to us on GitHub by creating an issue at + * https://github.com/TNG/ArchUnit/issues. + * Otherwise, at some point in the future we will remove this API without any replacement. + */ + @Deprecated @PublicAPI(usage = ACCESS) public static Set getModifiersForMethod(int asmAccess) { return getModifiersFor(ApplicableType.METHOD, asmAccess); From 2cad3831e587995e84bf1ad999c6b764bc6466c8 Mon Sep 17 00:00:00 2001 From: Manfred Hanke Date: Wed, 9 Nov 2022 00:12:03 +0100 Subject: [PATCH 2/2] recognize empty records As `ClassVisitor#visitRecordComponent` is not invoked for empty records (without components), we better rely on the access flags provided to `ClassVisitor#visit`. Signed-off-by: Manfred Hanke --- .../ClassFileImporterRecordsTest.java | 24 ++++++++++++++----- .../archunit/core/domain/JavaModifier.java | 13 +++++++++- .../core/importer/JavaClassProcessor.java | 21 +++------------- 3 files changed, 33 insertions(+), 25 deletions(-) diff --git a/archunit/src/jdk16test/java/com/tngtech/archunit/core/importer/ClassFileImporterRecordsTest.java b/archunit/src/jdk16test/java/com/tngtech/archunit/core/importer/ClassFileImporterRecordsTest.java index 304e8779b6..9d181fe94b 100644 --- a/archunit/src/jdk16test/java/com/tngtech/archunit/core/importer/ClassFileImporterRecordsTest.java +++ b/archunit/src/jdk16test/java/com/tngtech/archunit/core/importer/ClassFileImporterRecordsTest.java @@ -4,21 +4,33 @@ import com.tngtech.archunit.core.domain.JavaConstructor; import com.tngtech.archunit.core.domain.JavaField; import com.tngtech.archunit.core.domain.JavaMethod; +import com.tngtech.java.junit.dataprovider.DataProvider; +import com.tngtech.java.junit.dataprovider.DataProviderRunner; +import com.tngtech.java.junit.dataprovider.UseDataProvider; import org.junit.Test; +import org.junit.runner.RunWith; import static com.tngtech.archunit.testutil.Assertions.assertThat; +import static com.tngtech.java.junit.dataprovider.DataProviders.testForEach; +@RunWith(DataProviderRunner.class) public class ClassFileImporterRecordsTest { - - @Test - public void imports_simple_record() { - record RecordToImport(String component1, int component2) { + @DataProvider + public static Object[][] imports_record() { + record SimpleRecord(String component1, int component2) { } + record EmptyRecord() { + } + return testForEach(SimpleRecord.class, EmptyRecord.class); + } - JavaClass javaClass = new ClassFileImporter().importClasses(RecordToImport.class, Record.class).get(RecordToImport.class); + @Test + @UseDataProvider + public void imports_record(Class recordToImport) { + JavaClass javaClass = new ClassFileImporter().importClasses(recordToImport, Record.class).get(recordToImport); assertThat(javaClass) - .matches(RecordToImport.class) + .matches(recordToImport) .hasRawSuperclassMatching(Record.class) .hasNoInterfaces() .isInterface(false) diff --git a/archunit/src/main/java/com/tngtech/archunit/core/domain/JavaModifier.java b/archunit/src/main/java/com/tngtech/archunit/core/domain/JavaModifier.java index f88744204f..5ccfd2783f 100644 --- a/archunit/src/main/java/com/tngtech/archunit/core/domain/JavaModifier.java +++ b/archunit/src/main/java/com/tngtech/archunit/core/domain/JavaModifier.java @@ -18,6 +18,7 @@ import java.util.EnumSet; import java.util.Set; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import com.tngtech.archunit.PublicAPI; import org.objectweb.asm.Opcodes; @@ -73,7 +74,17 @@ public enum JavaModifier { @Deprecated @PublicAPI(usage = ACCESS) public static Set getModifiersForClass(int asmAccess) { - return getModifiersFor(ApplicableType.CLASS, asmAccess); + Set modifiers = getModifiersFor(ApplicableType.CLASS, asmAccess); + boolean opCodeForRecordIsPresent = (asmAccess & Opcodes.ACC_RECORD) != 0; + if (opCodeForRecordIsPresent) { + // As records are implicitly static and final (compare JLS 8.10 Record Declarations), + // we ensure that those modifiers are always present. (asmAccess does not contain STATIC.) + return ImmutableSet.builder() + .addAll(modifiers) + .add(JavaModifier.STATIC, JavaModifier.FINAL) + .build(); + } + return modifiers; } /** diff --git a/archunit/src/main/java/com/tngtech/archunit/core/importer/JavaClassProcessor.java b/archunit/src/main/java/com/tngtech/archunit/core/importer/JavaClassProcessor.java index ca6973d972..14ba4075de 100644 --- a/archunit/src/main/java/com/tngtech/archunit/core/importer/JavaClassProcessor.java +++ b/archunit/src/main/java/com/tngtech/archunit/core/importer/JavaClassProcessor.java @@ -27,7 +27,6 @@ import com.google.common.collect.HashMultimap; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; import com.google.common.collect.SetMultimap; import com.google.common.primitives.Booleans; import com.google.common.primitives.Bytes; @@ -58,7 +57,6 @@ import org.objectweb.asm.Label; import org.objectweb.asm.MethodVisitor; import org.objectweb.asm.Opcodes; -import org.objectweb.asm.RecordComponentVisitor; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -114,6 +112,7 @@ public void visit(int version, int access, String name, String signature, String boolean opCodeForInterfaceIsPresent = (access & Opcodes.ACC_INTERFACE) != 0; boolean opCodeForEnumIsPresent = (access & Opcodes.ACC_ENUM) != 0; boolean opCodeForAnnotationIsPresent = (access & Opcodes.ACC_ANNOTATION) != 0; + boolean opCodeForRecordIsPresent = (access & Opcodes.ACC_RECORD) != 0; Optional superclassName = getSuperclassName(superName, opCodeForInterfaceIsPresent); LOG.trace("Found superclass {} on class '{}'", superclassName.orElse(null), name); @@ -123,7 +122,8 @@ public void visit(int version, int access, String name, String signature, String .withInterface(opCodeForInterfaceIsPresent) .withEnum(opCodeForEnumIsPresent) .withAnnotation(opCodeForAnnotationIsPresent) - .withModifiers(JavaModifier.getModifiersForClass(access)); + .withModifiers(JavaModifier.getModifiersForClass(access)) + .withRecord(opCodeForRecordIsPresent); className = descriptor.getFullyQualifiedClassName(); declarationHandler.onNewClass(className, superclassName, interfaceNames); @@ -153,21 +153,6 @@ public void visitSource(String source, String debug) { } } - @Override - public RecordComponentVisitor visitRecordComponent(String name, String descriptor, String signature) { - javaClassBuilder.withRecord(true); - - // Records are implicitly static and final (compare JLS 8.10 Record Declarations) - // Thus we ensure that those modifiers are always present (the access flag in visit(..) does not contain STATIC) - ImmutableSet recordModifiers = ImmutableSet.builder() - .addAll(javaClassBuilder.getModifiers()) - .add(JavaModifier.STATIC, JavaModifier.FINAL) - .build(); - javaClassBuilder.withModifiers(recordModifiers); - - return super.visitRecordComponent(name, descriptor, signature); - } - @Override public void visitInnerClass(String name, String outerName, String innerName, int access) { if (importAborted()) {