diff --git a/CHANGELOG.md b/CHANGELOG.md index 996793e..9057cc0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ uses semantic versioning for public releases. ### Fixed +- preserve ordinary member-owned dependencies in type and package cycle checks when synthetic + edges are ignored (the default), and exclude synthetic member evidence correctly; checks may + now report real cycles that `0.1.0` silently missed; - accept documented dotted CLI rule IDs while still rejecting unknown rule fields; - retain input and parser failures in CLI/direct `CliAnalyzer` results so corrupt archives or malformed classes cannot silently produce passing partial checks; and diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md index 807d70e..8b61635 100644 --- a/docs/USER_GUIDE.md +++ b/docs/USER_GUIDE.md @@ -105,6 +105,13 @@ These entry points are all under the The generated Javadocs list the overloads and option values. Prefer a narrow selector and an explicit external-dependency policy over broad patterns that happen to pass today. +Type and package cycle rules ignore evidence from synthetic types, synthetic members, and bridge +methods by default. Ordinary member-owned dependencies remain part of the graph, including an edge +that has both ordinary and synthetic evidence. Evidence without an owning member is retained unless +the edge itself originates from a synthetic type or member. Use +`CycleRuleOptions.withSyntheticEdges(SyntheticEdgePolicy.INCLUDE)` to include generated evidence; +type-use dependencies such as field types are controlled separately and are included by default. + ## Layers, slices, modules, and PlantUML Layers and slices are projections over the imported dependency graph. Define membership first, diff --git a/src/main/java/dev/archunitjava/rules/CycleRules.java b/src/main/java/dev/archunitjava/rules/CycleRules.java index 1ec240d..37ad6cf 100644 --- a/src/main/java/dev/archunitjava/rules/CycleRules.java +++ b/src/main/java/dev/archunitjava/rules/CycleRules.java @@ -294,7 +294,9 @@ private static DependencyGraph withoutSyntheticEvidence( && syntheticMembers.contains(member)); edge.evidence().stream() .filter(evidence -> !syntheticOrigin - && evidence.ownerMember().map(syntheticMembers::contains).orElse(true)) + && evidence.ownerMember() + .map(member -> !syntheticMembers.contains(member)) + .orElse(true)) .forEach(evidence -> result.addDependency( edge.origin(), edge.target(), edge.kind(), evidence)); } diff --git a/src/test/java/dev/archunitjava/rules/CycleRulesBytecodeTest.java b/src/test/java/dev/archunitjava/rules/CycleRulesBytecodeTest.java new file mode 100644 index 0000000..6dcb30f --- /dev/null +++ b/src/test/java/dev/archunitjava/rules/CycleRulesBytecodeTest.java @@ -0,0 +1,173 @@ +package dev.archunitjava.rules; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import dev.archunitjava.cli.CliAnalysisResult; +import dev.archunitjava.cli.CliAnalyzer; +import dev.archunitjava.cli.CliConfigurationLoader; +import dev.archunitjava.graph.DependencyEvidence; +import dev.archunitjava.graph.DependencyKind; +import dev.archunitjava.graph.MemberId; +import dev.archunitjava.graph.TypeId; +import dev.archunitjava.model.JavaMemberModifier; +import dev.archunitjava.pattern.JavaPattern; +import dev.archunitjava.pattern.PatternDomain; +import dev.archunitjava.result.RuleStatus; +import dev.archunitjava.selector.PackageSelector; +import dev.archunitjava.selector.TypeSelector; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +class CycleRulesBytecodeTest { + private static final String PREFIX = "dev.archunitjava.rules.fixtures.cycles."; + private static final TypeId LEFT = TypeId.ofBinaryName(PREFIX + "left.CycleLeft"); + private static final TypeId RIGHT = TypeId.ofBinaryName(PREFIX + "right.CycleRight"); + + @TempDir Path temporaryDirectory; + private CliAnalysisResult analysis; + + @BeforeEach + void importJavacFixturesWithoutLoadingTargetClasses() throws IOException { + for (String name : List.of(LEFT.binaryName(), RIGHT.binaryName(), + LEFT.binaryName() + "$OneWay", RIGHT.binaryName() + "$Leaf")) { + String resource = name.replace('.', '/') + ".class"; + Path target = temporaryDirectory.resolve("classes").resolve(resource); + Files.createDirectories(target.getParent()); + try (var input = CycleRulesBytecodeTest.class.getResourceAsStream("/" + resource)) { + assertNotNull(input, resource); + Files.copy(input, target); + } + } + Path configuration = temporaryDirectory.resolve("archunitjava.properties"); + Files.writeString(configuration, """ + schema=archunitjava.cli.v1 + inputs=classes + rules=fixture + rule.fixture.domain=types + rule.fixture.mode=only + rule.fixture.origins=glob:dev.archunitjava.rules.fixtures.cycles.** + rule.fixture.targets=glob:dev.archunitjava.rules.fixtures.cycles.** + rule.fixture.external=ignore + """); + analysis = new CliAnalyzer().analyze( + CliConfigurationLoader.load(configuration, temporaryDirectory)); + assertTrue(analysis.imports().model().classFileDiagnostics().isEmpty()); + assertEquals(4, analysis.imports().model().types().size()); + } + + @Test + void defaultRulesDetectJavacTypeAndPackageCycles() { + TypeSelector selected = types(LEFT.binaryName(), RIGHT.binaryName()); + assertEquals(RuleStatus.FAILED, CycleRules.typesAreAcyclic( + analysis.imports().model(), analysis.graph(), selected).check().status()); + assertEquals(RuleStatus.FAILED, CycleRules.packagesAreAcyclic( + analysis.imports().model(), analysis.graph(), PackageSelector.all()).check().status()); + } + + @Test + void javacMethodCyclesAreDetectedUnderEverySyntheticPolicy() { + assertCyclesUnderEverySyntheticPolicy(DependencyKind.METHOD_CALL); + } + + @Test + void javacFieldCyclesRespectTheIndependentTypeUsePolicy() { + assertCyclesUnderEverySyntheticPolicy(DependencyKind.FIELD_TYPE); + var noTypeUses = CycleRuleOptions.defaults().includingOnly(Set.of(DependencyKind.FIELD_TYPE)) + .withTypeUseDependencies(TypeUseDependencyPolicy.IGNORE); + assertEquals(RuleStatus.PASSED, CycleRules.typesAreAcyclic( + analysis.imports().model(), analysis.graph(), types(LEFT.binaryName(), RIGHT.binaryName()), + noTypeUses).check().status()); + } + + @Test + void javacLambdaEvidenceIsFilteredWithoutRemovingOrdinaryCallsOnTheSameEdge() { + Set syntheticMembers = analysis.imports().model().types().stream() + .flatMap(type -> type.declaredMembers().stream() + .filter(member -> member.modifiers().contains(JavaMemberModifier.SYNTHETIC)) + .map(member -> MemberId.of(TypeId.ofBinaryName(type.binaryName()), + member.name(), member.descriptor()))) + .collect(Collectors.toSet()); + assertFalse(syntheticMembers.isEmpty(), "javac must generate a lambda implementation member"); + var calls = analysis.graph().edges().stream() + .filter(edge -> edge.origin().equals(LEFT) && edge.target().equals(RIGHT) + && edge.kind() == DependencyKind.METHOD_CALL) + .flatMap(edge -> edge.evidence().stream()).toList(); + assertTrue(calls.stream().anyMatch(evidence -> syntheticMembers.contains( + evidence.ownerMember().orElseThrow()))); + assertTrue(calls.stream().anyMatch(evidence -> !syntheticMembers.contains( + evidence.ownerMember().orElseThrow()))); + + for (SyntheticEdgePolicy policy : SyntheticEdgePolicy.values()) { + var result = CycleRules.typesAreAcyclic(analysis.imports().model(), analysis.graph(), + types(LEFT.binaryName(), RIGHT.binaryName()), CycleRuleOptions.defaults() + .includingOnly(Set.of(DependencyKind.METHOD_CALL)) + .withSyntheticEdges(policy)).check(); + assertEquals(RuleStatus.FAILED, result.status()); + Set owners = result.violations().getFirst().evidence().stream() + .map(DependencyEvidence::ownerMember).map(owner -> owner.orElseThrow()) + .collect(Collectors.toSet()); + assertTrue(owners.contains(MemberId.of(LEFT, "work", "()V"))); + assertTrue(owners.contains(MemberId.of(RIGHT, "work", "()V"))); + assertEquals(policy == SyntheticEdgePolicy.INCLUDE, + owners.stream().anyMatch(syntheticMembers::contains)); + } + } + + @Test + void javacOneWayDependenciesRemainAcyclicUnderEverySyntheticPolicy() { + TypeSelector selected = types(LEFT.binaryName() + "$OneWay", RIGHT.binaryName() + "$Leaf"); + assertEquals(2, selected.selectFrom(analysis.imports().model()).selected().size()); + Set importedKinds = analysis.graph().edges().stream() + .filter(edge -> edge.origin().equals(TypeId.ofBinaryName(LEFT.binaryName() + "$OneWay")) + && edge.target().equals(TypeId.ofBinaryName(RIGHT.binaryName() + "$Leaf"))) + .map(edge -> edge.kind()).collect(Collectors.toSet()); + assertTrue(importedKinds.containsAll(Set.of(DependencyKind.METHOD_CALL, DependencyKind.FIELD_TYPE))); + assertEquals(RuleStatus.PASSED, CycleRules.typesAreAcyclic( + analysis.imports().model(), analysis.graph(), selected).check().status()); + for (SyntheticEdgePolicy policy : SyntheticEdgePolicy.values()) { + var result = CycleRules.typesAreAcyclic(analysis.imports().model(), analysis.graph(), + selected, CycleRuleOptions.defaults().withSyntheticEdges(policy)).check(); + assertEquals(RuleStatus.PASSED, result.status(), policy.name()); + assertTrue(result.violations().isEmpty()); + } + } + + private void assertCyclesUnderEverySyntheticPolicy(DependencyKind kind) { + var edges = analysis.graph().edges().stream() + .filter(edge -> edge.kind() == kind) + .filter(edge -> Set.of(LEFT, RIGHT).contains(edge.origin()) + && Set.of(LEFT, RIGHT).contains(edge.target())) + .toList(); + assertEquals(2, edges.size(), kind.name()); + assertTrue(edges.stream().flatMap(edge -> edge.evidence().stream()) + .allMatch(evidence -> evidence.ownerMember().isPresent())); + for (SyntheticEdgePolicy policy : SyntheticEdgePolicy.values()) { + var options = CycleRuleOptions.defaults().includingOnly(Set.of(kind)).withSyntheticEdges(policy); + var result = CycleRules.typesAreAcyclic(analysis.imports().model(), analysis.graph(), + types(LEFT.binaryName(), RIGHT.binaryName()), options).check(); + assertEquals(RuleStatus.FAILED, result.status(), kind + "/" + policy); + assertEquals(1, result.violations().size()); + assertFalse(result.violations().getFirst().evidence().isEmpty()); + var packages = CycleRules.packagesAreAcyclic(analysis.imports().model(), + analysis.graph(), PackageSelector.all(), options).check(); + assertEquals(RuleStatus.FAILED, packages.status(), kind + "/" + policy); + assertEquals(1, packages.violations().size()); + } + } + + private static TypeSelector types(String first, String second) { + return TypeSelector.anyOf( + TypeSelector.binaryName(JavaPattern.exact(PatternDomain.QUALIFIED_NAME, first)), + TypeSelector.binaryName(JavaPattern.exact(PatternDomain.QUALIFIED_NAME, second))); + } +} diff --git a/src/test/java/dev/archunitjava/rules/CycleRulesTest.java b/src/test/java/dev/archunitjava/rules/CycleRulesTest.java index 85a1712..de0207f 100644 --- a/src/test/java/dev/archunitjava/rules/CycleRulesTest.java +++ b/src/test/java/dev/archunitjava/rules/CycleRulesTest.java @@ -36,8 +36,11 @@ class CycleRulesTest { private static final TypeId C = TypeId.ofBinaryName("app.C"); private static final TypeId D = TypeId.ofBinaryName("other.D"); private static final TypeId GENERATED = TypeId.ofBinaryName("app.Generated"); + private static final MemberId A_WORK = MemberId.of(A, "work", "()V"); private static final MemberId A_BRIDGE = MemberId.of(A, "bridge", "()V"); + private static final MemberId A_SYNTHETIC = MemberId.of(A, "generated", "()V"); private static final MemberId B_WORK = MemberId.of(B, "work", "()V"); + private static final MemberId D_WORK = MemberId.of(D, "work", "()V"); private static final MemberId GENERATED_WORK = MemberId.of(GENERATED, "work", "()V"); @TempDir Path temporaryDirectory; @@ -132,10 +135,91 @@ void typeUseAndSyntheticEdgesAreIndependentExplicitPolicies() { .check().status()); } + @Test + void ordinaryMemberEvidenceIsRetainedUnderEverySyntheticPolicy() { + DependencyEvidence outward = evidence(A_WORK, 10); + DependencyEvidence inward = evidence(B_WORK, 20); + DependencyGraph graph = DependencyGraph.builder() + .addNode(A).addNode(B) + .addDependency(A, B, DependencyKind.METHOD_CALL, outward) + .addDependency(B, A, DependencyKind.METHOD_CALL, inward) + .build(); + + assertEquals(RuleStatus.FAILED, + CycleRules.typesAreAcyclic(model, graph, packages("app")).check().status()); + for (SyntheticEdgePolicy policy : SyntheticEdgePolicy.values()) { + var result = CycleRules.typesAreAcyclic(model, graph, packages("app"), + CycleRuleOptions.defaults().withSyntheticEdges(policy)).check(); + assertEquals(RuleStatus.FAILED, result.status(), policy.name()); + assertEquals(1, result.violations().size()); + assertEquals(List.of(outward, inward), result.violations().getFirst().evidence()); + } + } + + @Test + void syntheticMemberEvidenceCannotCreateAnOrdinaryTypeCycle() { + for (MemberId owner : List.of(A_BRIDGE, A_SYNTHETIC)) { + DependencyGraph graph = DependencyGraph.builder() + .addNode(A).addNode(B) + .addDependency(A, B, DependencyKind.METHOD_CALL, evidence(owner, 10)) + .addDependency(B, A, DependencyKind.METHOD_CALL, evidence(null, 20)) + .build(); + + assertEquals(RuleStatus.PASSED, + CycleRules.typesAreAcyclic(model, graph, packages("app")).check().status(), + owner.stableKey()); + var included = CycleRules.typesAreAcyclic(model, graph, packages("app"), + CycleRuleOptions.defaults().withSyntheticEdges(SyntheticEdgePolicy.INCLUDE)) + .check(); + assertEquals(RuleStatus.FAILED, included.status()); + assertEquals(1, included.violations().size()); + } + } + + @Test + void mixedEvidenceKeepsTheOrdinaryCycleAndDropsOnlySyntheticOwners() { + DependencyEvidence ordinary = evidence(A_WORK, 10); + DependencyEvidence synthetic = evidence(A_BRIDGE, 20); + DependencyEvidence reverse = evidence(B_WORK, 30); + DependencyGraph graph = DependencyGraph.builder() + .addNode(A).addNode(B) + .addDependency(A, B, DependencyKind.METHOD_CALL, ordinary) + .addDependency(A, B, DependencyKind.METHOD_CALL, synthetic) + .addDependency(B, A, DependencyKind.METHOD_CALL, reverse) + .build(); + + var ignored = CycleRules.typesAreAcyclic(model, graph, packages("app")).check(); + assertEquals(RuleStatus.FAILED, ignored.status()); + assertEquals(List.of(ordinary, reverse), ignored.violations().getFirst().evidence()); + var included = CycleRules.typesAreAcyclic(model, graph, packages("app"), + CycleRuleOptions.defaults().withSyntheticEdges(SyntheticEdgePolicy.INCLUDE)).check(); + assertEquals(RuleStatus.FAILED, included.status()); + assertEquals(List.of(ordinary, synthetic, reverse).stream().sorted().toList(), + included.violations().getFirst().evidence()); + } + + @Test + void syntheticOriginsAreIgnoredEvenWithoutEvidenceOwners() { + for (var origin : List.of(GENERATED, A_BRIDGE)) { + DependencyGraph graph = DependencyGraph.builder() + .addNode(origin).addNode(B) + .addDependency(origin, B, DependencyKind.METHOD_CALL, evidence(null, 10)) + .addDependency(B, origin, DependencyKind.METHOD_CALL, evidence(null, 20)) + .build(); + + assertEquals(RuleStatus.PASSED, + CycleRules.typesAreAcyclic(model, graph, packages("app")).check().status()); + assertEquals(RuleStatus.FAILED, + CycleRules.typesAreAcyclic(model, graph, packages("app"), + CycleRuleOptions.defaults().withSyntheticEdges(SyntheticEdgePolicy.INCLUDE)) + .check().status()); + } + } + @Test void packageProjectionIgnoresIntraPackageEdgesAndRetainsUnderlyingEvidence() { - DependencyEvidence outward = evidence(null, 50); - DependencyEvidence inward = evidence(null, 60); + DependencyEvidence outward = evidence(A_WORK, 50); + DependencyEvidence inward = evidence(D_WORK, 60); DependencyGraph graph = DependencyGraph.builder() .addNode(A).addNode(B).addNode(D) .addDependency(A, B, DependencyKind.METHOD_CALL, evidence(null, 40)) @@ -182,6 +266,10 @@ private static byte[] type(String binaryName, boolean syntheticBridge) { "bridge", MethodTypeDesc.of(ClassDesc.ofDescriptor("V")), ClassFile.ACC_PUBLIC | ClassFile.ACC_SYNTHETIC | ClassFile.ACC_BRIDGE, code -> code.return_()); + builder.withMethodBody( + "generated", MethodTypeDesc.of(ClassDesc.ofDescriptor("V")), + ClassFile.ACC_PRIVATE | ClassFile.ACC_SYNTHETIC, + code -> code.return_()); } }); } diff --git a/src/test/java/dev/archunitjava/rules/fixtures/cycles/left/CycleLeft.java b/src/test/java/dev/archunitjava/rules/fixtures/cycles/left/CycleLeft.java new file mode 100644 index 0000000..9aa074d --- /dev/null +++ b/src/test/java/dev/archunitjava/rules/fixtures/cycles/left/CycleLeft.java @@ -0,0 +1,24 @@ +package dev.archunitjava.rules.fixtures.cycles.left; + +import dev.archunitjava.rules.fixtures.cycles.right.CycleRight; + +/** Compiled by the test build; imported as bytes, never loaded or executed by the analyzer. */ +public final class CycleLeft { + private CycleRight right; + + public void work() { + right.work(); + } + + public Runnable deferred() { + return () -> right.work(); + } + + public static final class OneWay { + private CycleRight.Leaf leaf; + + public void work() { + leaf.work(); + } + } +} diff --git a/src/test/java/dev/archunitjava/rules/fixtures/cycles/right/CycleRight.java b/src/test/java/dev/archunitjava/rules/fixtures/cycles/right/CycleRight.java new file mode 100644 index 0000000..dbe41ef --- /dev/null +++ b/src/test/java/dev/archunitjava/rules/fixtures/cycles/right/CycleRight.java @@ -0,0 +1,16 @@ +package dev.archunitjava.rules.fixtures.cycles.right; + +import dev.archunitjava.rules.fixtures.cycles.left.CycleLeft; + +/** Compiled by the test build; imported as bytes, never loaded or executed by the analyzer. */ +public final class CycleRight { + private CycleLeft left; + + public void work() { + left.work(); + } + + public static final class Leaf { + public void work() {} + } +}