Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RecordsFilter should filter out accessors #1393

Merged
merged 20 commits into from
Feb 6, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@ public class RecordsTarget {
record WithoutFields() { // assertFullyCovered()
}

record WithFields( // assertPartlyCovered()
record WithFields( // assertFullyCovered()
int x // assertEmpty()
) {
}

record WithCustomMethods(int x) { // assertFullyCovered()
public int x() {
return x; // assertNotCovered()
return x; // assertEmpty()
}

public String toString() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,4 +172,74 @@ public void should_not_filter_non_records() {
assertIgnored();
}

@Test
public void should_filter_field_int() {
context.superClassName = "java/lang/Record";
final MethodNode m = new MethodNode(InstrSupport.ASM_API_VERSION, 0,
"foo", "()Z", null, null);
m.visitVarInsn(Opcodes.ALOAD, 0);
m.visitFieldInsn(Opcodes.GETFIELD, "Dunno", "foo", "Z");
m.visitInsn(Opcodes.IRETURN);

filter.filter(m, context, output);

assertMethodIgnored(m);
}

@Test
public void should_filter_field_object() {
context.superClassName = "java/lang/Record";
final MethodNode m = new MethodNode(InstrSupport.ASM_API_VERSION, 0,
"foo", "()Ljava/lang/String;", null, null);
m.visitVarInsn(Opcodes.ALOAD, 0);
m.visitFieldInsn(Opcodes.GETFIELD, "Dunno", "foo", "Ljava/lang/String");
m.visitInsn(Opcodes.LRETURN);

filter.filter(m, context, output);

assertMethodIgnored(m);
}

@Test
public void should_not_filter_redirect_method() {
context.superClassName = "java/lang/Record";
final MethodNode m = new MethodNode(InstrSupport.ASM_API_VERSION, 0,
"foo", "()Ljava/lang/String;", null, null);
m.visitVarInsn(Opcodes.ALOAD, 0);
// The method name is different from the field name
m.visitFieldInsn(Opcodes.GETFIELD, "Dunno", "bar", "Ljava/lang/String");
m.visitInsn(Opcodes.LRETURN);

filter.filter(m, context, output);

assertIgnored();
}

@Test
public void should_not_filter_noreturn_method() {
context.superClassName = "java/lang/Record";
final MethodNode m = new MethodNode(InstrSupport.ASM_API_VERSION, 0,
"foo", "()Ljava/lang/String;", null, null);
m.visitVarInsn(Opcodes.ALOAD, 0);
m.visitFieldInsn(Opcodes.GETFIELD, "Dunno", "foo", "Ljava/lang/String");

filter.filter(m, context, output);

assertIgnored();
}

@Test
public void should_not_filter_other_method() {
context.superClassName = "java/lang/Record";
final MethodNode m = new MethodNode(InstrSupport.ASM_API_VERSION, 0,
"foo", "()Ljava/lang/String;", null, null);
m.visitVarInsn(Opcodes.ALOAD, 0);
m.visitFieldInsn(Opcodes.GETFIELD, "Dunno", "foo", "Ljava/lang/String");
// Not a return statement
m.visitFieldInsn(Opcodes.GETFIELD, "Dunno", "foo", "Ljava/lang/String");

filter.filter(m, context, output);

assertIgnored();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import org.objectweb.asm.Handle;
import org.objectweb.asm.Opcodes;
import org.objectweb.asm.tree.FieldInsnNode;
import org.objectweb.asm.tree.InvokeDynamicInsnNode;
import org.objectweb.asm.tree.MethodNode;

Expand All @@ -30,7 +31,8 @@ public void filter(final MethodNode methodNode,
}
final Matcher matcher = new Matcher();
if (matcher.isEquals(methodNode) || matcher.isHashCode(methodNode)
|| matcher.isToString(methodNode)) {
|| matcher.isToString(methodNode)
|| matcher.isFieldAccessor(methodNode)) {
output.ignore(methodNode.instructions.getFirst(),
methodNode.instructions.getLast());
}
Expand Down Expand Up @@ -58,6 +60,48 @@ boolean isHashCode(final MethodNode m) {
return cursor != null;
}

/**
* Criteria: method name == field name, only three instructions (aload0,
* getField, return), and note that this class only happens in a record,
* so it's safe to assume that this is the record field accessor
* generated. It may happen that the code is explicitly written by the
* developer and is intentionally kept the same as the default generated
* format, but that's just trivial code, and it still makes sense to
* filter them out anyway.
* <p>
* Exception: if the code is compiled within IntelliJ IDEA's Java
* instrumentation, there will be extra null-assertion instructions
* after the getField instruction. This case is <emph>ignored</emph>.
*/
boolean isFieldAccessor(final MethodNode m) {
// No parameter
if (!m.desc.startsWith("()")) {
return false;
}
ice1000 marked this conversation as resolved.
Show resolved Hide resolved
firstIsALoad0(m);
nextIs(Opcodes.GETFIELD);
if (!(cursor instanceof FieldInsnNode)) {
return false;
}
if (!((FieldInsnNode) cursor).name.equals(m.name)) {
return false;
}
next();
if (cursor == null) {
return false;
}
switch (cursor.getOpcode()) {
case Opcodes.IRETURN:
case Opcodes.LRETURN:
case Opcodes.FRETURN:
case Opcodes.DRETURN:
case Opcodes.ARETURN:
return true;
default:
return false;
}
}

boolean isEquals(final MethodNode m) {
if (!"equals".equals(m.name)
|| !"(Ljava/lang/Object;)Z".equals(m.desc)) {
Expand Down
3 changes: 3 additions & 0 deletions org.jacoco.doc/docroot/doc/changes.html
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ <h3>New Features</h3>
<li>Add parameter to include the current project in the <code>report-aggregate</code>
Maven goal
(GitHub <a href="https://github.com/jacoco/jacoco/issues/1007">#1007</a>).</li>
<li>Component accessors generated by the Java compilers for records are filtered
out during generation of report. Contributed by Tesla Zhang‮
ice1000 marked this conversation as resolved.
Show resolved Hide resolved
(GitHub <a href="https://github.com/jacoco/jacoco/issues/1393">#1393</a>).</li>
</ul>

<h3>Non-functional Changes</h3>
Expand Down