Muzzle should check whether used fields are declared somewhere
MODIFY
javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java
javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java
@@ -436,12 +436,14 @@ public final class Reference {
private final Set<Flag> flags;
private final String name;
private final Type type;
+ private final boolean declared;
- public Field(Source[] sources, Flag[] flags, String name, Type fieldType) {
+ public Field(Source[] sources, Flag[] flags, String name, Type fieldType, boolean declared) {
this.sources = new LinkedHashSet<>(Arrays.asList(sources));
this.flags = new LinkedHashSet<>(Arrays.asList(flags));
this.name = name;
- type = fieldType;
+ this.type = fieldType;
+ this.declared = declared;
}
public String getName() {
@@ -460,6 +462,14 @@ public final class Reference {
return type;
}
+ /**
+ * Denotes whether this field is declared in the class {@link Reference} it is a part of. If
+ * {@code false} then this field is just used and most likely is declared in the super class.
+ */
+ public boolean isDeclared() {
+ return declared;
+ }
+
public Field merge(Field anotherField) {
if (!equals(anotherField) || !type.equals(anotherField.type)) {
throw new IllegalStateException("illegal merge " + this + " != " + anotherField);
@@ -468,7 +478,8 @@ public final class Reference {
Reference.merge(sources, anotherField.sources).toArray(new Source[0]),
mergeFlags(flags, anotherField.flags).toArray(new Flag[0]),
name,
- type);
+ type,
+ declared || anotherField.declared);
}
@Override
@@ -537,8 +548,8 @@ public final class Reference {
}
public Builder withField(
- Source[] sources, Flag[] fieldFlags, String fieldName, Type fieldType) {
- Field field = new Field(sources, fieldFlags, fieldName, fieldType);
+ Source[] sources, Flag[] fieldFlags, String fieldName, Type fieldType, boolean declared) {
+ Field field = new Field(sources, fieldFlags, fieldName, fieldType, declared);
int existingIndex = fields.indexOf(field);
if (existingIndex == -1) {
fields.add(field);

MODIFY
javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/MuzzleCodeGenerator.java
javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/MuzzleCodeGenerator.java
@@ -399,6 +399,9 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper {
false);
}
+ // declared flag
+ mv.visitLdcInsn(field.isDeclared());
+
mv.visitMethodInsn(
Opcodes.INVOKEVIRTUAL,
"io/opentelemetry/javaagent/tooling/muzzle/Reference$Builder",
@@ -409,7 +412,8 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper {
Reference.Source[].class,
Reference.Flag[].class,
String.class,
- Type.class)),
+ Type.class,
+ boolean.class)),
false);
}
for (Reference.Method method : references[i].getMethods()) {

MODIFY
javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollectingClassVisitor.java
javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollectingClassVisitor.java
@@ -209,8 +209,15 @@ class ReferenceCollectingClassVisitor extends ClassVisitor {
// Additional references we could check
// - annotations on field
- // intentionally not creating refs to fields here.
- // Will create refs in method instructions to include line numbers.
+ Type fieldType = Type.getType(descriptor);
+
+ // remember that this field was declared in the currently visited helper class
+ addReference(
+ new Reference.Builder(refSourceClassName)
+ .withSource(refSourceClassName)
+ .withField(new Source[0], new Flag[0], name, fieldType, true)
+ .build());
+
return super.visitField(access, name, descriptor, signature, value);
}
@@ -328,7 +335,8 @@ class ReferenceCollectingClassVisitor extends ClassVisitor {
},
fieldFlags.toArray(new Reference.Flag[0]),
name,
- fieldType)
+ fieldType,
+ false)
.build());
Type underlyingFieldType = underlyingType(fieldType);

