Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail to detect cycles when the depending variable is final or in interface #298

Closed
ceachother opened this issue Jan 22, 2020 · 7 comments
Closed

Comments

@ceachother
Copy link

ceachother commented Jan 22, 2020

I'm at 0.13.0.

When I have 2 class:

public class ClassA {
    public static final String TEST = "test";
    public void test() {
        System.out.println(ClassB.TEST);
    }
}

public class ClassB {
    public static final String TEST = "test";
    public void test() {
        System.out.println(ClassA.TEST);
    }
}

Theslices().matching("pkg.(**)..").should().beFreeOfCycles(); won't report any violations.

Is this a bug or you guys made the decision intentionally?

@ceachother ceachother changed the title beFreeOfCycles() fail to detect cycle when the depending variable is final or in interface Fail to detect cycles when the depending variable is final or in interface Jan 22, 2020
@hankem
Copy link
Member

hankem commented Jan 22, 2020

ArchUnit currently does not detect such a dependency because it is not (so directly) present in the bytecode: a static final primitive or String is just inlined in the constant pool of the class.

When I change your example constants to testA / testB to make them distinct,
javap -c- p -v ClassA.class shows

this bytecode
public class ClassA
  minor version: 0
  major version: 55
  flags: (0x0021) ACC_PUBLIC, ACC_SUPER
  this_class: #6                          // ClassA
  super_class: #7                         // java/lang/Object
  interfaces: 0, fields: 1, methods: 2, attributes: 1
Constant pool:
   #1 = Methodref          #7.#19         // java/lang/Object."<init>":()V
   #2 = Fieldref           #20.#21        // java/lang/System.out:Ljava/io/PrintStream;
   #3 = Class              #22            // ClassB
   #4 = String             #23            // testB
   #5 = Methodref          #24.#25        // java/io/PrintStream.println:(Ljava/lang/String;)V
   #6 = Class              #26            // ClassA
   #7 = Class              #27            // java/lang/Object
   #8 = Utf8               TEST
   #9 = Utf8               Ljava/lang/String;
  #10 = Utf8               ConstantValue
  #11 = String             #28            // testA
  #12 = Utf8               <init>
  #13 = Utf8               ()V
  #14 = Utf8               Code
  #15 = Utf8               LineNumberTable
  #16 = Utf8               test
  #17 = Utf8               SourceFile
  #18 = Utf8               ClassA.java
  #19 = NameAndType        #12:#13        // "<init>":()V
  #20 = Class              #29            // java/lang/System
  #21 = NameAndType        #30:#31        // out:Ljava/io/PrintStream;
  #22 = Utf8               ClassB
  #23 = Utf8               testB
  #24 = Class              #32            // java/io/PrintStream
  #25 = NameAndType        #33:#34        // println:(Ljava/lang/String;)V
  #26 = Utf8               ClassA
  #27 = Utf8               java/lang/Object
  #28 = Utf8               testA
  #29 = Utf8               java/lang/System
  #30 = Utf8               out
  #31 = Utf8               Ljava/io/PrintStream;
  #32 = Utf8               java/io/PrintStream
  #33 = Utf8               println
  #34 = Utf8               (Ljava/lang/String;)V
{
  public static final java.lang.String TEST;
    descriptor: Ljava/lang/String;
    flags: (0x0019) ACC_PUBLIC, ACC_STATIC, ACC_FINAL
    ConstantValue: String testA

  public ClassA();
    descriptor: ()V
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokespecial #1                  // Method java/lang/Object."<init>":()V
         4: return
      LineNumberTable:
        line 1: 0

  public void test();
    descriptor: ()V
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=2, locals=1, args_size=1
         0: getstatic     #2                  // Field java/lang/System.out:Ljava/io/PrintStream;
         3: ldc           #4                  // String testB
         5: invokevirtual #5                  // Method java/io/PrintStream.println:(Ljava/lang/String;)V
         8: return
      LineNumberTable:
        line 4: 0
        line 5: 8
}

(Likewise for ClassB.)

So ClassA's dependency on ClassB.TEST in public void test() just becomes a ldc #4, which is a copy of the string "testB" in ClassA's own constant pool.

In #168, it was argued that such a dependency is not the most harmful one.

When I however change ClassB to have a

public static final Object TEST = new Object();

ClassB.TEST really becomes a

getstatic     #3                  // Field ClassB.TEST:Ljava/lang/Object;

Could you confirm that your ArchUnit test would catch such a case as you'd expect?

@hankem
Copy link
Member

hankem commented Jan 22, 2020

FYI: The dependency is actually present in ClassA's constant pool.

   #3 = Class              #22            // ClassB
   #4 = String             #23            // testB

So ArchUnit could, in principle, be extended to even catch those dependencies at some point.

@ceachother
Copy link
Author

I tried this and it still doesn't treat it as a cycle. Once I removed both of the final it can find the cycle.

public class ClassA {
    public static final String STRINGA = "test";
    public void test() {
        System.out.println(ClassB.OBJECTB);
    }
}

public class ClassB {
    public static final Object OBJECTB = new Object();
    public void test() {
        System.out.println(ClassA.STRINGA);
    }
}

@ceachother
Copy link
Author

That's a good thing to hear!
I guess my original intension was to look for a potential configuration can turn this on, but seems this can be a new feature to implement.

@hankem
Copy link
Member

hankem commented Jan 23, 2020

Sorry for being not explicit enough! 😉
I'd expect that only changing ClassB to expose such a new Object() constant will only give ArchUnit (as of 0.13.0) a dependency of ClassA on ClassB. To actually get a cycle, you'd also have to change ClassA: in your last example, ClassA's String constant is still just inlined in ClassB, therefore ClassB does not seem to depend on ClassA.

If you don't use final, then your fields might not be constant, so the compiler cannot just move them in a class file's constant pool anymore.

To maybe illustrate this even better, consider the following example:

class Dependency {
    static final int INLINABLE_CONSTANT = 42;  // primitive
    static final Integer NON_INLINABLE_CONSTANT = 42;  // Object
}

class ClassWithOnlyCompileTimeDependency {
   public static void main(String...args) {
     System.out.println(Dependency.INLINABLE_CONSTANT);
   }
}

class ClassWithCompileAndRuntimeDependency {
   public static void main(String...args) {
     System.out.println(Dependency.NON_INLINABLE_CONSTANT);
   }
}

If you compile those and delete Dependency.class,

  • java ClassWithOnlyCompileTimeDependency still runs successfully, but
  • java ClassWithCompileAndRuntimeDependency gives you a

    java.lang.NoClassDefFoundError: Dependency

@ceachother
Copy link
Author

Thanks Manfred!

@codecholeric
Copy link
Collaborator

I've opened #309 with the gist of this issue 😉
Thus I'll close this issue for now, because I think the reason for your problem is clear now and there is at the moment no workaround with ArchUnit to detect a dependency that is only observable through the constant pool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants