Dependency check of Enum Types in Switch-Expression/Statement

[bfncs]

If you define a dependency check for an Enum Type that is used in a switch-Expression (or old style Statement) this leads to a false positive failed test. I'll try to explain this better with an example:

If you define this Enum in a package lib:

public enum ExampleEnum {
  FOO,
  BAR
}

…and you use it like this in a package app:

public static String failingExample1(final ExampleEnum exampleEnum) {
	return switch (exampleEnum) {
		case FOO -> "foo";
		case BAR -> "bar";
	};
}

…and you have a test containing a check to assert classes in package app only depend on class that reside in package lib:

@ArchTest
public static final ArchRule DEPENDENCY_TEST_EXAMPLE =
    classes()
        .that()
        .resideInAPackage("..app..")
        .should()
        .onlyDependOnClassesThat(resideInAPackage("..lib.."));

…the Test fails with the following unexpected Architecture Violation:

Method <us.byteb.archunit.app.Example.failingExample1(us.byteb.archunit.lib.ExampleEnum)> gets field <us.byteb.archunit.app.Example$1.$SwitchMap$us$byteb$archunit$lib$ExampleEnum> in (Example.java:8)

This is also true if you refactor the same logic to use and old-style switch-Statement but goes away if you use if-Statements instead. I would expect all three semantically equal options to produce the same result: a successful test.

You can find a compiling implementation of the tests in this repository (Problem happens with Java 8, 11, 14 & 16). Please let me know, if I can be of any help in resolving it.

[bfncs]

In case someone else runs into this: a working hotfix is to explicitly allow dependencies to int arrays:

@ArchTest
public static final ArchRule DEPENDENCY_TEST_EXAMPLE =
    classes()
        .that()
        .resideInAPackage("..app..")
        .should()
        .onlyDependOnClassesThat(
            resideInAPackage("..lib..")
                .or(type(int[].class))); // <-- whitelist dependency on int[]

I still think that it is at least misleading to have a failed test here due to how the JDK adds synthetic constructs to implements Enum Values.

[codecholeric]

I agree, that this is not nice. There is also the JavaModifier.SYNTHETIC that could maybe be used to identify such irrelevant member. I once looked into ignoring synthetic classes and members for a while, but ignoring all caused a lot of pain somehow.
This is still an open issue for me, because I'd love to find a better solution, but so far could not find the time to look deeper into it, if there maybe is a certain set of synthetic members that can be ignored safely. (Originally I would really have thought all such members and classes can be ignored, but as I said, it caused a lot of test failures when I tried it)

[bfncs]

Thanks for looking into this and commenting. I already expected this to be complicated, sadly. Also I tried do debug the code but my lack of knowledge in this area was much too big to find a „simple“ fix. I'd happily help out if there's anything that can be done apart from fixing the underlying issue about synthetic classes and members.

[codecholeric]

Hmm, I think unfortunately the only reasonable fix that doesn't feel like a workaround is at the very core 🤔 In the end we could even put some "well-known" exceptions into the import process, if a full-blown synthetic member solution is not feasible...

[bfncs]

This issue still exists with ArchUnit 0.23.1. Has there been any progress made on the underlying structural problems of this issue? Is it maybe more feasible for someone to help out with fixing it? It still seems impossible for me to contribute something valuable without first agreeing on the general direction to go.

[codecholeric]

Sorry, I didn't have the time to look into it yet 🙈 But I agree that this is not nice and should be improved. We recently also did some synthetic method transformations, e.g. for lambda$...$123 methods to merge them together with the respective invokedynamic. This would likely need something similar 🤔
Because in the end you would want to see a dependency Example -> EvilDependency in the following case, right?

public static String failingExample(final ExampleEnum exampleEnum) {
	return switch (exampleEnum) {
		case FOO -> evilDependency.getEvilString();
	};
}

At the moment you would AFAIS have two dependencies Example -> Example$1 and Example$1 -> EvilDependency 🤔

So I don't think simply throwing out the first dependency to the synthetic class will do any good. Instead, we would likely have to fix this like lambdas, i.e. merge these well-known wrong dependency chains into single dependencies again 🤔 Unfortunately, this is a lot of bytecode specifics creeping into ArchUnit, but I see that it would improve usability 😞

I recently also tackled something similar for private accesses -> 99b0074

I'd be happy if you want to tackle this, otherwise I can also try to find some time (but can't promise when)! If you should start to work on it make sure to give me a little note her so we won't look into it at the same time 😉

[codecholeric]

I've actually found some time today to look into it 🙂 Looking into it more closely now I also saw that it is way easier than I thought 🥳 Because it is not an indirection, where the compiler puts the content of the case statement into a synthetic method, but it's really just an additional field lookup that needs to be ignored.
So I'm quite confident that I can create a PR shortly 😉 (and I'm glad I don't need to write the 3rd merging processor for this to fix accesses with an indirection through synthetic code)

[ecattez]

Hello guys. Will this fix be released soon ?

[codecholeric]

I'll merge it now, so it will be in the next release. However, the next release will be 1.0-rc1, so I'm gathering all the breaking changes at the moment. I'm waiting for a couple of reviews, then I'll release. I think something between 1-3 weeks until everything is merged and ready with all the async processes 😬

[bfncs]

Thanks for your great work! This fix is very much appreciated.