Comment 1 Andrey Loskutov 2019-03-21 02:23:03 EDT

Larry, I assume this happens only with one file? If yes, could you attach it? If not, which Eclipse version do you have?

Comment 2 larry 2019-03-21 04:12:42 EDT

Created attachment 277943 [details]
e.class

Comment 3 larry 2019-03-21 04:16:49 EDT

(In reply to Andrey Loskutov from comment #1)
> Larry, I assume this happens only with one file? If yes, could you attach
> it? If not, which Eclipse version do you have?

4.10 and 4.11 are same, see attachment.

Comment 4 Andrey Loskutov 2019-03-21 05:33:46 EDT

Larry, who has generated e.class? It looks like it reports only 2 parameters but the signature shows 3:

  // access flags 0xA
  private static a(Ljava/util/function/Function;Ljava/lang/Object;J)Z
    // annotable parameter count: 2 (invisible)
    @Lorg/jetbrains/annotations/NotNull;() // invisible, parameter 1


The similar code below shows this bytecode:

  // access flags 0xA
  private static a(Ljava/util/function/Function;Ljava/lang/Object;J)Z
    // annotable parameter count: 3 (invisible)
    @LNotNull;() // invisible, parameter 1

If we remove the "long" parameter, we will get:

  // access flags 0xA
  private static a(Ljava/util/function/Function;Ljava/lang/Object;)Z
    // annotable parameter count: 2 (invisible)
    @LNotNull;() // invisible, parameter 1


So for me this looks like an error in the obfuscator who generated e.class, which doesn't properly record parameters count for annotations. This is exact what Stephan commented on bug 474081 comment 1.

Below similar code trying to match the related part of the broken class file.

-----------------------------------------------------------

import java.lang.annotation.Documented;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.util.function.Function;


public class X {

	private static boolean a(Function f, @NotNull Object o, long l) {
		return false;
	}
}

@Documented
@Retention(RetentionPolicy.CLASS)
@Target({ElementType.FIELD,ElementType.METHOD,ElementType.PARAMETER,ElementType.LOCAL_VARIABLE})
@interface NotNull {
}

Comment 5 larry 2019-03-21 07:50:42 EDT

(In reply to Andrey Loskutov from comment #4)
> Larry, who has generated e.class? It looks like it reports only 2 parameters

Yes, exactly, this class generated by an obfuscator. I don't know who is.
In fact this file is part of IDEA, Maybe nobody might call this method, but it works fine.
This file parsed well in Structure of IDEA. In eclipse, package explorer, outline, debug view and so on couldn’t work. So can eclipse ignore this error method?

Comment 6 Stephan Herrmann 2019-03-21 08:10:49 EDT

Thanks larry and Andrey for details and analysis. So the detailed error message from bug 474081 indeed helps to identify the offending method. It can be confirmed that the class file violates JVMS. Ergo the bug should primarily be reported against the tool that created the class file.

I'm closing this bug as WORKSFORME.

If anyone feels inclined to provide a clean workaround on JDT side, I will tentatively welcome this, but in that case my expectation would be: the illegal class file must be reported in some suitable why rather than silently be swallowed. The code location that currently raises the exception does not have enough context to raise a regular compile error, so it would be tricky to say the least to find a way how to turn the exception into a normal compile error.

Comment 7 larry 2019-03-21 08:27:19 EDT

(In reply to Stephan Herrmann from comment #6)
> Thanks larry and Andrey for details and analysis. So the detailed error
> message from bug 474081 indeed helps to identify the offending method. It
> can be confirmed that the class file violates JVMS. Ergo the bug should
> primarily be reported against the tool that created the class file.
> 
> I'm closing this bug as WORKSFORME.
> 
> If anyone feels inclined to provide a clean workaround on JDT side, I will
> tentatively welcome this, but in that case my expectation would be: the
> illegal class file must be reported in some suitable why rather than
> silently be swallowed. The code location that currently raises the exception
> does not have enough context to raise a regular compile error, so it would
> be tricky to say the least to find a way how to turn the exception into a
> normal compile error.

This class can be reflected, so it is a LEGAL class file and NOT violates JVMS.

Comment 8 larry 2019-03-21 08:36:47 EDT

This class can be reflected, so it is a LEGAL class file and NOT violates JVMS.

Comment 9 Andrey Loskutov 2019-03-21 08:43:14 EDT

(In reply to larry from comment #8)
> This class can be reflected, so it is a LEGAL class file and NOT violates
> JVMS.

Not sure what do you mean by reflected, but the bytecode of this class violates specs.

@Stephan, I have a patch that will avoid error dialogs and only report errors to the log and also will show the "error" editor part. I will push it in a second.

Comment 10 Stephan Herrmann 2019-03-21 08:50:42 EDT

(In reply to larry from comment #8)
> This class can be reflected, so it is a LEGAL class file and NOT violates
> JVMS.

This is unsound reasoning.

To prove that the class is legal you'd have to argue how it conforms to the following from JVMS (§4.7.18/19):

"The value of the num_parameters item gives the number of formal parameters of the method represented by the method_info structure on which the annotation occurs.

This duplicates information that could be extracted from the method descriptor."

Interestingly, the last sentence was changed in version 10:

There is no assurance that this number is the same as the number of parameter descriptors in the method descriptor.

Comment 11 larry 2019-03-21 08:58:25 EDT

(In reply to Andrey Loskutov from comment #9)
> (In reply to larry from comment #8)
> > This class can be reflected, so it is a LEGAL class file and NOT violates
> > JVMS.
> 
> Not sure what do you mean by reflected, but the bytecode of this class
> violates specs.
> 
> @Stephan, I have a patch that will avoid error dialogs and only report
> errors to the log and also will show the "error" editor part. I will push it
> in a second.

This class can load by Class.forName().
I can call  getDeclaredMethods().
It seems that JVM thought is a legal class.

Comment 12 Andrey Loskutov 2019-03-21 09:00:49 EDT

(In reply to Stephan Herrmann from comment #10)
> Interestingly, the last sentence was changed in version 10:
> 
> There is no assurance that this number is the same as the number of
> parameter descriptors in the method descriptor.

Does it mean, we should be able to parse this class file now?

Comment 13 Stephan Herrmann 2019-03-21 09:02:53 EDT

Apparently, the spec change was driven by https://bugs.openjdk.java.net/browse/JDK-8067975 (which has several links to related bugs). 

I'm not a aware of any JSR/JEP that covers this. In particular the comment "However, javac has never emitted Runtime[In]VisibleParameterAnnotations attributes that follow the method descriptor." seems to indicate that they silently adjusted the spec to javac's behavior. This style of changing the spec underfoot is dangerous for any effort to keep ecj in line with the spec.

Anyway, the solution to the bug at hand is given further down in latest JVMS (12):


"The i'th entry in the parameter_annotations table may, but is not required to, correspond to the i'th parameter descriptor in the method descriptor (§4.3.3).

For example, a compiler may choose to create entries in the table corresponding only to those parameter descriptors which represent explicitly declared parameters in source code. In the Java programming language, a constructor of an inner class is specified to have an implicitly declared parameter before its explicitly declared parameters (JLS §8.8.1), so the corresponding <init> method in a class file has a parameter descriptor representing the implicitly declared parameter before any parameter descriptors representing explicitly declared parameters. If the first explicitly declared parameter is annotated in source code, then a compiler may create parameter_annotations[0] to store annotations corresponding to the second parameter descriptor."

This needs to be implemented in ecj.

Comment 14 Stephan Herrmann 2019-03-21 09:04:17 EDT

(In reply to larry from comment #11)
> It seems that JVM thought is a legal class.

Also JVM is just a piece of software which may have bugs.

(In reply to Andrey Loskutov from comment #12)
> Does it mean, we should be able to parse this class file now?

Yep, that's how I read the latest JVMS.

Comment 15 Manoj N Palat 2019-08-27 01:14:22 EDT

Bulk move to 4.14

Comment 16 Manoj N Palat 2019-08-27 02:06:27 EDT

Bulk move out of 4.13

Comment 17 larry 2020-01-09 20:41:44 EST

A year has passed, how about this?

Comment 18 Andrey Loskutov 2020-01-10 04:17:25 EST

(In reply to larry from comment #17)
> A year has passed, how about this?

https://wiki.eclipse.org/Platform/How_to_Contribute ?

Comment 19 Olivier Thomann 2020-01-10 10:40:17 EST

It looks like you can have less parameter annotations than the number of parameters. Instead of adding lots of check, we could simply handle the AIOOBE by returning null which would be equivalent to no annotation on the last parameters.

Comment 20 Olivier Thomann 2020-06-26 11:55:12 EDT

I will prepare a patch for this one.

Comment 21 Eclipse Genie 2022-06-30 19:41:43 EDT

This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.