Volatile should also be applicable to Java Methods

[ratoaq2]

I'm not sure if this is related to #87

I noticed that several methods that are actually in an abstract super class was appearing in one of my tests as they were in the subclass. After some investigation I noticed that all these methods are marked as "volatile" by java reflect (Modifier.isVolatile()) but the JavaModifier from ArchUnit wasn't showing them as "volatile". It seems that ArchUnit only allow volatile at the field level:
VOLATILE(EnumSet.of(ApplicableType.FIELD), Opcodes.ACC_VOLATILE),

The only way to filter them out from my tests was to create a predicate that does a reflect and use standard java reflect API to check the modifiers (which cause the test to be much slower).

[hankem]

Not allowing volatile methods is consistent with the Java Language Specification. 😉

Modifier.isVolatile(int) and Modifier.toString(int) cannot distinguish Modifier.VOLATILE for fields from Modifier.BRIDGE (internal) for methods – cf., e.g., Stack Overflow: 'volatile' in method signature?

[ratoaq2]

Is there a way to filter them out in ArchUnit?
I'm debugging now and I can see that I have 2 methods that looks the same. Same class, same name, same parameters, etc... but one with my annotation and the other without my annotation.

The reflect of both methods return the same java.lang.reflect.Method

[ratoaq2]

I also noticed that sometimes the reflect for the 2 JavaMethod instances returns the method that has the volatile modifier and sometimes not. I get different results in different executions.

[codecholeric]

As @hankem pointed out, by the JLS there is no VOLATILE for methods. If you look at the definition of volatile this also makes sense in my opinion 😉
Good old "integerly typing" will give the same result for Modifier.isVolatile(..) and Modifier.isBridge(..), so what you really want to check is if a method is a bridge method.
What is a shortcoming of ArchUnit though is, that there is no way to access those types of modifiers without using reflection. We should add BRIDGE and SYNTHETIC I think, even if we still haven't solved #87 (and maybe never will)

For now I think you have no other choice than to use reflection ☹️

What I'm a little puzzled about though, is that you say reflect() would sometimes give you the one and sometimes the other method. The code of reflect() is pretty much

reflectedOwner.getDeclaredMethod(getName(), reflect(getRawParameterTypes()))

so it falls back to the reflection API, and if you look at the explanation about bridge methods, a bridge method should not have the same signature as the concrete method. I.e. the bridge method has the signature of the raw type, the concrete method of the type parameter, I don't see, how that could fall together? Maybe you can provide a quick example so I could check it out?

[ratoaq2]

I'll try to create a test case later this week.
The situation I'm faced while investigating the volatile issue was a big different:

ArchUnit detected 2 JavaMethod for my class with exact same parameters, name, etc.
None of them were volatile. Both have everything the same, except that one has annotations and the other doesn't. And the reflect of both methods return the same java.lang.reflect.Method

[ratoaq2]

As promised, this is the failing test case:

import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.methods;

import org.junit.runner.RunWith;

import com.tngtech.archunit.core.domain.JavaModifier;
import com.tngtech.archunit.junit.AnalyzeClasses;
import com.tngtech.archunit.junit.ArchTest;
import com.tngtech.archunit.junit.ArchUnitRunner;
import com.tngtech.archunit.lang.ArchRule;

@RunWith(ArchUnitRunner.class)
@AnalyzeClasses(packagesOf = FooBarTest.class)
public class FooBarTest {

    @ArchTest
    public static final ArchRule FAILING = methods()
            .that().areDeclaredInClassesThat().doNotHaveModifier(JavaModifier.ABSTRACT)
            .and().areDeclaredInClassesThat().areAssignableTo(AbstractFoo.class)
            .should().beAnnotatedWith(Deprecated.class);

    static abstract class AbstractFoo<T> {

        abstract T bar();
    }

    public static class Foo extends AbstractFoo<Foo> {

        @Override
        @Deprecated
        Foo bar() {
            return null;
        }

        @Deprecated
        void thisIsFine() {
        }

        void validFailure() {
        }
    }
}

Instead of getting 1 failure (for validFailure()), we get 2 failures:

