dev tests failing locally?

[Colin Alworth]

I'm looking into fixing the javac.typemodel.JMethod creation to make it support static and default methods in Java8 interfaces so that generators (at least autobeans and requestfactory) can easily support categories without external classes (see https://github.com/gwtproject/gwt/issues/9289, I'm sure there are others). Along the way, I ran into a few unit tests that I apparently broke, and then discovered that these tests (at least locally) also consistently fail at current master. 

com.google.gwt.dev.resource.impl.ClassPathEntryTest

testResourceCreated

testResourceDeleted

testResourceRenamed

com.google.gwt.dev.resource.impl.ResourceAccumulatorTest

testDeleteFile

testMultipleListeners

testSymlinkInfiniteLoop

testSymlinks

testListensInNewDirectories

testRenameFile

testRenameDirectory

testRenameParentDirectory

testAddFile

My local tools dir is up to date, I'm running OS X 10.11.4, with ant 1.9.4 and jdk 1.8.0_60-b27. Are these tests known to be broken, or is there something likely wrong with my setup? I can share the stacktraces themselves, but all of these seem to be related to filesystem access. Is there something funny about these?

I do see notes about ClassPathEntryTest failing (common.ant.xml and dev/BUILD), but ResourceAccumulatorTest doesn't seem to have notes about it - but, it isn't added to a test suite. Are these tests broken in ant, but not excluded? Is there a better way to be running these aside from `ant test` to build sanely?

[Colin Alworth]

As an aside, the fix for the original bug seems to be very easy - the code that is building the JMethod instances seems to be overzealous about what modifiers must be applied. Aside from the constant failures, all compiler tests pass with this change:

diff --git a/dev/core/src/com/google/gwt/dev/javac/CompilationUnitTypeOracleUpdater.java b/dev/core/src/com/google/gwt/dev/javac/CompilationUnitTypeOracleUpdater.java
 
index f63e037..c926ec3 100644
--- a/dev/core/src/com/google/gwt/dev/javac/CompilationUnitTypeOracleUpdater.java
+++ b/dev/core/src/com/google/gwt/dev/javac/CompilationUnitTypeOracleUpdater.java
@@ -1121,10 +1121,6 @@ public class CompilationUnitTypeOracleUpdater extends TypeOracleUpdater {
     }
 
     addModifierBits(method, mapBits(ASM_TO_SHARED_MODIFIERS, methodData.getAccess()));
-    if (unresolvedType.isInterface() != null) {
-      // Always add implicit modifiers on interface methods.
-      addModifierBits(method, Shared.MOD_PUBLIC | Shared.MOD_ABSTRACT);
-    }
 
     if ((methodData.getAccess() & Opcodes.ACC_VARARGS) != 0) {
       setVarArgs(method);

With a little more testing, I'll submit this for review, along with a test that verifies that it has the desired behavior.

[Thomas Broyer]

I think this change is overzealous ;-)

> Every method declaration in the body of an interface is implicitly public (§6.6). It is permitted, but discouraged as a matter of style, to redundantly specify the public modifier for a method declaration in an interface.

> […]

> An interface method lacking a default modifier or a static modifier is implicitly abstract, so its body is represented by a semicolon, not a block. It is permitted, but discouraged as a matter of style, to redundantly specify the abstract modifier for such a method declaration.

— Source: https://docs.oracle.com/javase/specs/jls/se8/html/jls-9.html#jls-9.4

So I think, it should unconditionally add the MOD_PUBLIC bit, and conditionally add the MOD_ABSTRACT bit if neither MOD_STATIC nor MOD_DEFAULT is present.

[Colin Alworth]

There is no MOD_DEFAULT, at least not yet. I initially attempted to add one and tried to copy the same date from org.objectweb.asm, but their Opcodes type doesn't really have a field that seems to match (no ACC_DEFAULT, etc). If memory serves, bytecode doesn't hold this information, but just has the method marked as not-abstract, and the change to support default methods was in the java lang itself, not the jvm.

This works as expected - here is the test I had stubbed in to confirm that not setting MOD_ABSTRACT still results in it being set (via mapBits):

public interface Java8Interface {
  default int defaultImplMethod() {
    return 1;
  }
  static int staticImplMethod() {
    return 1;
  }
  int noImplMethod();
}

...
      JMethod method = type.getMethod("defaultImplMethod", noParamTypes);
      assertSame(JPrimitiveType.INT, method.getReturnType());
      assertEquals(0, method.getParameters().length);
      assertFalse(method.isStatic());
      assertFalse(method.isAbstract());


      method = type.getMethod("staticImplMethod", noParamTypes);
      assertSame(JPrimitiveType.INT, method.getReturnType());
      assertEquals(0, method.getParameters().length);
      assertTrue(method.isStatic());
      assertFalse(method.isAbstract());


      method = type.getMethod("noImplMethod", noParamTypes);
      assertSame(JPrimitiveType.INT, method.getReturnType());
      assertEquals(0, method.getParameters().length);
      assertFalse(method.isStatic());
      assertTrue(method.isAbstract());

I've not yet tested public to ensure that it is sane (though was hoping that other unit tests would be unhappy if it wasn't). Before I submit the change, I will check this as well, but the test is currently passing.

[Colin Alworth]

Confirmed, isPublic is true in all three cases, as expected, without the old code in place.