Fix a bug in test case of RecursiveClinitTest

[quadhier]

This PR should fix the following failing test, which is a part of #274.

gov.nasa.jpf.test.vm.basic.RecursiveClinitTest::testNewInstance

I have tested locally and found that it fixes this test case with no more regressions (10 failing tests left now).

Problem Analysis

This is not a bug of JPF but instead a bug of the test case.

Running on JPF, this test case thows the following exception:

gov.nasa.jpf.vm.NoUncaughtExceptionsProperty
java.lang.AssertionError: instantiation failed with java.lang.NoSuchMethodException: gov.nasa.jpf.test.vm.basic.RecursiveClinitTest$Derived.<init>()
        at gov.nasa.jpf.util.test.TestJPF.fail(TestJPF.java:164)
        at gov.nasa.jpf.test.vm.basic.RecursiveClinitTest.testNewInstance(RecursiveClinitTest.java:72)
        at java.lang.reflect.Method.invoke(gov.nasa.jpf.vm.JPF_java_lang_reflect_Method)
        at gov.nasa.jpf.util.test.TestJPF.runTestMethod(TestJPF.java:648)

Derived has defined a constructor with an int parameter; there is no default constructor for it. So instead of calling getDeclaredConstructor() to get its constructor, getDeclaredConstructor(int.class) should be called. According to document of getDeclaredConstructor, in the case of not finding a matching constructor this behavior (throwing java.lang.NoSuchMethodException) is correct.

Furthermore, according to the document of newInstance, it also need to be called with an argument.

As evidence, running the following code (which is quite similar to the test case code) on OpenJDK 11,

public class Main {

  static class Base {
    static int d = 1;
    static {
      System.out.println("Base clinit");
    }
  }

  static class Derived extends Base {
    static int d = Base.d * 42;
    static {
      System.out.println("Derived clinit");
    }
    
    public Derived (int i){
      System.out.println("Derived(" + i + ')');
    }
    
    public static void foo(){
      System.out.println("Derived.foo()");
    }
  }

  void test() throws Exception {
    // NOTE this line
    Derived.class.getDeclaredConstructor();
  }

  public static void main (String[] args) throws Exception {
    new Main().test();
  }
}

generating

Exception in thread "main" java.lang.NoSuchMethodException: Main$Derived.<init>()
	at java.lang.Class.getConstructor0(Class.java:3082)
	at java.lang.Class.getDeclaredConstructor(Class.java:2178)
	at Main.test(Main.java:26)
	at Main.main(Main.java:30)

And running the following code (which is quite similar to the test case code) on OpenJDK 11,

public class Main {

  static class Base {
    static int d = 1;
    static {
      System.out.println("Base clinit");
    }
  }

  static class Derived extends Base {
    static int d = Base.d * 42;
    static {
      System.out.println("Derived clinit");
    }
    
    public Derived (int i){
      System.out.println("Derived(" + i + ')');
    }
    
    public static void foo(){
      System.out.println("Derived.foo()");
    }
  }

  void test() throws Exception {
    // NOTE this line
    Derived.class.getDeclaredConstructor(int.class).newInstance();
  }

  public static void main (String[] args) throws Exception {
    new Main().test();
  }
}

generating

Base clinit
Derived clinit
Exception in thread "main" java.lang.IllegalArgumentException: wrong number of arguments
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
	at Main.test(Main.java:26)
	at Main.main(Main.java:30)

Discussions

Why this test "passes" on master branch (on OpenJDK 8)?

The test case is not same with that onmaster branch. This is the case on master branch:

jpf-core/src/tests/gov/nasa/jpf/test/vm/basic/RecursiveClinitTest.java

Lines 68 to 70 in 3408119

	try {
	Derived.class.newInstance();
	} catch (Throwable t) {

It doesn't invoke getDeclaredConstructor() but instead uses .class expression, which avoid NoSuchMethodException. It calls java.lang.Class.newInstance() which is identical to call new Derived(). This should throw java.lang.NoSuchMethodException but it doesn't (the same thing happens on java-10-gradle branch, i.e., Derived.class.newInstance() passes). So there must be some bug with java.lang.Class.newInstance()'s implementations. I'll diagnose it later.

[cyrille-artho]

OK, please also create an issue (or a test) for the broken functionality, so we have a new issue (or unit test) to remind us of it.

[quadhier]

Of course. I'll do it.

[quadhier]

OK, please also create an issue (or a test) for the broken functionality, so we have a new issue (or unit test) to remind us of it.

Hi @cyrille-artho, I have created a new PR to fix the bug and add a test case (#353). BTW, according to Java SE API Spec, the correct exception to throw is java.lang.InstantiationException instead of java.lang.NoSuchMethodException I mentioned above.

[quadhier]

Hi, @cyrille-artho. I just looked into the test case a little bit more, and found that, instead of changing the test case testNewInstance() as this patch does, it might be better to add a default constructor to the class Derived and leave testNewInstance() as it is now. Because what this patch does seems to have been tested in the following test case.

jpf-core/src/tests/gov/nasa/jpf/test/vm/basic/RecursiveClinitTest.java

Lines 97 to 114 in 3408119

	@Test
	public void testCtorReflection (){
	if (verifyNoPropertyViolation()) {
	try {
	Class<?> clazz = Class.forName("gov.nasa.jpf.test.vm.basic.RecursiveClinitTest$Derived");
	System.out.println("main now creating Derived(-42)");
	Constructor ctor = clazz.getConstructor(new Class[] {int.class});
	Object o = ctor.newInstance( new Object[] {Integer.valueOf(-42)});
	System.out.println("back in main");
	assertTrue( o instanceof Derived);
	assertTrue(Derived.d == 42);
	} catch (Throwable t){
	fail("test failed with: " + t);
	}
	}
	}

If you agree with that, similar change might also apply to the master branch.

[cyrille-artho]

OK, thanks, I'll merge this PR.