java.lang.AssertionError: Architecture Violation [Priority: MEDIUM] - Rule 'methods that are declared in classes that do not have modifier ABSTRACT and are declared in classes that are assignable to com.mypackage.archunit.FooBarTest$AbstractFoo should be annotated with @Deprecated' was violated (2 times):
Method <com.mypackage.archunit.FooBarTest$Foo.bar()> is not annotated with @Deprecated in (FooBarTest.java:0)
Method <com.mypackage.archunit.FooBarTest$Foo.validFailure()> is not annotated with @Deprecated in (FooBarTest.java:0)
	at com.tngtech.archunit.lang.ArchRule$Assertions.assertNoViolation(ArchRule.java:92)
	at com.tngtech.archunit.lang.ArchRule$Assertions.check(ArchRule.java:82)
	at com.tngtech.archunit.lang.ArchRule$Factory$SimpleArchRule.check(ArchRule.java:196)
	at com.tngtech.archunit.lang.syntax.ObjectsShouldInternal.check(ObjectsShouldInternal.java:81)
	at com.tngtech.archunit.junit.ArchRuleExecution.evaluateOn(ArchRuleExecution.java:41)
	at com.tngtech.archunit.junit.ArchUnitRunner.runChild(ArchUnitRunner.java:154)
	at com.tngtech.archunit.junit.ArchUnitRunner.runChild(ArchUnitRunner.java:63)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at com.tngtech.archunit.junit.ArchUnitRunner$1.evaluate(ArchUnitRunner.java:88)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:89)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:41)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:541)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:763)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:463)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:209)

While debugging I noticed the created JavaMethod for Foo::bar reflects the correct method (not the bridge one). But the JavaMethod annotations is empty (probably it is reading them from the bridge method).

The difference from the method and its bridge is the return type: Foo and AbstractFoo, respectively

[codecholeric]

Hmm, this is really weird, because this test does behave differently when I execute it, I only get one (correct) failure and when I look at the bytecode, I see

javap -v FooBarTest\$Foo.class

java.lang.Object bar();
    descriptor: ()Ljava/lang/Object;
    flags: (0x1040) ACC_BRIDGE, ACC_SYNTHETIC
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokevirtual #2                  // Method bar:()Lcom/tngtech/archunit/exampletest/FooBarTest$Foo;
         4: areturn
      LineNumberTable:
        line 34: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       5     0  this   Lcom/tngtech/archunit/exampletest/FooBarTest$Foo;
    RuntimeVisibleAnnotations:
      0: #18()
        java.lang.Deprecated

I.e. the synthetic method has the same annotation as the overridden method. This might be JDK specific, I compiled it with an Oracle JDK 10 and an Open JDK 11 and got the same result though.

It seems to me, that what you get when you call JavaClass.getMethod(..) might indeed not be deterministic, because internally it simply looks for a method declared within the class that has a matching name and signature. If your generic type is not part of the signature (like bar()), then both methods match equally well ☹️ So that should definitely be fixed, i.e. to skip synthetic methods here.

[ratoaq2]

I'm using oracle Java 8 in Windows 10 (work env). I'll update you with the exact version later today.

I'm compiling and running it inside eclipse 2018-09 if I'm not mistaken. It might well be that eclipse compiler doesn't annotate the bridge method. Anyway I'll give you more info later today

[ratoaq2]

So, my environmnet is:

OS Name:                   Microsoft Windows 10 Enterprise
OS Version:                10.0.16299 N/A Build 16299

java version "1.8.0_221"
Java(TM) SE Runtime Environment (build 1.8.0_221-b11)
Java HotSpot(TM) 64-Bit Server VM (build 25.221-b11, mixed mode)

Eclipse Java EE IDE for Web Developers.
Version: 2018-09 (4.9.0)
Build id: 20180917-1800

The compiled class (Using eclipse compiler)

javap.exe" -v FooBarTest$Foo.class
Classfile /C:/projects/workspaces/myproject/test-common/target/test-classes/com/mypackage/archunit/FooBarTest$Foo.class
  Last modified Aug 14, 2019; size 989 bytes
  MD5 checksum 929850eb7f4bf3d742bf1dbc4879e073
  Compiled from "FooBarTest.java"
public class com.mypackage.archunit.FooBarTest$Foo extends com.mypackage.archunit.FooBarTest$AbstractFoo<com.mypackage.archunit.FooBarTest$Foo>
  minor version: 0
  major version: 52
  flags: ACC_PUBLIC, ACC_SUPER
