TNG / ArchUnit

A Java architecture test library, to specify and assert architecture rules in plain Java
http://archunit.org
Apache License 2.0
3.24k stars 297 forks source link

ArchUnit thinks "Switch with arrows" produces non-final fields #1313

Open lasselindqvist opened 5 months ago

lasselindqvist commented 5 months ago

Example code:

having some method with a call inside it like:

            var status = service.waitForResult(taskId, Duration.ofMinutes(1));
            switch (status) {
            case COMPLETED -> result.setStatus(StatusEnum.COMPLETED);
            case FAILED -> result.setStatus(StatusEnum.FAILED);
            default -> result.setStatus(StatusEnum.PENDING);
            }

causes a violation like Field <com.controller.Controller.$SWITCH_TABLE$...$StatusEnum> is not final in (Controller.java:0)

with a test

    @ArchTest
    public static final ArchRule rule = ArchRuleDefinition.classes()
        .that()
        .areAnnotatedWith(Controller.class)
        .should().haveOnlyFinalFields();

Maybe this is the fault of the compiler used (Eclipse), but I wonder if ArchUnit can ignore these switch tables when checking the rule?

lasselindqvist commented 5 months ago

I guess this can be handled by skipping fields with JavaModifier.SYNTHETIC. Even if we don't want to do that in the default haveOnlyFinalFields method, maybe it would be good to show/document this workaround somehow.

I suppose one way is to check this using ArchRuleDefinition::fields, filtering with ‎MembersThatInternal::doNotHaveModifier(JavaModifier.SYNTHETIC) and then check for the final modifier.

geoludbit commented 2 months ago

Is this going to be fixed in some version or is there a workaround? We are having this issue with java 21.

lasselindqvist commented 1 month ago

Here is a proper example of the workaround example for anyone wondering:

    @ArchTest
    public static final ArchRule rule = 
        ArchRuleDefinition.fields()
        .that()
        .doNotHaveModifier(JavaModifier.SYNTHETIC)
        .and()
        .areDeclaredInClassesThat(CanBeAnnotated.Predicates.annotatedWith(Controller.class))
        .should(ArchConditions.beFinal());
codecholeric commented 6 days ago

Sorry for the late reply and thanks for the workaround! It might make sense to ignore this by default in reasonable predicates. This is a good case where it likely never makes sense to consider synthetic fields. I've tried to exclude all synthetic members and classes by default at some point, but that actually caused a ton of problems back then. But for specific predefined predicates we could likely do this without any danger to overlook something 🤔