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

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java

@@ -436,12 +436,14 @@ public static class Field {
     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 Type getType() {
       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 Field merge(Field anotherField) {
           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 Builder withFlag(Flag flag) {
     }
 
     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);

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/MuzzleCodeGenerator.java

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

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollectingClassVisitor.java

@@ -209,8 +209,15 @@ public FieldVisitor visitField(
     // 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 @@ public void visitFieldInsn(int opcode, String owner, String name, String descrip
                   },
                   fieldFlags.toArray(new Reference.Flag[0]),
                   name,
-                  fieldType)
+                  fieldType,
+                  false)
               .build());
 
       Type underlyingFieldType = underlyingType(fieldType);

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/HelperReferenceWrapper.java

@@ -14,6 +14,7 @@
 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 int hashCode() {
     }
   }
 
+  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 @@ private Method toMethod(Reference.Method method) {
             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 @@ private Method toMethod(InDefinedShape method) {
         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());
+      }
     }
   }
 }

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/ReferenceMatcher.java

@@ -25,6 +25,7 @@
 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 @@ private List<Mismatch> checkMatch(Reference reference, ClassLoader loader) {
   }
 
   // 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 @@ private List<Mismatch> checkHelperClassMatch(Reference helperClass, TypePool typ
     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) {
 

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

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