Constant pool:
   #1 = Class              #2             // com/mypackage/archunit/FooBarTest$Foo
   #2 = Utf8               com/mypackage/archunit/FooBarTest$Foo
   #3 = Class              #4             // com/mypackage/archunit/FooBarTest$AbstractFoo
   #4 = Utf8               com/mypackage/archunit/FooBarTest$AbstractFoo
   #5 = Utf8               <init>
   #6 = Utf8               ()V
   #7 = Utf8               Code
   #8 = Methodref          #3.#9          // com/mypackage/archunit/FooBarTest$AbstractFoo."<init>":()V
   #9 = NameAndType        #5:#6          // "<init>":()V
  #10 = Utf8               LineNumberTable
  #11 = Utf8               LocalVariableTable
  #12 = Utf8               this
  #13 = Utf8               Lcom/mypackage/archunit/FooBarTest$Foo;
  #14 = Utf8               bar
  #15 = Utf8               ()Lcom/mypackage/archunit/FooBarTest$Foo;
  #16 = Utf8               Deprecated
  #17 = Utf8               RuntimeVisibleAnnotations
  #18 = Utf8               Ljava/lang/Deprecated;
  #19 = Utf8               thisIsFine
  #20 = Utf8               validFailure
  #21 = Utf8               ()Ljava/lang/Object;
  #22 = Methodref          #1.#23         // com/mypackage/archunit/FooBarTest$Foo.bar:()Lcom/mypackage/archunit/FooBarTest$Foo;
  #23 = NameAndType        #14:#15        // bar:()Lcom/mypackage/archunit/FooBarTest$Foo;
  #24 = Utf8               SourceFile
  #25 = Utf8               FooBarTest.java
  #26 = Utf8               Signature
  #27 = Utf8               Lcom/mypackage/archunit/FooBarTest$AbstractFoo<Lcom/mypackage/archunit/FooBarTest$Foo;>;
  #28 = Utf8               InnerClasses
  #29 = Class              #30            // com/mypackage/archunit/FooBarTest
  #30 = Utf8               com/mypackage/archunit/FooBarTest
  #31 = Utf8               AbstractFoo
  #32 = Utf8               Foo
{
  public com.mypackage.archunit.FooBarTest$Foo();
    descriptor: ()V
    flags: ACC_PUBLIC
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokespecial #8                  // Method com/mypackage/archunit/FooBarTest$AbstractFoo."<init>":()V
         4: return
      LineNumberTable:
        line 27: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       5     0  this   Lcom/mypackage/archunit/FooBarTest$Foo;

  com.mypackage.archunit.FooBarTest$Foo bar();
    descriptor: ()Lcom/mypackage/archunit/FooBarTest$Foo;
    flags:
    Deprecated: true
    RuntimeVisibleAnnotations:
      0: #18()
    Code:
      stack=1, locals=1, args_size=1
         0: aconst_null
         1: areturn
      LineNumberTable:
        line 32: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       2     0  this   Lcom/mypackage/archunit/FooBarTest$Foo;

  void thisIsFine();
    descriptor: ()V
    flags:
    Deprecated: true
    RuntimeVisibleAnnotations:
      0: #18()
    Code:
      stack=0, locals=1, args_size=1
         0: return
      LineNumberTable:
        line 37: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       1     0  this   Lcom/mypackage/archunit/FooBarTest$Foo;

  void validFailure();
    descriptor: ()V
    flags:
    Code:
      stack=0, locals=1, args_size=1
         0: return
      LineNumberTable:
        line 40: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       1     0  this   Lcom/mypackage/archunit/FooBarTest$Foo;

  java.lang.Object bar();
    descriptor: ()Ljava/lang/Object;
    flags: ACC_BRIDGE, ACC_SYNTHETIC
    Deprecated: true
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokevirtual #22                 // Method bar:()Lcom/mypackage/archunit/FooBarTest$Foo;
         4: areturn
      LineNumberTable:
        line 1: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
}
SourceFile: "FooBarTest.java"
Signature: #27                          // Lcom/mypackage/archunit/FooBarTest$AbstractFoo<Lcom/mypackage/archunit/FooBarTest$Foo;>;
InnerClasses:
     static abstract #31= #3 of #29; //AbstractFoo=class com/mypackage/archunit/FooBarTest$AbstractFoo of class com/mypackage/archunit/FooBarTest
     public static #32= #1 of #29; //Foo=class com/mypackage/archunit/FooBarTest$Foo of class com/mypackage/archunit/FooBarTest

[codecholeric]

Looks like good ol' JDK 8 does not put RuntimeVisibleAnnotations on bridge methods, while later JDKs do 😉 Guess I've learned something new there.
I don't think there is an easy way to take out the bridge method in general (because it caused all kinds of trouble when I tried that in #87), but we should definitely make it easy to filter those out by adding the respective modifiers to JavaModifier and parse them from the bytecode.
Also the behavior of JavaMethod.reflect() should be fixed / made deterministic.

[codecholeric]

Since #243 solves a big part of the leftovers here, I've created #256 as a follow up so we can close this issue