Bug ID: JDK-8334871 javac does not accept classfiles with certain permitted RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes

JDK-8334871 : javac does not accept classfiles with certain permitted RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes

Type: CSR
Component: tools
Sub-Component: javac

Priority: P4
Status: Closed
Resolution: Approved
Fix Versions: 24

Submitted: 2024-06-24
Updated: 2024-09-06
Resolved: 2024-09-04

Related Reports

CSR :	JDK-8334870 - javac does not accept classfiles with certain permitted RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes
Relates :	JDK-8303112 - For classes declared in instance methods, constructor.getParameterAnnotations does not include implicit enclosing param

Description

Summary
-------

javac does not accept some classfiles which contain valid RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes, as specified in JVMS 4.7.18 and 4.7.19.


Problem
-------

The RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes, as specified in JVMS 4.7.18 and 4.7.19 do not currently put any requirements on the number of entries in the attributes, or even the ordering of the entries. javac, however, only accepts classfiles where the number of entries in the attributes match its internal parameter count for the given method. javac rejects all other classfiles.

_Historical considerations:_

In JDK 8 and below, the attributes were specified as:

> The i'th entry in the table corresponds to the i'th formal parameter in the method descriptor (§4.3.3).

https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.7.18

javac was not implementing the specification correctly in two aspects:

 - when writing a classfile, when the `Signature` attribute was used for a method, it was only producing as many entries of these attributes as there were parameters in the Signature attribute
 - when reading a classfile, if the `Signature` attribute was present for the method, javac expected the attribute to have exactly as many entries as there were parameters in the Signature attribute

In JDK 9, the wording for these attributes has been changed to:

> 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).

https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-4.html#jvms-4.7.18

This change made the classfile written by javac legal, but also made many other forms of the classfiles legal. javac was never adjusted to this change.

Solution
--------

Regardless of the number of entries in the RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes, javac will never produce an error.

As the wording in the specification does not allow to reliably map the annotations to the parameters, it is proposed javac will use a number of heuristics to do the mapping. If none of these heuristics will yield a result, javac will ignore the attributes.

Note the heuristics below are intended to not change result for any classfiles which javac can read without errors currently. They only add new cases which are accepted by javac.

The proposal herein only concerns javac. Other parts of the JDK (notably the core reflection) may need to be adjusted separately, and are outside of the scope of this change.

Specification
-------------

The proposed heuristics are (if at least one of RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes is present):

 - if both RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes are present, javac will ignore them both with a warning unless they have the same number of entries
 - if, for the given method, there is the `Signature` attribute, and the number of entries in the attributes is the same as the number of parameters in the `Signature` attribute, the entries in the attributes will be used in order to fill the parameters from the `Signature` attribute,
 - otherwise, if for the given method, there is no `Signature` attribute, if the method is a constructor, and the class is heuristically determined to be an inner class (the class is a non-static inner class and is determined to be neither an anonymous class, nor a local class), the first parameter is stripped, and if the number of parameters of this adjusted type is the same as the number of entries in the attributes, the entries in the attributes will be used in order to fill the parameters from this adjusted type,
 - otherwise, if for the given method, there is no `Signature` attribute and if the method is not a constructor of a heuristically determined innerclass, and the number of entries in the attributes is the same as the number of parameters in the method's descriptor, the entries in the attributes will be used in order to fill the parameters from method's descritor,
 - otherwise, if there is `MethodParameters` attribute for the method, and the number of non-synthetic and non-mandatory parameters as per the `MethodParameters` attribute is the same as the number of entries in the RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes, the entries in the RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes will be used in order to fill the annotations on the non-synthetic and non-mandated parameters,
 - otherwise, if there is `MethodParameters` attribute for the method, and the number parameters in the method's descritor is the same as the number of entries in the RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes, the entries in the RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes that correspond to either synthetic and mandated parameters will be ignored, and the others will be used in order to fill the annotations on the non-synthetic and non-mandated parameters,
 - otherwise, if there is no `MethodParameters`, the current method is a constructor for an enum types, there is no `Signature` attribute and the number of parameters in the method's descriptor minus 2 is the same as the number of entries in the the RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes, the entries in the RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes will be used in order to fill the annotations for the parameters starting from the 2nd parameter,
 - otherwise, if the given classfile represents an (direct or indirect) local class, and there is no `Signature` attribute, and the number of parameters in the method's descriptor is greater than the number of entries in the the RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes, the entries in the RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes will be used in order to fill the annotations for the parameters starting from the 1st parameter.

javac will never produce an error based on the number of entries in the RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes. javac will produce warnings if:

 - the number of entries in the RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations attributes differ
 - none of the above heuristics applies

Comments