MODIFY
javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/HelperReferenceWrapper.java
javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/HelperReferenceWrapper.java
@@ -14,6 +14,7 @@ import io.opentelemetry.javaagent.tooling.muzzle.Reference.Flag.VisibilityFlag;
import java.util.Map;
import java.util.Objects;
import java.util.stream.Stream;
+import net.bytebuddy.description.field.FieldDescription;
import net.bytebuddy.description.method.MethodDescription.InDefinedShape;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.pool.TypePool;
@@ -38,6 +39,9 @@ public interface HelperReferenceWrapper {
/** Returns an iterable with all non-private, non-static methods declared in the wrapped type. */
Stream<Method> getMethods();
+ /** Returns an iterable with all non-private fields declared in the wrapped type. */
+ Stream<Field> getFields();
+
final class Method {
private final boolean isAbstract;
private final String declaringClass;
@@ -85,6 +89,41 @@ public interface HelperReferenceWrapper {
}
}
+ final class Field {
+ private final String name;
+ private final String descriptor;
+
+ public Field(String name, String descriptor) {
+ this.name = name;
+ this.descriptor = descriptor;
+ }
+
+ public String getName() {
+ return name;
+ }
+
+ public String getDescriptor() {
+ return descriptor;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ Field field = (Field) o;
+ return Objects.equals(name, field.name) && Objects.equals(descriptor, field.descriptor);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(name, descriptor);
+ }
+ }
+
class Factory {
private final TypePool classpathPool;
private final Map<String, Reference> helperReferences;
@@ -161,6 +200,21 @@ public interface HelperReferenceWrapper {
method.getName(),
method.getDescriptor());
}
+
+ @Override
+ public Stream<Field> getFields() {
+ return reference.getFields().stream()
+ .filter(this::isDeclaredAndNotPrivate)
+ .map(this::toField);
+ }
+
+ private boolean isDeclaredAndNotPrivate(Reference.Field field) {
+ return field.isDeclared() && !field.getFlags().contains(VisibilityFlag.PRIVATE);
+ }
+
+ private Field toField(Reference.Field field) {
+ return new Field(field.getName(), field.getType().getDescriptor());
+ }
}
private static final class ClasspathType implements HelperReferenceWrapper {
@@ -210,6 +264,19 @@ public interface HelperReferenceWrapper {
return new Method(
method.isAbstract(), type.getName(), method.getInternalName(), method.getDescriptor());
}
+
+ @Override
+ public Stream<Field> getFields() {
+ return type.getDeclaredFields().stream().filter(this::isNotPrivate).map(this::toField);
+ }
+
+ private boolean isNotPrivate(FieldDescription.InDefinedShape field) {
+ return !field.isPrivate();
+ }
+
+ private Field toField(FieldDescription.InDefinedShape field) {
+ return new Field(field.getName(), field.getDescriptor());
+ }
}
}
}

