Synthetic fields considered by matching rules

[bedla]

Hi, I have found that synthetic fields are matched against specified rules. I think that this kind of fields (also methods?) should be skipped. I can create PR, but first I want to discuss it... Ivos

    @Test
    public void xxx() {
        noClasses()
                .should()
                .accessClassesThat()
                .resideOutsideOfPackages(
                        "com.tngtech.archunit.lang.syntax.elements..",
                        "java..")
                .check(importClasses(Foo.class));
    }

    public enum Foo {
        A, B, C;

        static void doIt() {
            for (Foo value : values()) {
                System.out.println(value);
            }
        }
    }

Violation output is

java.lang.AssertionError: Architecture Violation [Priority: MEDIUM] - Rule 'no classes should access classes that reside outside of packages ['com.tngtech.archunit.lang.syntax.elements..', 'java..']' was violated (1 times):
Method <com.tngtech.archunit.lang.syntax.elements.ClassesShouldTest$Foo.values()> calls method <[Lcom.tngtech.archunit.lang.syntax.elements.ClassesShouldTest$Foo;.clone()> in (ClassesShouldTest.java:1175)

[codecholeric]

This already came up recently: https://github.com/TNG/ArchUnit/issues/81

I do agree, that there is little value in checking synthetic methods for architecture tests, I'm just not completely sure, at which level this should be approached.

ArchUnit tries to mimic the Reflection API at its core layer, and the Reflection API allows access to synthetic method, thus up to now synthetic methods are imported.

I see two approaches:

1. Don't import any synthetic methods / fields at all
2. Filter synthetic accesses at the rule API level

With 1. the advantage is, that it is a very limited scope that needs to be adjusted, just at the import level. On the other hand, information will be missing if this should be relevant on some level for tests. I don't like that implication too much, but I have to admit that I can't really think of an use case for having access to synthetic members at the moment. With 2. the advantage would be, that the core API stays as powerful as it has been up to now, but on the downside it's various places that need to be adjusted, and every user has to consider synthetic members in custom predicates.

An alternative might be, to filter on the import level, but make the behavior configurable, i.e. extend ArchConfiguration to allow specifying this behavior, with the default skipping any synthetic members from the import.

Any opinions or further ideas?

[bedla]

I also cannot find relevant usecase of somebody want's to access synthetic members. I like 3rd opition - make it configurable and by default skip them :]

[codecholeric]

Upon closer investigation I have my doubts, that this concrete case is really solved by excluding synthetic members. Enum.values() does not count as synthetic (even though the compiler creates it, compare https://docs.oracle.com/javase/specs/jvms/se10/html/jvms-4.html#jvms-4.7.8). And the access to [Lcom.MyEnum;.clone() is a different type of beast (the target will obviously not be present in the import, so it will always be a stub, providing no further information than its name) This would probably need some customized solution, where specific (irrelevant) accesses will be filtered out. It might be good enough to just skip that one call to clone() for enums, don't know. On the other hand, one could argue that maybe all calls to [Lsome.Type; are kind of irrelevant, the target package will always be empty (just like some.Type[].class.getPackage()).

However, I could imagine it a valid use case to ensure, that calls to arrays of a certain type are handled specifically... So maybe it should just be a mechanism where it is easy to plug in specific cases like the call values() -> clone(), which are then filtered by default?

[bedla]

To be clear Enum.values() is not synthetis, but field $VALUES is ... see description here http://www.benf.org/other/cfr/how-are-enums-implemented.html . So If I understand it correctly, we do not have problem with call of .values(). Maybe I am missing something, but that .clone() call on synthetic field could be skipped, right? Because it is on synthetic field, or I am on wrong path :]

[codecholeric]

But the access that causes you the problem, is

 ClassesShouldTest$Foo.values() -> [ClassesShouldTest$Foo;.clone()

not the access to the synthetic field $VALUES (which is a member of Foo). So the violation is the call of the (non-synthetic) method Foo[].clone()

[bedla]

True. (Call of non-synthetic method .clone(), but on synthetic field $VALUES.) So question is if we want to skip all accesses to every synthetic member.

[codecholeric]

The information that in this case the method is called on an instance of a synthetic field is definitely not that easily available (even though it is certainly possible). We would need to consider the context, i.e. which instance is put on the stack before invoke... is called. That is just a new aspect of the JavaClassProcessor, since up to now, such context was not necessary.

[bedla]

Ahh, I see. In RecordAccessHandler#handleMethodInstruction that RawAccessRecord does not have info on which field it has been called.

[codecholeric]

Yes, so I guess that would mean some heavier refactoring, where some sort of 'context' or 'history' object is dragged along, so that one can see, if the last stack operation before a method call referred to a synthetic field.

[bedla]

I am going to play with it a bit :] Do you think that this "contextual" information would be helpful for some future assertions in DSL?

[codecholeric]

Yes, feel free :smiley: I don't think generic "contextual" information (like which field was read before this method was called) is necessary for the DSL right now, also this would couple the way classes are imported (ASM visitor) closer to the Core API, which I don't think is good. I think we should extract the info we need for the imported JavaClasses from this "context" on creation and drop it afterwards. E.g. if it's necessary to know that a Method call is to a synthetic field, I would add just that info to the JavaMethodCall (or just filter it out, like discussed), not the whole context.

[JohT]

I ran into the same issue: Method <...Foo.values()> calls method <[L...Foo;.clone()> Are there any news? Is there a workaround? I tried very simple ones like not(name("clone")), but it seems to get ignored.

[codecholeric]

@JohT I tried to play around with it, but so far I couldn't manage to find a way to not also lose critical information in cases of overridden methods with generics, etc. Can you tell me a little more precisely what issue you ran into? Did you try exactly the same as on top of this issue? Because meanwhile an array should be considered to have the same package as a class, so if com.myapp.Foo.values() is allowed to depend on com.myapp, then com.myapp.Foo.values() -> com.myapp.Foo[].clone() should actually be legal... Also what you describe, exclude name("clone") should also work for a synthetic method / call.

[JohT]

Hi @codecholeric, I managed to isolate the issue with a simple reproducer.

I played around with some predicates, but i found no solution. This may be on me, since i'm still learning to use ArchUnit (absolutely great lib btw).

[codecholeric]

@JohT thanks a lot for your example!! :+1: That makes it a lot easier for me. I think that is a different issue though (and not trivial, I have to look more into it). The problem is not, that there are synthetic methods involved, but that onlyCallMethodsThat(are(not(annotatedWith(Deprecated.class)))) does not work as expected in that case. I've opened a new issue to track and fix it: #340

[JohT]

I've opened a new issue to track and fix it: #340

@codecholeric, thank you!