Thanks for the approval. I wrote a release note here: https://bugs.openjdk.org/browse/JDK-8339641 please let me know if it should be adjusted. Thanks!
06-09-2024
Moving to Approved, contingent on a release not being written for the parent issue. The heuristics look reasonable.
04-09-2024
This looks good for now; a future attempt to unify parameter processing will most likely introduce a new rule that precedes all rules on both javac and core reflection. We can drop some of these rules when we drop support of old releases in javac (as these rules are never part of JVMS)
30-08-2024
In the list of rules above, the rules before `MethodParameters` are considered are basically to replicate existing behavior (for classfiles javac can read currently). It is extremely difficult to quantify what would be the impact of "prefering `MethodParameters`" - it is unclear what combinations of attributes are in the existing classfiles produced no only by javac historically, but also by other compilers. (Also, `Signature` is absolutely crucial for compilers, and in case of some kind of mismatch between the content of `MethodParameters` and `Signature`, it is not quite clear why `MethodParameters` should be preferred.) Also please note that the current problem is not caused by any mismatch between `MethodParameters` and `Signature`. It is solely about interpretation of what `Runtime(In)VisibleParameterAnnotations` attribute means in cases which javac cannot currently handle. It is not clear that the potential to introduce potential changes in behavior is warranted here.
03-07-2024
Regarding core reflection behavior around parameter annotations: 1. Core reflection retrieves the annotations from the attribute as-is, producing a 2-dimensional array. 2. If the annotation count is equal to all formal parameter count, core reflection proceeds. (This is probably why ecj emits a full list of formal parameters, which is compatible with core reflection) 3. Else, some methods have known explicit formal parameter and implicit/synthetic formal parameter mismatches: handleParameterNumberMismatch handles these cases if the implicit/synthetic parameters are in leading positions only. For example, inner/nested member classes or enums can match, while core reflection rejects matching for local and anonymous class constructors, which can have extra synthetic parameters performing local variable captures in the trailing positions. 4. If the results match, core reflection returns the matched parameter annotations corresponding to all formal parameters; it returns as-is for local/anonymous classes for user to perform a best-guess. Otherwise, it throws an AnnotationFormatError, such as for Enum constructors without 2 leading synthetic parameters (which can be generated by other compilers) A note about core reflection is that the annotations are only retrieved lazily, and if not queried, the error will never be thrown. This is the most significant difference from the current behavior in javac. It would be the closet match if javac still errors, but only if the parameter annotation is actually queried (which might be impossible if the annotations are used in equality comparisons) In the case from [~jlahoda], the path of "give up on local class" is taken, and core reflection returns the bytecode attribute's parsing result as is. This means that the number of annotation elements available is only the number of explicit formal parameters (1) without the implicit leading receiver. Parameter expects this returned array to cover all formal parameters, and thus gets an AIOOBE. (We can switch Parameter off to a package-private API like Executable.getAllGenericParameterTypes to fix the problem in Parameter, and we should encourage users to check Parameter for reliable and correct behavior without backward compatibility concerns) It is tracked at JDK-8303112.
02-07-2024
Regarding core reflection's handling of mismatches in `RuntimeVisibleTypeAnnotations`, `RuntimeVisibleAnnotations`, `Signature`, `MethodParameters`: Core reflection aims to use `MethodParameters` and the associated `java.lang.reflect.Parameter` API as the standard. If any of the other 2 attributes mismatch `MethodParameters` in terms of element count, `Parameter` API should refuse to apply them onto individual parameters, treating as if they were absent. `RuntimeVisibleTypeAnnotations` is special that it carries no information about parameter count; so core reflection can only correctly apply it when `MethodParameters` is present; and it can only be applied on explicit formal parameters, unlike `RuntimeVisibleAnnotations` that can apply on implicit ones too if the full length matches. It would be helpful if javac can prefer `MethodParameters` over other attributes as the first source of truth and derive the explicit parameter count and location information. This can then be used to match signatures and annotations reliably. The behavior without MethodParameters can probably be kept as-is to preserve backward compatibility, especially with older versions that don't always produce this attribute (namely, versions before 21)
02-07-2024
[~darcy] to answer your questions: - yes, the set of class files read without errors should be a strict superset of the current class files - the effect on javax.lang.model API is likely to be only indirect - the intent is that for class files javac was able to read so far, the result should be unchanged. For class files javac wasn't able to read, there was no model, and there will be a model now. Yes, the intent is to make this change irrespective on the source level. The JVMS does not contain any restrictions on class file versions. Regarding core reflection, note that even current class files produced by javac may not be handled correctly by code reflection: ``` package test; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.reflect.Constructor; import java.lang.reflect.Parameter; import java.util.Arrays; public class Test { public static void main(String[] args) { new Test().test(); } private void test() { class Local { public Local(@Annotation int i) { } public String toString() { return Test.this.toString(); } } Constructor<?> c = new Local(0).getClass().getDeclaredConstructors()[0]; for (Parameter p : c.getParameters()) { System.err.print(Arrays.asList(p.getAnnotations())); System.err.print(" "); System.err.print(p.getType()); System.err.print(" "); System.err.println(p.getName()); } } @Retention(RetentionPolicy.RUNTIME) @interface Annotation {} } ``` ``` [@test.Test.Annotation()] class test.Test this$0 Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1 at java.base/java.lang.reflect.Parameter.getDeclaredAnnotations(Parameter.java:333) at java.base/java.lang.reflect.Parameter.getAnnotations(Parameter.java:373) at test.Test.test(Test.java:27) at test.Test.main(Test.java:12) ```
02-07-2024
Moving to Provisional, not Approved. [~liach], can you summarize how core reflection handles the cases discussed above? Thanks. [~jlahoda], to infer the effects of this change: * javac would succeed (in other words) not error out on a strictly larger set of class files * the annotations read by the javax.lang.model API would be improved Are those effects correct? Are there any other effects? As phrased, the intention is to make this change for all --source/--release compilation levels, right?
02-07-2024