From 9c1083b5410d2a1f2ece899bbcd0c933ac7f6bbd Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Sat, 15 May 2021 23:48:18 +0200 Subject: [PATCH] Muzzle code generation cleanup (#2985) * Muzzle code generation cleanup - remove hardcoded class names from MuzzleCodeGenerator (easier renaming in the future) - store field/method descriptors in references instead of types/lists of types - remove unnecessary primitive type comparison in ReferenceMatcher (comparing descriptors is enough) - improve printMuzzleReferences output --- .../javaagent/extension/muzzle/Reference.java | 100 +++++++------- .../muzzle/collector/MuzzleCodeGenerator.java | 127 +++++++++--------- .../ReferenceCollectingClassVisitor.java | 2 +- .../matcher/HelperReferenceWrapper.java | 2 +- .../tooling/muzzle/matcher/Mismatch.java | 69 +++++++--- .../matcher/MuzzleGradlePluginUtil.java | 43 +++--- .../muzzle/matcher/ReferenceMatcher.java | 92 +++---------- .../muzzle/ReferenceCollectorTest.groovy | 2 +- .../groovy/muzzle/ReferenceMatcherTest.groovy | 30 +++-- .../src/test/java/muzzle/TestClasses.java | 5 + 10 files changed, 223 insertions(+), 249 deletions(-) diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/muzzle/Reference.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/muzzle/Reference.java index 03e79a2f7c..37dec0c1f4 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/muzzle/Reference.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/muzzle/Reference.java @@ -5,6 +5,8 @@ package io.opentelemetry.javaagent.extension.muzzle; +import static java.util.Collections.emptySet; + import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -12,6 +14,8 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Objects; import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; import net.bytebuddy.jar.asm.Opcodes; import net.bytebuddy.jar.asm.Type; import org.checkerframework.checker.nullness.qual.Nullable; @@ -149,7 +153,10 @@ public final class Reference { @Override public String toString() { - return "Reference<" + className + ">"; + String extendsPart = superName == null ? "" : " extends " + superName; + String implementsPart = + interfaces.isEmpty() ? "" : " implements " + String.join(", ", interfaces); + return "ClassRef: " + className + extendsPart + implementsPart; } /** Represents the source (file name, line number) of a reference. */ @@ -342,39 +349,22 @@ public final class Reference { private final Set sources; private final Set flags; private final String name; - private final Type returnType; - private final List parameterTypes; - - public Method(String name, String descriptor) { - this( - new Source[0], - new Flag[0], - name, - Type.getMethodType(descriptor).getReturnType(), - Type.getMethodType(descriptor).getArgumentTypes()); - } + private final String descriptor; public Method( Source[] sources, Flag[] flags, String name, Type returnType, Type[] parameterTypes) { this( - COLLECT_SOURCES ? new LinkedHashSet<>(Arrays.asList(sources)) : new LinkedHashSet<>(), + COLLECT_SOURCES ? new LinkedHashSet<>(Arrays.asList(sources)) : emptySet(), new LinkedHashSet<>(Arrays.asList(flags)), name, - returnType, - Arrays.asList(parameterTypes)); + Type.getMethodDescriptor(returnType, parameterTypes)); } - private Method( - Set sources, - Set flags, - String name, - Type returnType, - List parameterTypes) { + private Method(Set sources, Set flags, String name, String descriptor) { this.sources = sources; this.flags = flags; this.name = name; - this.returnType = returnType; - this.parameterTypes = parameterTypes; + this.descriptor = descriptor; } public Set getSources() { @@ -389,12 +379,8 @@ public final class Reference { return name; } - public Type getReturnType() { - return returnType; - } - - public List getParameterTypes() { - return parameterTypes; + public String getDescriptor() { + return descriptor; } public Method merge(Method anotherMethod) { @@ -410,16 +396,19 @@ public final class Reference { mergedFlags.addAll(flags); mergedFlags.addAll(anotherMethod.flags); - return new Method(mergedSources, mergedFlags, name, returnType, parameterTypes); + return new Method(mergedSources, mergedFlags, name, descriptor); } @Override public String toString() { - return name + getDescriptor(); - } - - public String getDescriptor() { - return Type.getMethodType(returnType, parameterTypes.toArray(new Type[0])).getDescriptor(); + Type methodType = Type.getMethodType(getDescriptor()); + String returnType = methodType.getReturnType().getClassName(); + String modifiers = flags.stream().map(Flag::toString).collect(Collectors.joining(" ")); + String parameters = + Stream.of(methodType.getArgumentTypes()) + .map(Type::getClassName) + .collect(Collectors.joining(", ", "(", ")")); + return "MethodRef: " + modifiers + " " + returnType + " " + name + parameters; } @Override @@ -431,12 +420,12 @@ public final class Reference { return false; } Method other = (Method) obj; - return name.equals(other.name) && getDescriptor().equals(other.getDescriptor()); + return name.equals(other.name) && descriptor.equals(other.descriptor); } @Override public int hashCode() { - return Objects.hash(name, getDescriptor()); + return Objects.hash(name, descriptor); } } @@ -444,15 +433,24 @@ public final class Reference { private final Set sources; private final Set flags; private final String name; - private final Type type; + private final String descriptor; private final boolean declared; - public Field(Source[] sources, Flag[] flags, String name, Type fieldType, boolean declared) { - this.sources = - COLLECT_SOURCES ? new LinkedHashSet<>(Arrays.asList(sources)) : new LinkedHashSet<>(); - this.flags = new LinkedHashSet<>(Arrays.asList(flags)); + private Field(Source[] sources, Flag[] flags, String name, Type fieldType, boolean declared) { + this( + COLLECT_SOURCES ? new LinkedHashSet<>(Arrays.asList(sources)) : emptySet(), + new LinkedHashSet<>(Arrays.asList(flags)), + name, + fieldType.getDescriptor(), + declared); + } + + private Field( + Set sources, Set flags, String name, String descriptor, boolean declared) { + this.sources = sources; + this.flags = flags; this.name = name; - this.type = fieldType; + this.descriptor = descriptor; this.declared = declared; } @@ -468,8 +466,8 @@ public final class Reference { return flags; } - public Type getType() { - return type; + public String getDescriptor() { + return descriptor; } /** @@ -481,20 +479,22 @@ public final class Reference { } public Field merge(Field anotherField) { - if (!equals(anotherField) || !type.equals(anotherField.type)) { + if (!equals(anotherField) || !descriptor.equals(anotherField.descriptor)) { throw new IllegalStateException("illegal merge " + this + " != " + anotherField); } return new Field( - Reference.merge(sources, anotherField.sources).toArray(new Source[0]), - mergeFlags(flags, anotherField.flags).toArray(new Flag[0]), + Reference.merge(sources, anotherField.sources), + mergeFlags(flags, anotherField.flags), name, - type, + descriptor, declared || anotherField.declared); } @Override public String toString() { - return "FieldRef:" + name + type.getInternalName(); + String modifiers = flags.stream().map(Flag::toString).collect(Collectors.joining(" ")); + String fieldType = Type.getType(getDescriptor()).getClassName(); + return "FieldRef: " + modifiers + " " + fieldType + " " + name; } @Override diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/MuzzleCodeGenerator.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/MuzzleCodeGenerator.java index 1cf0a1432b..4fe69b31cd 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/MuzzleCodeGenerator.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/MuzzleCodeGenerator.java @@ -236,6 +236,14 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { } private void generateMuzzleReferencesMethod(ReferenceCollector collector) { + Type referenceType = Type.getType(Reference.class); + Type referenceArrayType = Type.getType(Reference[].class); + Type referenceBuilderType = Type.getType(Reference.Builder.class); + Type referenceFlagType = Type.getType(Reference.Flag.class); + Type referenceSourceType = Type.getType(Reference.Source.class); + Type stringType = Type.getType(String.class); + Type typeType = Type.getType(Type.class); + /* * public synchronized Reference[] getMuzzleReferences() { * if (null == this.muzzleReferences) { @@ -251,7 +259,7 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { super.visitMethod( Opcodes.ACC_PUBLIC + Opcodes.ACC_SYNCHRONIZED, MUZZLE_REFERENCES_METHOD_NAME, - "()[Lio/opentelemetry/javaagent/extension/muzzle/Reference;", + Type.getMethodDescriptor(referenceArrayType), null, null); @@ -267,26 +275,24 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { Opcodes.GETFIELD, instrumentationClassName, MUZZLE_REFERENCES_FIELD_NAME, - Type.getDescriptor(Reference[].class)); + referenceArrayType.getDescriptor()); mv.visitJumpInsn(Opcodes.IF_ACMPNE, ret); mv.visitVarInsn(Opcodes.ALOAD, 0); Reference[] references = collector.getReferences().values().toArray(new Reference[0]); mv.visitLdcInsn(references.length); - mv.visitTypeInsn( - Opcodes.ANEWARRAY, "io/opentelemetry/javaagent/extension/muzzle/Reference"); + mv.visitTypeInsn(Opcodes.ANEWARRAY, referenceType.getInternalName()); for (int i = 0; i < references.length; ++i) { mv.visitInsn(Opcodes.DUP); mv.visitLdcInsn(i); - mv.visitTypeInsn( - Opcodes.NEW, "io/opentelemetry/javaagent/extension/muzzle/Reference$Builder"); + mv.visitTypeInsn(Opcodes.NEW, referenceBuilderType.getInternalName()); mv.visitInsn(Opcodes.DUP); mv.visitLdcInsn(references[i].getClassName()); mv.visitMethodInsn( Opcodes.INVOKESPECIAL, - "io/opentelemetry/javaagent/extension/muzzle/Reference$Builder", + referenceBuilderType.getInternalName(), "", "(Ljava/lang/String;)V", false); @@ -295,9 +301,9 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { mv.visitLdcInsn(source.getLine()); mv.visitMethodInsn( Opcodes.INVOKEVIRTUAL, - "io/opentelemetry/javaagent/extension/muzzle/Reference$Builder", + referenceBuilderType.getInternalName(), "withSource", - "(Ljava/lang/String;I)Lio/opentelemetry/javaagent/extension/muzzle/Reference$Builder;", + Type.getMethodDescriptor(referenceBuilderType, stringType, Type.INT_TYPE), false); } for (Reference.Flag flag : references[i].getFlags()) { @@ -306,49 +312,46 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { Opcodes.GETSTATIC, enumClassName, flag.name(), "L" + enumClassName + ";"); mv.visitMethodInsn( Opcodes.INVOKEVIRTUAL, - "io/opentelemetry/javaagent/extension/muzzle/Reference$Builder", + referenceBuilderType.getInternalName(), "withFlag", - "(Lio/opentelemetry/javaagent/extension/muzzle/Reference$Flag;)Lio/opentelemetry/javaagent/extension/muzzle/Reference$Builder;", + Type.getMethodDescriptor(referenceBuilderType, referenceFlagType), false); } if (null != references[i].getSuperName()) { mv.visitLdcInsn(references[i].getSuperName()); mv.visitMethodInsn( Opcodes.INVOKEVIRTUAL, - "io/opentelemetry/javaagent/extension/muzzle/Reference$Builder", + referenceBuilderType.getInternalName(), "withSuperName", - "(Ljava/lang/String;)Lio/opentelemetry/javaagent/extension/muzzle/Reference$Builder;", + Type.getMethodDescriptor(referenceBuilderType, stringType), false); } for (String interfaceName : references[i].getInterfaces()) { mv.visitLdcInsn(interfaceName); mv.visitMethodInsn( Opcodes.INVOKEVIRTUAL, - "io/opentelemetry/javaagent/extension/muzzle/Reference$Builder", + referenceBuilderType.getInternalName(), "withInterface", - "(Ljava/lang/String;)Lio/opentelemetry/javaagent/extension/muzzle/Reference$Builder;", + Type.getMethodDescriptor(referenceBuilderType, stringType), false); } for (Reference.Field field : references[i].getFields()) { { // sources mv.visitLdcInsn(field.getSources().size()); - mv.visitTypeInsn( - Opcodes.ANEWARRAY, - "io/opentelemetry/javaagent/extension/muzzle/Reference$Source"); + mv.visitTypeInsn(Opcodes.ANEWARRAY, referenceSourceType.getInternalName()); int j = 0; for (Reference.Source source : field.getSources()) { mv.visitInsn(Opcodes.DUP); mv.visitLdcInsn(j); - mv.visitTypeInsn( - Opcodes.NEW, "io/opentelemetry/javaagent/extension/muzzle/Reference$Source"); + mv.visitTypeInsn(Opcodes.NEW, referenceSourceType.getInternalName()); mv.visitInsn(Opcodes.DUP); mv.visitLdcInsn(source.getName()); mv.visitLdcInsn(source.getLine()); mv.visitMethodInsn( Opcodes.INVOKESPECIAL, - "io/opentelemetry/javaagent/extension/muzzle/Reference$Source", + referenceSourceType.getInternalName(), "", "(Ljava/lang/String;I)V", false); @@ -360,8 +363,7 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { { // flags mv.visitLdcInsn(field.getFlags().size()); - mv.visitTypeInsn( - Opcodes.ANEWARRAY, "io/opentelemetry/javaagent/extension/muzzle/Reference$Flag"); + mv.visitTypeInsn(Opcodes.ANEWARRAY, referenceFlagType.getInternalName()); int j = 0; for (Reference.Flag flag : field.getFlags()) { @@ -378,12 +380,12 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { mv.visitLdcInsn(field.getName()); { // field type - mv.visitLdcInsn(field.getType().getDescriptor()); + mv.visitLdcInsn(field.getDescriptor()); mv.visitMethodInsn( Opcodes.INVOKESTATIC, - Type.getInternalName(Type.class), + typeType.getInternalName(), "getType", - Type.getMethodDescriptor(Type.class.getMethod("getType", String.class)), + Type.getMethodDescriptor(typeType, stringType), false); } @@ -392,7 +394,7 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { mv.visitMethodInsn( Opcodes.INVOKEVIRTUAL, - "io/opentelemetry/javaagent/extension/muzzle/Reference$Builder", + referenceBuilderType.getInternalName(), "withField", Type.getMethodDescriptor( Reference.Builder.class.getMethod( @@ -406,21 +408,19 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { } for (Reference.Method method : references[i].getMethods()) { mv.visitLdcInsn(method.getSources().size()); - mv.visitTypeInsn( - Opcodes.ANEWARRAY, "io/opentelemetry/javaagent/extension/muzzle/Reference$Source"); + mv.visitTypeInsn(Opcodes.ANEWARRAY, referenceSourceType.getInternalName()); int j = 0; for (Reference.Source source : method.getSources()) { mv.visitInsn(Opcodes.DUP); mv.visitLdcInsn(j); - mv.visitTypeInsn( - Opcodes.NEW, "io/opentelemetry/javaagent/extension/muzzle/Reference$Source"); + mv.visitTypeInsn(Opcodes.NEW, referenceSourceType.getInternalName()); mv.visitInsn(Opcodes.DUP); mv.visitLdcInsn(source.getName()); mv.visitLdcInsn(source.getLine()); mv.visitMethodInsn( Opcodes.INVOKESPECIAL, - "io/opentelemetry/javaagent/extension/muzzle/Reference$Source", + referenceSourceType.getInternalName(), "", "(Ljava/lang/String;I)V", false); @@ -430,8 +430,7 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { } mv.visitLdcInsn(method.getFlags().size()); - mv.visitTypeInsn( - Opcodes.ANEWARRAY, "io/opentelemetry/javaagent/extension/muzzle/Reference$Flag"); + mv.visitTypeInsn(Opcodes.ANEWARRAY, referenceFlagType.getInternalName()); j = 0; for (Reference.Flag flag : method.getFlags()) { mv.visitInsn(Opcodes.DUP); @@ -445,38 +444,44 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { mv.visitLdcInsn(method.getName()); - { // return type - mv.visitLdcInsn(method.getReturnType().getDescriptor()); + { + // we cannot pass the whole method descriptor string as it won't be shaded, so we + // have to pass the return and parameter types separately - strings in Type.getType() + // calls will be shaded correctly + Type methodType = Type.getMethodType(method.getDescriptor()); + + // return type + mv.visitLdcInsn(methodType.getReturnType().getDescriptor()); mv.visitMethodInsn( Opcodes.INVOKESTATIC, - Type.getInternalName(Type.class), + typeType.getInternalName(), "getType", - Type.getMethodDescriptor(Type.class.getMethod("getType", String.class)), - false); - } - - mv.visitLdcInsn(method.getParameterTypes().size()); - mv.visitTypeInsn(Opcodes.ANEWARRAY, Type.getInternalName(Type.class)); - j = 0; - for (Type parameterType : method.getParameterTypes()) { - mv.visitInsn(Opcodes.DUP); - mv.visitLdcInsn(j); - - mv.visitLdcInsn(parameterType.getDescriptor()); - mv.visitMethodInsn( - Opcodes.INVOKESTATIC, - Type.getInternalName(Type.class), - "getType", - Type.getMethodDescriptor(Type.class.getMethod("getType", String.class)), + Type.getMethodDescriptor(typeType, stringType), false); - mv.visitInsn(Opcodes.AASTORE); - j++; + mv.visitLdcInsn(methodType.getArgumentTypes().length); + mv.visitTypeInsn(Opcodes.ANEWARRAY, typeType.getInternalName()); + j = 0; + for (Type parameterType : methodType.getArgumentTypes()) { + mv.visitInsn(Opcodes.DUP); + mv.visitLdcInsn(j); + + mv.visitLdcInsn(parameterType.getDescriptor()); + mv.visitMethodInsn( + Opcodes.INVOKESTATIC, + typeType.getInternalName(), + "getType", + Type.getMethodDescriptor(typeType, stringType), + false); + + mv.visitInsn(Opcodes.AASTORE); + j++; + } } mv.visitMethodInsn( Opcodes.INVOKEVIRTUAL, - "io/opentelemetry/javaagent/extension/muzzle/Reference$Builder", + referenceBuilderType.getInternalName(), "withMethod", Type.getMethodDescriptor( Reference.Builder.class.getMethod( @@ -490,9 +495,9 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { } mv.visitMethodInsn( Opcodes.INVOKEVIRTUAL, - "io/opentelemetry/javaagent/extension/muzzle/Reference$Builder", + referenceBuilderType.getInternalName(), "build", - "()Lio/opentelemetry/javaagent/extension/muzzle/Reference;", + Type.getMethodDescriptor(referenceType), false); mv.visitInsn(Opcodes.AASTORE); } @@ -501,7 +506,7 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { Opcodes.PUTFIELD, instrumentationClassName, MUZZLE_REFERENCES_FIELD_NAME, - Type.getDescriptor(Reference[].class)); + referenceArrayType.getDescriptor()); mv.visitLabel(ret); if (frames) { @@ -512,7 +517,7 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { Opcodes.GETFIELD, instrumentationClassName, MUZZLE_REFERENCES_FIELD_NAME, - Type.getDescriptor(Reference[].class)); + referenceArrayType.getDescriptor()); mv.visitInsn(Opcodes.ARETURN); mv.visitLabel(finish); diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollectingClassVisitor.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollectingClassVisitor.java index 15e501a442..6458ee7a2b 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollectingClassVisitor.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollectingClassVisitor.java @@ -333,7 +333,7 @@ class ReferenceCollectingClassVisitor extends ClassVisitor { false) .build()); - Type underlyingFieldType = underlyingType(fieldType); + Type underlyingFieldType = underlyingType(Type.getType(descriptor)); if (underlyingFieldType.getSort() == Type.OBJECT) { addReference( new Reference.Builder(underlyingFieldType.getInternalName()) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/HelperReferenceWrapper.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/HelperReferenceWrapper.java index e96c34c49f..86b7e98110 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/HelperReferenceWrapper.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/HelperReferenceWrapper.java @@ -217,7 +217,7 @@ interface HelperReferenceWrapper { } private Field toField(Reference.Field field) { - return new Field(field.getName(), field.getType().getDescriptor()); + return new Field(field.getName(), field.getDescriptor()); } } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/Mismatch.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/Mismatch.java index 2519d90768..dbf46f8f71 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/Mismatch.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/Mismatch.java @@ -8,6 +8,9 @@ package io.opentelemetry.javaagent.tooling.muzzle.matcher; import io.opentelemetry.javaagent.extension.muzzle.Reference; import java.io.PrintWriter; import java.io.StringWriter; +import java.util.Collection; +import java.util.Collections; +import net.bytebuddy.jar.asm.Type; /** * A mismatch between a {@link Reference} and a runtime class. @@ -17,16 +20,16 @@ import java.io.StringWriter; */ public abstract class Mismatch { /** Instrumentation sources which caused the mismatch. */ - private final Reference.Source[] mismatchSources; + private final Collection mismatchSources; - private Mismatch(Reference.Source[] mismatchSources) { + private Mismatch(Collection mismatchSources) { this.mismatchSources = mismatchSources; } @Override public String toString() { - if (mismatchSources.length > 0) { - return mismatchSources[0].toString() + " " + getMismatchDetails(); + if (mismatchSources.size() > 0) { + return mismatchSources.iterator().next().toString() + " " + getMismatchDetails(); } else { return " " + getMismatchDetails(); } @@ -38,8 +41,13 @@ public abstract class Mismatch { public static class MissingClass extends Mismatch { private final String className; - public MissingClass(Reference.Source[] sources, String className) { - super(sources); + public MissingClass(Reference classRef) { + super(classRef.getSources()); + this.className = classRef.getClassName(); + } + + public MissingClass(Reference classRef, String className) { + super(classRef.getSources()); this.className = className; } @@ -55,7 +63,7 @@ public abstract class Mismatch { private final int foundAccess; public MissingFlag( - Reference.Source[] sources, + Collection sources, String classMethodOrFieldDesc, Reference.Flag expectedFlag, int foundAccess) { @@ -74,19 +82,30 @@ public abstract class Mismatch { public static class MissingField extends Mismatch { private final String className; private final String fieldName; - private final String fieldDesc; + private final String fieldDescriptor; - public MissingField( - Reference.Source[] sources, String className, String fieldName, String fieldDesc) { - super(sources); - this.className = className; - this.fieldName = fieldName; - this.fieldDesc = fieldDesc; + MissingField(Reference classRef, Reference.Field fieldRef) { + super(fieldRef.getSources()); + this.className = classRef.getClassName(); + this.fieldName = fieldRef.getName(); + this.fieldDescriptor = fieldRef.getDescriptor(); + } + + MissingField(Reference classRef, HelperReferenceWrapper.Field field) { + super(classRef.getSources()); + this.className = classRef.getClassName(); + this.fieldName = field.getName(); + this.fieldDescriptor = field.getDescriptor(); } @Override String getMismatchDetails() { - return "Missing field " + className + "#" + fieldName + fieldDesc; + return "Missing field " + + Type.getType(fieldDescriptor).getClassName() + + " " + + fieldName + + " in class " + + className; } } @@ -95,12 +114,18 @@ public abstract class Mismatch { private final String methodName; private final String methodDescriptor; - public MissingMethod( - Reference.Source[] sources, String className, String methodName, String methodDescriptor) { - super(sources); - this.className = className; - this.methodName = methodName; - this.methodDescriptor = methodDescriptor; + public MissingMethod(Reference classRef, Reference.Method methodRef) { + super(methodRef.getSources()); + this.className = classRef.getClassName(); + this.methodName = methodRef.getName(); + this.methodDescriptor = methodRef.getDescriptor(); + } + + public MissingMethod(Reference classRef, HelperReferenceWrapper.Method method) { + super(classRef.getSources()); + this.className = method.getDeclaringClass(); + this.methodName = method.getName(); + this.methodDescriptor = method.getDescriptor(); } @Override @@ -117,7 +142,7 @@ public abstract class Mismatch { public ReferenceCheckError( Exception e, Reference referenceBeingChecked, ClassLoader classLoaderBeingChecked) { - super(new Reference.Source[0]); + super(Collections.emptyList()); referenceCheckException = e; this.referenceBeingChecked = referenceBeingChecked; this.classLoaderBeingChecked = classLoaderBeingChecked; diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/MuzzleGradlePluginUtil.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/MuzzleGradlePluginUtil.java index c62ebe9970..8c063a86e7 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/MuzzleGradlePluginUtil.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/MuzzleGradlePluginUtil.java @@ -5,6 +5,7 @@ package io.opentelemetry.javaagent.tooling.muzzle.matcher; +import static java.lang.System.lineSeparator; import static java.util.Arrays.asList; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; @@ -20,6 +21,7 @@ import net.bytebuddy.dynamic.ClassFileLocator; /** Entry point for the muzzle gradle plugin. */ public final class MuzzleGradlePluginUtil { + private static final String INDENT = " "; /** * Verifies that all instrumentations present in the {@code agentClassLoader} can be safely @@ -134,7 +136,7 @@ public final class MuzzleGradlePluginUtil { try { System.out.println(instrumentationModule.getClass().getName()); for (Reference ref : instrumentationModule.getMuzzleReferences()) { - System.out.println(prettyPrint(" ", ref)); + System.out.print(prettyPrint(ref)); } } catch (Exception e) { String message = @@ -146,36 +148,25 @@ public final class MuzzleGradlePluginUtil { } } - private static String prettyPrint(String prefix, Reference ref) { - StringBuilder builder = new StringBuilder(prefix).append(ref.getClassName()); - if (ref.getSuperName() != null) { - builder.append(" extends<").append(ref.getSuperName()).append(">"); - } - if (!ref.getInterfaces().isEmpty()) { - builder.append(" implements "); - for (String iface : ref.getInterfaces()) { - builder.append(" <").append(iface).append(">"); + private static String prettyPrint(Reference ref) { + StringBuilder builder = new StringBuilder(INDENT).append(ref).append(lineSeparator()); + if (!ref.getSources().isEmpty()) { + builder.append(INDENT).append(INDENT).append("Sources:").append(lineSeparator()); + for (Reference.Source source : ref.getSources()) { + builder + .append(INDENT) + .append(INDENT) + .append(INDENT) + .append("at: ") + .append(source) + .append(lineSeparator()); } } - for (Reference.Source source : ref.getSources()) { - builder.append("\n").append(prefix).append(prefix); - builder.append("Source: ").append(source.toString()); - } for (Reference.Field field : ref.getFields()) { - builder.append("\n").append(prefix).append(prefix); - builder.append("Field: "); - for (Reference.Flag flag : field.getFlags()) { - builder.append(flag).append(" "); - } - builder.append(field.toString()); + builder.append(INDENT).append(INDENT).append(field).append(lineSeparator()); } for (Reference.Method method : ref.getMethods()) { - builder.append("\n").append(prefix).append(prefix); - builder.append("Method: "); - for (Reference.Flag flag : method.getFlags()) { - builder.append(flag).append(" "); - } - builder.append(method.toString()); + builder.append(INDENT).append(INDENT).append(method).append(lineSeparator()); } return builder.toString(); } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/ReferenceMatcher.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/ReferenceMatcher.java index 245ee70500..797e654868 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/ReferenceMatcher.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/ReferenceMatcher.java @@ -6,11 +6,11 @@ package io.opentelemetry.javaagent.tooling.muzzle.matcher; import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; import static net.bytebuddy.dynamic.loading.ClassLoadingStrategy.BOOTSTRAP_LOADER; import io.opentelemetry.instrumentation.api.caching.Cache; import io.opentelemetry.javaagent.extension.muzzle.Reference; -import io.opentelemetry.javaagent.extension.muzzle.Reference.Source; import io.opentelemetry.javaagent.tooling.AgentTooling; import io.opentelemetry.javaagent.tooling.Utils; import io.opentelemetry.javaagent.tooling.muzzle.InstrumentationClassPredicate; @@ -29,6 +29,7 @@ import java.util.stream.Collectors; import net.bytebuddy.description.field.FieldDescription; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.jar.asm.Type; import net.bytebuddy.pool.TypePool; /** Matches a set of references against a classloader. */ @@ -116,18 +117,14 @@ public final class ReferenceMatcher { if (instrumentationClassPredicate.isInstrumentationClass(reference.getClassName())) { // make sure helper class is registered if (!helperClassNames.contains(reference.getClassName())) { - return Collections.singletonList( - new Mismatch.MissingClass( - reference.getSources().toArray(new Source[0]), reference.getClassName())); + return singletonList(new Mismatch.MissingClass(reference)); } // helper classes get their own check: whether they implement all abstract methods return checkHelperClassMatch(reference, typePool); } else { TypePool.Resolution resolution = typePool.describe(reference.getClassName()); if (!resolution.isResolved()) { - return Collections.singletonList( - new Mismatch.MissingClass( - reference.getSources().toArray(new Source[0]), reference.getClassName())); + return singletonList(new Mismatch.MissingClass(reference)); } return checkThirdPartyTypeMatch(reference, resolution.resolve()); } @@ -136,11 +133,10 @@ public final class ReferenceMatcher { // bytebuddy throws an illegal state exception with this message if it cannot resolve types // TODO: handle missing type resolutions without catching bytebuddy's exceptions String className = e.getMessage().replace("Cannot resolve type description for ", ""); - return Collections.singletonList( - new Mismatch.MissingClass(reference.getSources().toArray(new Source[0]), className)); + return singletonList(new Mismatch.MissingClass(reference, className)); } else { // Shouldn't happen. Fail the reference check and add a mismatch for debug logging. - return Collections.singletonList(new Mismatch.ReferenceCheckError(e, reference, loader)); + return singletonList(new Mismatch.ReferenceCheckError(e, reference, loader)); } } } @@ -155,7 +151,7 @@ public final class ReferenceMatcher { Set undeclaredFields = helperClass.getFields().stream() .filter(f -> !f.isDeclared()) - .map(f -> new HelperReferenceWrapper.Field(f.getName(), f.getType().getDescriptor())) + .map(f -> new HelperReferenceWrapper.Field(f.getName(), f.getDescriptor())) .collect(Collectors.toSet()); // if there are any fields in this helper class that's not declared here, check the type @@ -166,14 +162,7 @@ public final class ReferenceMatcher { undeclaredFields.removeAll(superClassFields); for (HelperReferenceWrapper.Field missingField : undeclaredFields) { - mismatches = - lazyAdd( - mismatches, - new Mismatch.MissingField( - helperClass.getSources().toArray(new Source[0]), - helperClass.getClassName(), - missingField.getName(), - missingField.getDescriptor())); + mismatches = lazyAdd(mismatches, new Mismatch.MissingField(helperClass, missingField)); } } @@ -191,13 +180,7 @@ public final class ReferenceMatcher { abstractMethods.removeAll(plainMethods); for (HelperReferenceWrapper.Method unimplementedMethod : abstractMethods) { mismatches = - lazyAdd( - mismatches, - new Mismatch.MissingMethod( - helperClass.getSources().toArray(new Reference.Source[0]), - unimplementedMethod.getDeclaringClass(), - unimplementedMethod.getName(), - unimplementedMethod.getDescriptor())); + lazyAdd(mismatches, new Mismatch.MissingMethod(helperClass, unimplementedMethod)); } return mismatches; @@ -232,24 +215,14 @@ public final class ReferenceMatcher { lazyAdd( mismatches, new Mismatch.MissingFlag( - reference.getSources().toArray(new Source[0]), - desc, - flag, - typeOnClasspath.getActualModifiers(false))); + reference.getSources(), desc, flag, typeOnClasspath.getActualModifiers(false))); } } for (Reference.Field fieldRef : reference.getFields()) { FieldDescription.InDefinedShape fieldDescription = findField(fieldRef, typeOnClasspath); if (fieldDescription == null) { - mismatches = - lazyAdd( - mismatches, - new Mismatch.MissingField( - fieldRef.getSources().toArray(new Reference.Source[0]), - reference.getClassName(), - fieldRef.getName(), - fieldRef.getType().getInternalName())); + mismatches = lazyAdd(mismatches, new Mismatch.MissingField(reference, fieldRef)); } else { for (Reference.Flag flag : fieldRef.getFlags()) { if (!flag.matches(fieldDescription.getModifiers())) { @@ -257,15 +230,12 @@ public final class ReferenceMatcher { reference.getClassName() + "#" + fieldRef.getName() - + fieldRef.getType().getInternalName(); + + Type.getType(fieldRef.getDescriptor()).getInternalName(); mismatches = lazyAdd( mismatches, new Mismatch.MissingFlag( - fieldRef.getSources().toArray(new Source[0]), - desc, - flag, - fieldDescription.getModifiers())); + fieldRef.getSources(), desc, flag, fieldDescription.getModifiers())); } } } @@ -274,14 +244,7 @@ public final class ReferenceMatcher { for (Reference.Method methodRef : reference.getMethods()) { MethodDescription.InDefinedShape methodDescription = findMethod(methodRef, typeOnClasspath); if (methodDescription == null) { - mismatches = - lazyAdd( - mismatches, - new Mismatch.MissingMethod( - methodRef.getSources().toArray(new Reference.Source[0]), - reference.getClassName(), - methodRef.getName(), - methodRef.getDescriptor())); + mismatches = lazyAdd(mismatches, new Mismatch.MissingMethod(reference, methodRef)); } else { for (Reference.Flag flag : methodRef.getFlags()) { if (!flag.matches(methodDescription.getModifiers())) { @@ -291,10 +254,7 @@ public final class ReferenceMatcher { lazyAdd( mismatches, new Mismatch.MissingFlag( - methodRef.getSources().toArray(new Source[0]), - desc, - flag, - methodDescription.getModifiers())); + methodRef.getSources(), desc, flag, methodDescription.getModifiers())); } } } @@ -302,31 +262,11 @@ public final class ReferenceMatcher { return mismatches; } - private static boolean matchesPrimitive(String longName, String shortName) { - // The two meta type systems in use here differ in their treatment of primitive type names.... - return shortName.equals("I") && longName.equals(int.class.getName()) - || shortName.equals("C") && longName.equals(char.class.getName()) - || shortName.equals("Z") && longName.equals(boolean.class.getName()) - || shortName.equals("J") && longName.equals(long.class.getName()) - || shortName.equals("S") && longName.equals(short.class.getName()) - || shortName.equals("F") && longName.equals(float.class.getName()) - || shortName.equals("D") && longName.equals(double.class.getName()) - || shortName.equals("B") && longName.equals(byte.class.getName()); - } - private static FieldDescription.InDefinedShape findField( Reference.Field fieldRef, TypeDescription typeOnClasspath) { for (FieldDescription.InDefinedShape fieldType : typeOnClasspath.getDeclaredFields()) { if (fieldType.getName().equals(fieldRef.getName()) - && ((fieldType - .getType() - .asErasure() - .getInternalName() - .equals(fieldRef.getType().getInternalName())) - || (fieldType.getType().asErasure().isPrimitive() - && matchesPrimitive( - fieldType.getType().asErasure().getInternalName(), - fieldRef.getType().getInternalName())))) { + && fieldType.getDescriptor().equals(fieldRef.getDescriptor())) { return fieldType; } } diff --git a/testing-common/src/test/groovy/muzzle/ReferenceCollectorTest.groovy b/testing-common/src/test/groovy/muzzle/ReferenceCollectorTest.groovy index 3634f983c2..df740ee7cc 100644 --- a/testing-common/src/test/groovy/muzzle/ReferenceCollectorTest.groovy +++ b/testing-common/src/test/groovy/muzzle/ReferenceCollectorTest.groovy @@ -333,7 +333,7 @@ class ReferenceCollectorTest extends Specification { private static findMethod(Reference reference, String methodName, String methodDesc) { for (def method : reference.methods) { - if (method == new Reference.Method(methodName, methodDesc)) { + if (method.name == methodName && method.descriptor == methodDesc) { return method } } diff --git a/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy b/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy index b7e10a1b53..580593ee63 100644 --- a/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy +++ b/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy @@ -5,22 +5,28 @@ package muzzle -import static io.opentelemetry.javaagent.tooling.muzzle.matcher.Mismatch.* -import static io.opentelemetry.javaagent.extension.muzzle.Reference.Flag.ManifestationFlag.* +import static io.opentelemetry.javaagent.extension.muzzle.Reference.Flag.ManifestationFlag.ABSTRACT +import static io.opentelemetry.javaagent.extension.muzzle.Reference.Flag.ManifestationFlag.INTERFACE +import static io.opentelemetry.javaagent.extension.muzzle.Reference.Flag.ManifestationFlag.NON_INTERFACE +import static io.opentelemetry.javaagent.extension.muzzle.Reference.Flag.MinimumVisibilityFlag.PACKAGE_OR_HIGHER import static io.opentelemetry.javaagent.extension.muzzle.Reference.Flag.MinimumVisibilityFlag.PRIVATE_OR_HIGHER import static io.opentelemetry.javaagent.extension.muzzle.Reference.Flag.MinimumVisibilityFlag.PROTECTED_OR_HIGHER import static io.opentelemetry.javaagent.extension.muzzle.Reference.Flag.OwnershipFlag.NON_STATIC import static io.opentelemetry.javaagent.extension.muzzle.Reference.Flag.OwnershipFlag.STATIC +import static io.opentelemetry.javaagent.tooling.muzzle.matcher.Mismatch.MissingClass +import static io.opentelemetry.javaagent.tooling.muzzle.matcher.Mismatch.MissingField +import static io.opentelemetry.javaagent.tooling.muzzle.matcher.Mismatch.MissingFlag +import static io.opentelemetry.javaagent.tooling.muzzle.matcher.Mismatch.MissingMethod import static muzzle.TestClasses.MethodBodyAdvice import external.LibraryBaseClass import io.opentelemetry.instrumentation.TestHelperClasses import io.opentelemetry.instrumentation.test.utils.ClasspathUtils -import io.opentelemetry.javaagent.tooling.muzzle.matcher.Mismatch import io.opentelemetry.javaagent.extension.muzzle.Reference import io.opentelemetry.javaagent.extension.muzzle.Reference.Source -import io.opentelemetry.javaagent.tooling.muzzle.matcher.ReferenceMatcher import io.opentelemetry.javaagent.tooling.muzzle.collector.ReferenceCollector +import io.opentelemetry.javaagent.tooling.muzzle.matcher.Mismatch +import io.opentelemetry.javaagent.tooling.muzzle.matcher.ReferenceMatcher import net.bytebuddy.jar.asm.Type import spock.lang.Shared import spock.lang.Specification @@ -153,13 +159,15 @@ class ReferenceMatcherTest extends Specification { getMismatchClassSet(mismatches) == expectedMismatches as Set where: - fieldName | fieldType | fieldFlags | classToCheck | expectedMismatches | fieldTestDesc - "missingField" | "Ljava/lang/String;" | [] | MethodBodyAdvice.A | [MissingField] | "mismatch missing field" - "privateField" | "Ljava/lang/String;" | [] | MethodBodyAdvice.A | [MissingField] | "mismatch field type signature" - "privateField" | "Ljava/lang/Object;" | [PRIVATE_OR_HIGHER] | MethodBodyAdvice.A | [] | "match private field" - "privateField" | "Ljava/lang/Object;" | [PROTECTED_OR_HIGHER] | MethodBodyAdvice.A2 | [MissingFlag] | "mismatch private field in supertype" - "protectedField" | "Ljava/lang/Object;" | [STATIC] | MethodBodyAdvice.A | [MissingFlag] | "mismatch static field" - "staticB" | Type.getType(MethodBodyAdvice.B).getDescriptor() | [STATIC, PROTECTED_OR_HIGHER] | MethodBodyAdvice.A | [] | "match static field" + fieldName | fieldType | fieldFlags | classToCheck | expectedMismatches | fieldTestDesc + "missingField" | "Ljava/lang/String;" | [] | MethodBodyAdvice.A | [MissingField] | "mismatch missing field" + "privateField" | "Ljava/lang/String;" | [] | MethodBodyAdvice.A | [MissingField] | "mismatch field type signature" + "privateField" | "Ljava/lang/Object;" | [PRIVATE_OR_HIGHER] | MethodBodyAdvice.A | [] | "match private field" + "privateField" | "Ljava/lang/Object;" | [PROTECTED_OR_HIGHER] | MethodBodyAdvice.A2 | [MissingFlag] | "mismatch private field in supertype" + "protectedField" | "Ljava/lang/Object;" | [STATIC] | MethodBodyAdvice.A | [MissingFlag] | "mismatch static field" + "staticB" | Type.getType(MethodBodyAdvice.B).getDescriptor() | [STATIC, PROTECTED_OR_HIGHER] | MethodBodyAdvice.A | [] | "match static field" + "number" | "I" | [PACKAGE_OR_HIGHER] | MethodBodyAdvice.Primitives | [] | "match primitive int" + "flag" | "Z" | [PACKAGE_OR_HIGHER] | MethodBodyAdvice.Primitives | [] | "match primitive boolean" } def "should not check abstract #desc helper classes"() { diff --git a/testing-common/src/test/java/muzzle/TestClasses.java b/testing-common/src/test/java/muzzle/TestClasses.java index bf55b3426e..79e854925f 100644 --- a/testing-common/src/test/java/muzzle/TestClasses.java +++ b/testing-common/src/test/java/muzzle/TestClasses.java @@ -59,6 +59,11 @@ public class TestClasses { public static class A2 extends A {} + public static class Primitives { + int number = 1; + boolean flag = false; + } + public interface SomeInterface { void someMethod(); }