Different inner class references generated for same code

[iloveeclipse]

Found in eclipse-platform/eclipse.platform.releng.aggregator#1557 (comment)

• Checkout org.eclipse.team.core source code
• Open org/eclipse/team/internal/core/subscribers/BatchingLock.java in the editor
• Go to BatchingLock.IFlushOperation.flush(ThreadInfo, IProgressMonitor) method
• Open Bytecode view
• Observe this code:

// class version 61.0 (61)
// access flags 0x601
public abstract interface BatchingLock$IFlushOperation {

  // compiled from: BatchingLock.java
  NESTHOST BatchingLock
  // access flags 0x609
  public static abstract INNERCLASS BatchingLock$IFlushOperation BatchingLock IFlushOperation

  // access flags 0x401
  public abstract flush(BatchingLock$ThreadInfo, IProgressMonitor) : void throws TeamException 
}

• "Touch" the class by adding an empty line somewhere.
• Unselect/select BatchingLock.IFlushOperation.flush(ThreadInfo, IProgressMonitor) method in the editor again
• Bytecode view should be updated
• Observe this code:

// class version 61.0 (61)
// access flags 0x601
public abstract interface BatchingLock$IFlushOperation {

  // compiled from: BatchingLock.java
  NESTHOST BatchingLock
  // access flags 0x609
  public static abstract INNERCLASS BatchingLock$IFlushOperation BatchingLock IFlushOperation
  // access flags 0x9
  public static INNERCLASS BatchingLock$ThreadInfo BatchingLock ThreadInfo

  // access flags 0x401
  public abstract flush(BatchingLock$ThreadInfo, IProgressMonitor) : void throws TeamException 
}

• Select org.eclipse.team.core project and do a "Clean Build".
• Original bytecode version is generated
• Difference is always this extra reference to the inner class which is generated or not based on compilation order/number of compiled classes:

  // access flags 0x9
  public static INNERCLASS BatchingLock$ThreadInfo BatchingLock ThreadInfo

TO BE

I would expect compiler generates the inner class reference only if it needed, and if it is needed, always generates this reference and not based on the compilation order or number of compiled classes.

[iloveeclipse]

@srikanth-sankaran : could you please check if that

• is something "easy to fix"?
• is this a severe bug (who needs that reference and what happens if it is not generated?)
• or is this same as the "lambda generation order" issue below?

I assume this is similar to another known ecj issue, where (numeric) lambda names are generated based on code visit order, see https://bugs.eclipse.org/bugs/show_bug.cgi?id=578351#c25.

[srikanth-sankaran]

Trying to come up with a standalone test is challenging so far, although I have been able to minimize the program size needed to reproduce:

package org.eclipse.team.internal.core.subscribers;

import org.eclipse.core.resources.IResource;
import org.eclipse.core.runtime.IProgressMonitor;

public class BatchingLock {	

	public static class ThreadInfo {
		
		public boolean isEmpty() {
			return true;
		}
		public IResource[] getChangedResources() {
			return null;
		}    
	}     
	
	public interface IFlushOperation {
		public void flush(ThreadInfo info, IProgressMonitor monitor);
	}	
}

This minimal version of BatchingLock.java in place, not as a standalone java program is enough to see the problem.

Bizarrely most any change you make to the minimal version above makes the problem disappear (E.g getting rid of the isEmpty method or changing its return type from boolean to void, providing a inlined version of the imported class IProgressMonitor etc.)

[srikanth-sankaran]

Problem arises from the differences between org.eclipse.jdt.internal.compiler.lookup.MethodBinding.signature() vs org.eclipse.jdt.internal.compiler.lookup.MethodBinding.signature(ClassFile) in updating (latter) and failing to update (former) the TagBits.ContainsNestedTypeReferences tagbit.

When doing a clean and full build, signature of org.eclipse.team.internal.core.subscribers.BatchingLock.IFlushOperation.flush(ThreadInfo, IProgressMonitor) gets computed with the former while in incremental build it is getting computed with the latter.

org.eclipse.jdt.internal.compiler.lookup.MethodBinding.signature() should be brought in line with its doppelganger or we need to see if the calls to it should really be going to org.eclipse.jdt.internal.compiler.lookup.MethodBinding.signature(ClassFile) as the javadoc seems to suggest (i.e This is the one that must be used during code generation.)

[srikanth-sankaran]

To address the question of whether the InnerClasses attribute entry for ThreadInfo is necessary: here is JVMS 4.7.6

If the constant pool of a class or interface C contains at least one
CONSTANT_Class_info entry (§4.4.1) which represents a class or interface that is
not a member of a package, then there must be exactly one InnerClasses attribute
in the attributes table of the ClassFile structure for C.

In the minimal example above (which pretty much retains the original form for IFlushOperation) the type involved is:

public interface IFlushOperation {
		public void flush(ThreadInfo info, IProgressMonitor monitor);
	}	

Will the constant pool of this type IFlushOperation have a CONSTANT_Class_info entry for ThreadInfo - the answer is no, ThreadInfo will be encoded as a part of the signature of the method in the form of a CONSTANT_Utf8_info structure only.

That said, it appears javac and ecj gloss over this point and generate an inner class attribute entry for more cases than strictly necessary.

See https://bugs.eclipse.org/bugs/show_bug.cgi?id=171184#c4

[srikanth-sankaran]

JVMS 4.7 says this:

Ten attributes are not critical to correct interpretation of the class file by the
Java Virtual Machine, but are either critical to correct interpretation of the
class file by the class libraries of the Java SE Platform, or are useful for tools
(in which case the section that specifies an attribute describes it as "optional"):

• Exceptions
• InnerClasses

...

[srikanth-sankaran]

Given:

interface IResource {}

interface IProgressMonitor {}

public class X {	

	public static class ThreadInfo {
		
		public boolean isEmpty() {
			return true;
		}
		public IResource[] getChangedResources() {
			return null;
		}    
	}      
	 
	public interface IFlushOperation {
		public void flush(ThreadInfo info, IProgressMonitor monitor);
		public void flush(String info, IProgressMonitor monitor);
	}
}

Javac from JDK21 generates this code:

Classfile /home/srikanth/shared-drives/C:/tmp/X$IFlushOperation.class
  Last modified 28-Nov-2023; size 317 bytes
  SHA-256 checksum 9e8d83d6c08d074dc614b38a3e8a7f10da93a49bf3c1668628ce0c1d4d74f73b
  Compiled from "X.java"
public interface X$IFlushOperation
  minor version: 0
  major version: 65
  flags: (0x0601) ACC_PUBLIC, ACC_INTERFACE, ACC_ABSTRACT
  this_class: #1                          // X$IFlushOperation
  super_class: #3                         // java/lang/Object
  interfaces: 0, fields: 0, methods: 2, attributes: 3
Constant pool:
   #1 = Class              #2             // X$IFlushOperation
   #2 = Utf8               X$IFlushOperation
   #3 = Class              #4             // java/lang/Object
   #4 = Utf8               java/lang/Object
   #5 = Utf8               flush
   #6 = Utf8               (LX$ThreadInfo;LIProgressMonitor;)V
   #7 = Utf8               (Ljava/lang/String;LIProgressMonitor;)V
   #8 = Utf8               SourceFile
   #9 = Utf8               X.java
  #10 = Utf8               NestHost
  #11 = Class              #12            // X
  #12 = Utf8               X
  #13 = Utf8               InnerClasses
  #14 = Utf8               IFlushOperation
  #15 = Class              #16            // X$ThreadInfo
  #16 = Utf8               X$ThreadInfo
  #17 = Utf8               ThreadInfo
{
  public abstract void flush(X$ThreadInfo, IProgressMonitor);
    descriptor: (LX$ThreadInfo;LIProgressMonitor;)V
    flags: (0x0401) ACC_PUBLIC, ACC_ABSTRACT

  public abstract void flush(java.lang.String, IProgressMonitor);
    descriptor: (Ljava/lang/String;LIProgressMonitor;)V
    flags: (0x0401) ACC_PUBLIC, ACC_ABSTRACT
}
SourceFile: "X.java"
NestHost: class X
InnerClasses:
  public static #14= #1 of #11;           // IFlushOperation=class X$IFlushOperation of class X
  public static #17= #15 of #11;          // ThreadInfo=class X$ThreadInfo of class X

The CONSTANT_Class_info for ThreadInfo comes about only because InnerClasses attributes needs it. It is not the case that InnerClasses entry for ThreadInfo gets emitted because there is otherwise a CONSTANT_Class_info for ThreadInfo

These are long standing behaviors - this attribute got introduced in Java 1.1 and behaviors are best left untouched unless absolutely needed. In this case, I think we should continue to emit an InnerClasses entry for ThreadInfo even though a pedantic interpretation of JVMS 4.7.6 would suggest it is not necessary

[srikanth-sankaran]

So here is a summary of what is going on: There are two signature methods on MethodBinding - one that gets invoked ahead of the other in a full build computes the signatures and caches it - this computation is deficient in not setting certain tag bits to alert subsequent stages that nested types are being referenced in the signature.

I have a standalone regression test that demonstrates the behavior.

Plausible fixes are - (a) to align the two signature methods wrt to flagging uses of nested classes (b) figure out if we can always use the "better" method.

This is under study.

[srikanth-sankaran]

@srikanth-sankaran : could you please check if that

• is something "easy to fix"?

I think so.

• is this a severe bug (who needs that reference and what happens if it is not generated?)

This behavior has been there for a long time. I tested 4.10 and it behaves the same way - I am fairly certain it goes back even many years more. Also given JVMS 4.7 wording cited above (Ten attributes are not critical to correct interpretation of the class file by the Java Virtual Machine and it includes InnerClasses) I would say not critical. But given the investment I have made spending nearly a day on it, I will wrap it up and include for 4.31 M1.

• or is this same as the "lambda generation order" issue below?

I assume this is similar to another known ecj issue, where (numeric) lambda names are generated based on code visit order, see https://bugs.eclipse.org/bugs/show_bug.cgi?id=578351#c25.

I don't think so except at an abstract level of a race/ordering influencing the outcome

[srikanth-sankaran]

@iloveeclipse, With this merge, we may see some comparator errors. Now clean + build and incremental build both emit two inner class entries for the problem scenario. For many many releases, we have seen only one entry for a full build. But emitting two entries aligns us with Javac.

Also to note: This fix backs out the code change brought in by https://bugs.eclipse.org/bugs/show_bug.cgi?id=575503 - the test brought in by that fix is still intact and passes without a problem even after the code change that supposedly fixes it is withdrawn. (The concerned fix does not look right to me)

[iloveeclipse]

@srikanth-sankaran : many thanks! Instabilities in environment are really bad, nice to eliminate another one.

With this merge, we may see some comparator errors

@akurtakov : I see there are many merges on aggregator today. Not sure what else you have planned, but in case you are done with aggregator, a new I build would be nice to see how much bundles would be affected because of compiler change here.

[akurtakov]

This commit lacks version bump for org.eclipse.jdt.core.tests.builder and fails master branch build - https://ci.eclipse.org/jdt/job/eclipse.jdt.core-Github/job/master/296/console . Please fix and once this is done just start an I-bulid.
Releng work is ongoing and will take more time but I would rather see comparator issue ASAP.

[iloveeclipse]

@akurtakov : I will push the version bump for JDT.

However there is another issue with PDE that bumped bundle version too much, see comment on https://github.com/eclipse-pde/eclipse.pde/pull/940/files.
That should be fixed before next IBuild.

[iloveeclipse]

Fixed this and PDE problem and triggered new I Build: https://ci.eclipse.org/releng/job/Builds/job/I-build-4.31/9/

[iloveeclipse]

we may see some comparator errors

Fortunately, we only have one single class changed, so not much trouble here.