MODIFY
javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/ReferenceMatcher.java
javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/ReferenceMatcher.java
@@ -25,6 +25,7 @@ import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.Predicate;
+import java.util.stream.Collectors;
import net.bytebuddy.description.field.FieldDescription;
import net.bytebuddy.description.method.MethodDescription;
import net.bytebuddy.description.type.TypeDescription;
@@ -146,12 +147,38 @@ public final class ReferenceMatcher {
}
// for helper classes we make sure that all abstract methods from super classes and interfaces are
- // implemented
+ // implemented and that all accessed fields are defined somewhere in the type hierarchy
private List<Mismatch> checkHelperClassMatch(Reference helperClass, TypePool typePool) {
List<Mismatch> mismatches = emptyList();
HelperReferenceWrapper helperWrapper = new Factory(typePool, references).create(helperClass);
+ Set<HelperReferenceWrapper.Field> undeclaredFields =
+ helperClass.getFields().stream()
+ .filter(f -> !f.isDeclared())
+ .map(f -> new HelperReferenceWrapper.Field(f.getName(), f.getType().getDescriptor()))
+ .collect(Collectors.toSet());
+
+ // if there are any fields in this helper class that's not declared here, check the type
+ // hierarchy
+ if (!undeclaredFields.isEmpty()) {
+ Set<HelperReferenceWrapper.Field> superClassFields = new HashSet<>();
+ collectFieldsFromTypeHierarchy(helperWrapper, superClassFields);
+
+ 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()));
+ }
+ }
+
+ // skip abstract method check if this type does not have super type or is abstract
if (!helperWrapper.hasSuperTypes() || helperWrapper.isAbstract()) {
return mismatches;
}
@@ -177,6 +204,13 @@ public final class ReferenceMatcher {
return mismatches;
}
+ private static void collectFieldsFromTypeHierarchy(
+ HelperReferenceWrapper type, Set<HelperReferenceWrapper.Field> fields) {
+
+ type.getFields().forEach(fields::add);
+ type.getSuperTypes().forEach(superType -> collectFieldsFromTypeHierarchy(superType, fields));
+ }
+
private static void collectMethodsFromTypeHierarchy(
HelperReferenceWrapper type, Set<Method> abstractMethods, Set<Method> plainMethods) {

MODIFY
testing-common/src/test/groovy/muzzle/HelperReferenceWrapperTest.groovy
testing-common/src/test/groovy/muzzle/HelperReferenceWrapperTest.groovy
@@ -30,6 +30,9 @@ class HelperReferenceWrapperTest extends Specification {
.withSuperName(baseHelperClass.className)
.withInterface(HelperReferenceWrapperTestClasses.Interface2.name)
.withMethod(new Reference.Source[0], new Reference.Flag[0], "bar", Type.VOID_TYPE)
+ .withField(new Reference.Source[0], new Reference.Flag[0], "field", Type.getType("Ljava/lang/Object;"), false)
+ .withField(new Reference.Source[0], new Reference.Flag[0], "declaredField", Type.getType("Ljava/lang/Object;"), true)
+ .withField(new Reference.Source[0], [Reference.Flag.VisibilityFlag.PRIVATE] as Reference.Flag[], "privateFieldsAreSkipped", Type.getType("Ljava/lang/Object;"), true)
.build()
def "should wrap helper types"() {
@@ -56,6 +59,14 @@ class HelperReferenceWrapperTest extends Specification {
}
}
+ with(helper.fields.collect(toList())) {
+ it.size() == 1
+ with(it[0]) {
+ it.name == "declaredField"
+ it.descriptor == "Ljava/lang/Object;"
+ }
+ }
+
helper.hasSuperTypes()
with(helper.superTypes.collect(toList())) {
it.size() == 2
@@ -84,6 +95,14 @@ class HelperReferenceWrapperTest extends Specification {
abstractClasspathType.getMethods().collect(toList()).isEmpty()
+ with(abstractClasspathType.fields.collect(toList())) {
+ it.size() == 1
+ with(it[0]) {
+ it.name == "field"
+ it.descriptor == "Ljava/lang/Object;"
+ }
+ }
+
abstractClasspathType.hasSuperTypes()
with(abstractClasspathType.superTypes.collect(toList())) {
it.size() == 2

MODIFY
testing-common/src/test/groovy/muzzle/ReferenceCollectorTest.groovy
testing-common/src/test/groovy/muzzle/ReferenceCollectorTest.groovy
@@ -148,6 +148,28 @@ class ReferenceCollectorTest extends Specification {
}
}
+ def "should collect field declaration references"() {
+ when:
+ def collector = new ReferenceCollector({ it == DeclaredFieldTestClass.Helper.name })
+ collector.collectReferencesFromAdvice(DeclaredFieldTestClass.Advice.name)
+ def references = collector.references
+
+ then:
+ println references
+
+ with(references[DeclaredFieldTestClass.Helper.name]) {helperClass ->
+ def superField = findField(helperClass, 'superField')
+ !superField.declared
+
+ def field = findField(helperClass, 'helperField')
+ field.declared
+ }
+
+ with(references[DeclaredFieldTestClass.LibraryBaseClass.name]) {libraryBaseClass ->
+ libraryBaseClass.fields.empty
+ }
+ }
+
def "should find all helper classes"() {
when:
def collector = new ReferenceCollector({ false })

MODIFY
testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy
testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy
@@ -18,6 +18,7 @@ import static io.opentelemetry.javaagent.tooling.muzzle.matcher.Mismatch.Missing
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.Reference
@@ -146,7 +147,7 @@ class ReferenceMatcherTest extends Specification {
def "field match #fieldTestDesc"() {
setup:
def reference = new Reference.Builder(classToCheck.name)
- .withField(new Source[0], fieldFlags as Reference.Flag[], fieldName, Type.getType(fieldType))
+ .withField(new Source[0], fieldFlags as Reference.Flag[], fieldName, Type.getType(fieldType), false)
.build()
when:
@@ -182,7 +183,7 @@ class ReferenceMatcherTest extends Specification {
def "should not check abstract #desc helper classes"() {
given:
- def reference = new Reference.Builder("io.opentelemetry.instrumentation.Helper")
+ def reference = new Reference.Builder(className)
.withSuperName(TestHelperClasses.HelperSuperClass.name)
.withFlag(ABSTRACT)
.withMethod(new Source[0], [ABSTRACT] as Reference.Flag[], "unimplemented", Type.VOID_TYPE)
@@ -197,7 +198,7 @@ class ReferenceMatcherTest extends Specification {
where:
desc | className
- "internal" | "com.external.otel.instrumentation.Helper"
+ "internal" | "io.opentelemetry.instrumentation.Helper"
"external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper"
}
@@ -217,7 +218,7 @@ class ReferenceMatcherTest extends Specification {
where:
desc | className
- "internal" | "com.external.otel.instrumentation.Helper"
+ "internal" | "io.opentelemetry.instrumentation.Helper"
"external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper"
}
@@ -237,7 +238,7 @@ class ReferenceMatcherTest extends Specification {
where:
desc | className
- "internal" | "com.external.otel.instrumentation.Helper"
+ "internal" | "io.opentelemetry.instrumentation.Helper"
"external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper"
}
@@ -259,7 +260,7 @@ class ReferenceMatcherTest extends Specification {
where:
desc | className
- "internal" | "com.external.otel.instrumentation.Helper"
+ "internal" | "io.opentelemetry.instrumentation.Helper"
"external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper"
}
@@ -286,10 +287,52 @@ class ReferenceMatcherTest extends Specification {
where:
desc | className
- "internal" | "com.external.otel.instrumentation.Helper"
+ "internal" | "io.opentelemetry.instrumentation.Helper"
+ "external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper"
+ }
+
+ def "should check #desc helper class whether used fields are declared in the super class"() {
+ given:
+ def helper = new Reference.Builder(className)
+ .withSuperName(LibraryBaseClass.name)
+ .withField(new Source[0], new Reference.Flag[0], "field", Type.getType("Ljava/lang/Integer;"), false)
+ .build()
+
+ when:
+ def mismatches = createMatcher([helper], [helper.className])
+ .getMismatchedReferenceSources(this.class.classLoader)
+
+ then:
+ mismatches.empty
+
+ where:
+ desc | className
+ "internal" | "io.opentelemetry.instrumentation.Helper"
"external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper"
}
+ def "should fail helper class when it uses fields undeclared in the super class: #desc"() {
+ given:
+ def helper = new Reference.Builder(className)
+ .withSuperName(LibraryBaseClass.name)
+ .withField(new Source[0], new Reference.Flag[0], fieldName, Type.getType(fieldType), false)
+ .build()
+
+ when:
+ def mismatches = createMatcher([helper], [helper.className])
+ .getMismatchedReferenceSources(this.class.classLoader)
+
+ then:
+ getMismatchClassSet(mismatches) == [MissingField] as Set
+
+ where:
+ desc | className | fieldName | fieldType
+ "internal helper, different field name" | "io.opentelemetry.instrumentation.Helper" | "differentField" | "Ljava/lang/Integer;"
+ "internal helper, different field type" | "io.opentelemetry.instrumentation.Helper" | "field" | "Lcom/external/DifferentType;"
+ "external helper, different field name" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper" | "differentField" | "Ljava/lang/Integer;"
+ "external helper, different field type" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper" | "field" | "Lcom/external/DifferentType;"
+ }
+
private static ReferenceMatcher createMatcher(Collection<Reference> references = [],
List<String> helperClasses = []) {
new ReferenceMatcher(helperClasses, references as Reference[], { it.startsWith(TEST_EXTERNAL_INSTRUMENTATION_PACKAGE) })

ADD
testing-common/src/test/java/external/LibraryBaseClass.java
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package external;
+
+public abstract class LibraryBaseClass {
+ protected Integer field;
+}

ADD
testing-common/src/test/java/muzzle/DeclaredFieldTestClass.java
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package muzzle;
+
+public class DeclaredFieldTestClass {
+ public static class Advice {
+ public void instrument() {
+ new Helper().foo();
+ }
+ }
+
+ public static class Helper extends LibraryBaseClass {
+ private String helperField;
+
+ public void foo() {
+ superField.toString();
+ }
+ }
+
+ public static class LibraryBaseClass {
+ protected Object superField;
+ }
+}

MODIFY
testing-common/src/test/java/muzzle/HelperReferenceWrapperTestClasses.java
testing-common/src/test/java/muzzle/HelperReferenceWrapperTestClasses.java
@@ -15,6 +15,9 @@ public class HelperReferenceWrapperTestClasses {
}
abstract static class AbstractClasspathType implements Interface1 {
+ private Object privateFieldsAreIgnored;
+ protected Object field;
+
static void staticMethodsAreIgnored() {}
private void privateMethodsToo() {}

Muzzle should check whether used fields are declared somewhere (#2870)