Commit 982fa265 authored by Eric Bruneton's avatar Eric Bruneton

Remove the workaround introduced for synthetic parameters.

A workaround was added in 4accca37 for issue #307392. The root cause of the bug was that, via a ClassReader->ClassWriter transform, the length of the parameter_annotations array in the Runtime[In]VisibleParameterAnnotations attribute was not preserved (the number read in ClassReader was ignored, and it was recomputed from the descriptor in MethodWriter).

The workaround added at this time was fixing that, and it was also attempting to report parameter indices corresponding to the source parameters, assuming that the implicit parameters were at added at the begining. However, synthetic parameters can also be added at the end (see issue #317788), so the second part of the above workaround does not work.

This change removes this workaround, and replaces it with a new visitAnnotableParameterCount method to preserve the size of the above array in a ClassReader->ClassWriter chain. In other words, this change still fixes the root cause of #307392 (closed), but no longer attempts to map bytecode parameter indices to source level parameter indices. We believe there is no universal method for doing that, but users can implement their own method, if desired, on top of the ASM API. Indeed, the JVMS spec states that (https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-4.html#jvms-4.7.18):

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

Note that the new visitMethod need not be called. By default (for instance for classes generated from scratch, without a ClassReader), the number of annotable parameters is the number of parameters in the method descriptor. This was also the case with the current code (i.e. when no ASM specific Ljava/lang/Synthetic; annotation was used to mark synthetic parameters). In theory, we should define a new API version, e.g. ASM_6_1, and implement the full backward compatibility mechanism for this new visit method, but I'm not sure it is worth doing so. What do you think?
parent 84b053be
Pipeline #300 passed with stage
in 6 minutes and 30 seconds
......@@ -125,6 +125,16 @@ public class MethodNode extends MethodVisitor {
*/
public Object annotationDefault;
/**
* The number of method parameters than can have runtime visible annotations. This number must be
* less or equal than the number of parameter types in the method descriptor (the default value 0
* indicates that all the parameters described in the method descriptor can have annotations). It
* can be strictly less when a method has synthetic parameters and when these parameters are
* ignored when computing parameter indices for the purpose of parameter annotations (see
* https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-4.html#jvms-4.7.18).
*/
public int visibleAnnotableParameterCount;
/**
* The runtime visible parameter annotations of this method. These lists are lists of {@link
* AnnotationNode} objects. May be <tt>null</tt>.
......@@ -134,6 +144,16 @@ public class MethodNode extends MethodVisitor {
*/
public List<AnnotationNode>[] visibleParameterAnnotations;
/**
* The number of method parameters than can have runtime invisible annotations. This number must
* be less or equal than the number of parameter types in the method descriptor (the default value
* 0 indicates that all the parameters described in the method descriptor can have annotations).
* It can be strictly less when a method has synthetic parameters and when these parameters are
* ignored when computing parameter indices for the purpose of parameter annotations (see
* https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-4.html#jvms-4.7.18).
*/
public int invisibleAnnotableParameterCount;
/**
* The runtime invisible parameter annotations of this method. These lists are lists of {@link
* AnnotationNode} objects. May be <tt>null</tt>.
......@@ -337,6 +357,15 @@ public class MethodNode extends MethodVisitor {
return an;
}
@Override
public void visitAnnotableParameterCount(final int parameterCount, final boolean visible) {
if (visible) {
visibleAnnotableParameterCount = parameterCount;
} else {
invisibleAnnotableParameterCount = parameterCount;
}
}
@Override
@SuppressWarnings("unchecked")
public AnnotationVisitor visitParameterAnnotation(
......@@ -729,6 +758,9 @@ public class MethodNode extends MethodVisitor {
TypeAnnotationNode an = invisibleTypeAnnotations.get(i);
an.accept(mv.visitTypeAnnotation(an.typeRef, an.typePath, an.desc, false));
}
if (visibleAnnotableParameterCount > 0) {
mv.visitAnnotableParameterCount(visibleAnnotableParameterCount, true);
}
n = visibleParameterAnnotations == null ? 0 : visibleParameterAnnotations.length;
for (i = 0; i < n; ++i) {
List<?> l = visibleParameterAnnotations[i];
......@@ -740,6 +772,9 @@ public class MethodNode extends MethodVisitor {
an.accept(mv.visitParameterAnnotation(i, an.desc, true));
}
}
if (invisibleAnnotableParameterCount > 0) {
mv.visitAnnotableParameterCount(invisibleAnnotableParameterCount, false);
}
n = invisibleParameterAnnotations == null ? 0 : invisibleParameterAnnotations.length;
for (i = 0; i < n; ++i) {
List<?> l = invisibleParameterAnnotations[i];
......
......@@ -601,6 +601,19 @@ public class ASMifier extends Printer {
return visitTypeAnnotation(typeRef, typePath, desc, visible);
}
@Override
public ASMifier visitAnnotableParameterCount(final int parameterCount, final boolean visible) {
buf.setLength(0);
buf.append(name)
.append(".visitAnnotableParameterCount(")
.append(parameterCount)
.append(", ")
.append(visible)
.append(");\n");
text.add(buf.toString());
return this;
}
@Override
public ASMifier visitParameterAnnotation(
final int parameter, final String desc, final boolean visible) {
......
......@@ -73,6 +73,18 @@ public class CheckMethodAdapter extends MethodVisitor {
/** The access flags of the method. */
private int access;
/**
* The number of method parameters that can have runtime visible annotations. 0 means that all the
* parameters from the method descriptor can have annotations.
*/
private int npanns;
/**
* The number of method parameters that can have runtime invisible annotations. 0 means that all
* the parameters from the method descriptor can have annotations.
*/
private int nipanns;
/** <tt>true</tt> if the visitCode method has been called. */
private boolean startCode;
......@@ -481,10 +493,25 @@ public class CheckMethodAdapter extends MethodVisitor {
return new CheckAnnotationAdapter(super.visitAnnotationDefault(), false);
}
@Override
public void visitAnnotableParameterCount(final int parameterCount, final boolean visible) {
checkEndMethod();
if (visible) {
npanns = parameterCount;
} else {
nipanns = parameterCount;
}
super.visitAnnotableParameterCount(parameterCount, visible);
}
@Override
public AnnotationVisitor visitParameterAnnotation(
final int parameter, final String desc, final boolean visible) {
checkEndMethod();
if ((visible && npanns > 0 && parameter >= npanns)
|| (!visible && nipanns > 0 && parameter >= nipanns)) {
throw new IllegalArgumentException("Invalid parameter index");
}
checkDesc(desc, false);
return new CheckAnnotationAdapter(super.visitParameterAnnotation(parameter, desc, visible));
}
......
......@@ -482,6 +482,20 @@ public abstract class Printer {
throw new RuntimeException("Must be overriden");
}
/**
* Number of method parameters that can have annotations. See {@link
* org.objectweb.asm.MethodVisitor#visitAnnotableParameterCount}.
*
* @param parameterCount the number of method parameters than can have annotations.
* @param visible <tt>true</tt> to define the number of method parameters that can have
* annotations visible at runtime, <tt>false</tt> to define the number of method parameters
* that can have annotations invisible at runtime.
* @return the printer
*/
public Printer visitAnnotableParameterCount(final int parameterCount, final boolean visible) {
throw new RuntimeException("Must be overriden");
}
/**
* Method parameter annotation. See {@link
* org.objectweb.asm.MethodVisitor#visitParameterAnnotation}.
......
......@@ -810,6 +810,16 @@ public class Textifier extends Printer {
return visitTypeAnnotation(typeRef, typePath, desc, visible);
}
@Override
public Textifier visitAnnotableParameterCount(final int parameterCount, final boolean visible) {
buf.setLength(0);
buf.append(tab2).append("// annotable parameter count: ");
buf.append(parameterCount);
buf.append(visible ? " (visible)\n" : " (invisible)\n");
text.add(buf.toString());
return this;
}
@Override
public Textifier visitParameterAnnotation(
final int parameter, final String desc, final boolean visible) {
......
......@@ -88,6 +88,13 @@ public final class TraceMethodVisitor extends MethodVisitor {
return new TraceAnnotationVisitor(av, p);
}
@Override
public void visitAnnotableParameterCount(
final int parameterCount, final boolean visible) {
p.visitAnnotableParameterCount(parameterCount, visible);
super.visitAnnotableParameterCount(parameterCount, visible);
}
@Override
public AnnotationVisitor visitParameterAnnotation(
final int parameter, final String desc, final boolean visible) {
......
......@@ -125,6 +125,7 @@ public class ASMContentHandler extends DefaultHandler implements Opcodes {
RULES.add("*/annotation", new AnnotationRule());
RULES.add("*/typeAnnotation", new TypeAnnotationRule());
RULES.add("*/annotableParameterCount", new AnnotableParameterCountRule());
RULES.add("*/parameterAnnotation", new AnnotationParameterRule());
RULES.add("*/insnAnnotation", new InsnAnnotationRule());
RULES.add("*/tryCatchAnnotation", new TryCatchAnnotationRule());
......@@ -1261,6 +1262,16 @@ public class ASMContentHandler extends DefaultHandler implements Opcodes {
}
}
final class AnnotableParameterCountRule extends Rule {
@Override
public void begin(final String name, final Attributes attrs) {
int parameterCount = Integer.parseInt(attrs.getValue("count"));
boolean visible = Boolean.valueOf(attrs.getValue("visible")).booleanValue();
((MethodVisitor) peek()).visitAnnotableParameterCount(parameterCount, visible);
}
}
final class AnnotationParameterRule extends Rule {
@Override
......
......@@ -356,6 +356,15 @@ public final class SAXCodeAdapter extends MethodVisitor {
sa, "typeAnnotation", visible ? 1 : -1, null, desc, typeRef, typePath);
}
@Override
public void visitAnnotableParameterCount(
final int parameterCount, final boolean visible) {
AttributesImpl attrs = new AttributesImpl();
attrs.addAttribute("", "count", "count", "", Integer.toString(parameterCount));
attrs.addAttribute("", "visible", "visible", "", visible ? "true" : "false");
sa.addElement("annotableParameterCount", attrs);
}
@Override
public AnnotationVisitor visitParameterAnnotation(
final int parameter, final String desc, final boolean visible) {
......
......@@ -261,16 +261,16 @@ final class AnnotationWriter extends AnnotationVisitor {
* Puts the given annotation lists into the given byte vector.
*
* @param panns an array of annotation writer lists.
* @param off index of the first annotation to be written.
* @param npanns the number of elements in panns to put (elements [0..npanns[ are put).
* @param out where the annotations must be put.
*/
static void put(final AnnotationWriter[] panns, final int off, final ByteVector out) {
int size = 1 + 2 * (panns.length - off);
for (int i = off; i < panns.length; ++i) {
static void put(final AnnotationWriter[] panns, final int npanns, final ByteVector out) {
int size = 1 + 2 * npanns;
for (int i = 0; i < npanns; ++i) {
size += panns[i] == null ? 0 : panns[i].getSize();
}
out.putInt(size).putByte(panns.length - off);
for (int i = off; i < panns.length; ++i) {
out.putInt(size).putByte(npanns);
for (int i = 0; i < npanns; ++i) {
AnnotationWriter aw = panns[i];
AnnotationWriter last = null;
int n = 0;
......
......@@ -1863,25 +1863,11 @@ public class ClassReader {
*/
private void readParameterAnnotations(
final MethodVisitor mv, final Context context, int v, final boolean visible) {
int i;
int n = b[v++] & 0xFF;
// workaround for a bug in javac (javac compiler generates a parameter
// annotation array whose size is equal to the number of parameters in
// the Java source file, while it should generate an array whose size is
// equal to the number of parameters in the method descriptor - which
// includes the synthetic parameters added by the compiler). This work-
// around supposes that the synthetic parameters are the first ones.
int synthetics = Type.getArgumentTypes(context.desc).length - n;
mv.visitAnnotableParameterCount(n, visible);
AnnotationVisitor av;
for (i = 0; i < synthetics; ++i) {
// virtual annotation to detect synthetic parameters in MethodWriter
av = mv.visitParameterAnnotation(i, "Ljava/lang/Synthetic;", false);
if (av != null) {
av.visitEnd();
}
}
char[] c = context.buffer;
for (; i < n + synthetics; ++i) {
for (int i = 0; i < n; ++i) {
int j = readUnsignedShort(v);
v += 2;
for (; j > 0; --j) {
......
......@@ -30,17 +30,17 @@ package org.objectweb.asm;
/**
* A visitor to visit a Java method. The methods of this class must be called in the following
* order: ( <tt>visitParameter</tt> )* [ <tt>visitAnnotationDefault</tt> ] (
* <tt>visitAnnotation</tt> | <tt>visitParameterAnnotation</tt> <tt>visitTypeAnnotation</tt> |
* <tt>visitAttribute</tt> )* [ <tt>visitCode</tt> ( <tt>visitFrame</tt> |
* <tt>visit<i>X</i>Insn</tt> | <tt>visitLabel</tt> | <tt>visitInsnAnnotation</tt> |
* <tt>visitTryCatchBlock</tt> | <tt>visitTryCatchAnnotation</tt> | <tt>visitLocalVariable</tt> |
* <tt>visitLocalVariableAnnotation</tt> | <tt>visitLineNumber</tt> )* <tt>visitMaxs</tt> ]
* <tt>visitEnd</tt>. In addition, the <tt>visit<i>X</i>Insn</tt> and <tt>visitLabel</tt> methods
* must be called in the sequential order of the bytecode instructions of the visited code,
* <tt>visitInsnAnnotation</tt> must be called <i>after</i> the annotated instruction,
* <tt>visitTryCatchBlock</tt> must be called <i>before</i> the labels passed as arguments have been
* visited, <tt>visitTryCatchBlockAnnotation</tt> must be called <i>after</i> the corresponding try
* catch block has been visited, and the <tt>visitLocalVariable</tt>,
* <tt>visitAnnotation</tt> | <tt>visitAnnotableParameterCount</tt> |
* <tt>visitParameterAnnotation</tt> <tt>visitTypeAnnotation</tt> | <tt>visitAttribute</tt> )* [
* <tt>visitCode</tt> ( <tt>visitFrame</tt> | <tt>visit<i>X</i>Insn</tt> | <tt>visitLabel</tt> |
* <tt>visitInsnAnnotation</tt> | <tt>visitTryCatchBlock</tt> | <tt>visitTryCatchAnnotation</tt> |
* <tt>visitLocalVariable</tt> | <tt>visitLocalVariableAnnotation</tt> | <tt>visitLineNumber</tt> )*
* <tt>visitMaxs</tt> ] <tt>visitEnd</tt>. In addition, the <tt>visit<i>X</i>Insn</tt> and
* <tt>visitLabel</tt> methods must be called in the sequential order of the bytecode instructions
* of the visited code, <tt>visitInsnAnnotation</tt> must be called <i>after</i> the annotated
* instruction, <tt>visitTryCatchBlock</tt> must be called <i>before</i> the labels passed as
* arguments have been visited, <tt>visitTryCatchBlockAnnotation</tt> must be called <i>after</i>
* the corresponding try catch block has been visited, and the <tt>visitLocalVariable</tt>,
* <tt>visitLocalVariableAnnotation</tt> and <tt>visitLineNumber</tt> methods must be called
* <i>after</i> the labels passed as arguments have been visited.
*
......@@ -160,10 +160,35 @@ public abstract class MethodVisitor {
return null;
}
/**
* Visits the number of method parameters that can have annotations. By default (i.e. when this
* method is not called), all the method parameters defined by the method descriptor can have
* annotations.
*
* @param parameterCount the number of method parameters than can have annotations. This number
* must be less or equal than the number of parameter types in the method descriptor. It can
* be strictly less when a method has synthetic parameters and when these parameters are
* ignored when computing parameter indices for the purpose of parameter annotations (see
* https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-4.html#jvms-4.7.18).
* @param visible <tt>true</tt> to define the number of method parameters that can have
* annotations visible at runtime, <tt>false</tt> to define the number of method parameters
* that can have annotations invisible at runtime.
*/
public void visitAnnotableParameterCount(int parameterCount, boolean visible) {
if (mv != null) {
mv.visitAnnotableParameterCount(parameterCount, visible);
}
}
/**
* Visits an annotation of a parameter this method.
*
* @param parameter the parameter index.
* @param parameter the parameter index. This index must be strictly smaller than the number of
* parameters in the method descriptor, and strictly smaller than the parameter count
* specified in {@link #visitAnnotableParameterCount()}. Important note: <i>a parameter index
* i is not required to correspond to the i'th parameter descriptor in the method
* descriptor</i>, in particular in case of synthetic parameters (see
* https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-4.html#jvms-4.7.18).
* @param desc the class descriptor of the annotation class.
* @param visible <tt>true</tt> if the annotation is visible at runtime.
* @return a visitor to visit the annotation values, or <tt>null</tt> if this visitor is not
......
......@@ -171,15 +171,18 @@ class MethodWriter extends MethodVisitor {
/** The runtime invisible type annotations of this method. May be <tt>null</tt>. */
private AnnotationWriter itanns;
/** The number of method parameters that can have runtime visible annotations, or 0. */
private int npanns;
/** The runtime visible parameter annotations of this method. May be <tt>null</tt>. */
private AnnotationWriter[] panns;
/** The number of method parameters that can have runtime invisible annotations, or 0. */
private int nipanns;
/** The runtime invisible parameter annotations of this method. May be <tt>null</tt>. */
private AnnotationWriter[] ipanns;
/** The number of synthetic parameters of this method. */
private int synthetics;
/** The non standard attributes of the method. */
private Attribute attrs;
......@@ -436,16 +439,19 @@ class MethodWriter extends MethodVisitor {
return aw;
}
@Override
public void visitAnnotableParameterCount(int parameterCount, boolean visible) {
if (visible) {
npanns = parameterCount;
} else {
nipanns = parameterCount;
}
}
@Override
public AnnotationVisitor visitParameterAnnotation(
final int parameter, final String desc, final boolean visible) {
ByteVector bv = new ByteVector();
if ("Ljava/lang/Synthetic;".equals(desc)) {
// workaround for a bug in javac with synthetic parameters
// see ClassReader.readParameterAnnotations
synthetics = Math.max(synthetics, parameter + 1);
return new AnnotationWriter(cw, false, bv, null, 0);
}
// write type, and reserve space for values count
bv.putShort(cw.newUTF8(desc)).putShort(0);
AnnotationWriter aw = new AnnotationWriter(cw, true, bv, bv, 2);
......@@ -2003,15 +2009,17 @@ class MethodWriter extends MethodVisitor {
}
if (panns != null) {
cw.newUTF8("RuntimeVisibleParameterAnnotations");
size += 7 + 2 * (panns.length - synthetics);
for (int i = panns.length - 1; i >= synthetics; --i) {
int i = npanns == 0 ? panns.length : npanns;
size += 7 + 2 * i;
for (--i; i >= 0; --i) {
size += panns[i] == null ? 0 : panns[i].getSize();
}
}
if (ipanns != null) {
cw.newUTF8("RuntimeInvisibleParameterAnnotations");
size += 7 + 2 * (ipanns.length - synthetics);
for (int i = ipanns.length - 1; i >= synthetics; --i) {
int i = nipanns == 0 ? ipanns.length : nipanns;
size += 7 + 2 * i;
for (--i; i >= 0; --i) {
size += ipanns[i] == null ? 0 : ipanns[i].getSize();
}
}
......@@ -2215,11 +2223,11 @@ class MethodWriter extends MethodVisitor {
}
if (panns != null) {
out.putShort(cw.newUTF8("RuntimeVisibleParameterAnnotations"));
AnnotationWriter.put(panns, synthetics, out);
AnnotationWriter.put(panns, npanns == 0 ? panns.length : npanns, out);
}
if (ipanns != null) {
out.putShort(cw.newUTF8("RuntimeInvisibleParameterAnnotations"));
AnnotationWriter.put(ipanns, synthetics, out);
AnnotationWriter.put(ipanns, nipanns == 0 ? ipanns.length : nipanns, out);
}
if (attrs != null) {
attrs.put(cw, null, 0, -1, -1, out);
......
  • We have 3 options, be fully backward compatible even if it makes little sense to support a code that was doing things wrong, try to find a way to warn user that override visitParameterAnnotation, silently fix the issue as the current patch does.

    The simplest way to warn user is to change visitParameterAnnotation, like this

    public AnnotationVisitor visitParameterAnnotation(
          final int parameter, final String desc, final int parameterCount, final boolean visible) {

    This is an incompatible change but only for people that override visitParameterAnnotation(), so i think it's fine. I think it's better than the current patch on the long term because the current patch introduce a dependency between the two method visitParameterAnnotation and visitParameterCount, things we have tried to avoid.

    Also, i believe that the real fix is to fix the spec to say that if the number of parameters from the descriptor and the number of parameter in the Runtime*ParameterAnnotations attribute are not the same, then the compiler should also emit a Parameter attribute which indicate which parameter is synthetic or not.

Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment