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

Add filter for Java assert statement #1196

Merged
merged 5 commits into from Jun 10, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
@@ -0,0 +1,27 @@
/*******************************************************************************
* Copyright (c) 2009, 2021 Mountainminds GmbH & Co. KG and Contributors
* This program and the accompanying materials are made available under
* the terms of the Eclipse Public License 2.0 which is available at
* http://www.eclipse.org/legal/epl-2.0
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* Evgeny Mandrikov - initial API and implementation
*
*******************************************************************************/
package org.jacoco.core.test.validation.java5;

import org.jacoco.core.test.validation.ValidationTestBase;
import org.jacoco.core.test.validation.java5.targets.AssertTarget;

/**
* Test of code coverage in {@link AssertTarget}.
*/
public class AssertTest extends ValidationTestBase {

public AssertTest() {
super(AssertTarget.class);
}

}
@@ -0,0 +1,29 @@
/*******************************************************************************
* Copyright (c) 2009, 2021 Mountainminds GmbH & Co. KG and Contributors
* This program and the accompanying materials are made available under
* the terms of the Eclipse Public License 2.0 which is available at
* http://www.eclipse.org/legal/epl-2.0
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* Evgeny Mandrikov - initial API and implementation
*
*******************************************************************************/
package org.jacoco.core.test.validation.java5.targets;

import static org.jacoco.core.test.validation.targets.Stubs.t;

/**
* This target exercises assert statement.
*/
public class AssertTarget { // assertFullyCovered()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What instructions are left on this line? Shouldn't it be empty?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marchof we ignore only part of static initializer that corresponds to initialization of field $assertionsDisabled, so in this case there is also return with line number of class:

  static {};
    descriptor: ()V
    flags: (0x0008) ACC_STATIC
    Code:
      stack=1, locals=0, args_size=0
         0: ldc           #7                  // class org/jacoco/core/test/validation/java5/targets/AssertTarget
         2: invokevirtual #8                  // Method java/lang/Class.desiredAssertionStatus:()Z
         5: ifne          12
         8: iconst_1
         9: goto          13
        12: iconst_0
        13: putstatic     #2                  // Field $assertionsDisabled:Z
        16: return
      LineNumberTable:
        line 20: 0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether we should also ignore the return if there is no other code. Otherwise we have this line highlighted only if there is a assertion statement in this class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also if I add a static initializer the return is accounted to that line and the highlighting of the class definition disappears. I think it is more consistent if we ignore the return statement if there is no other code:

static Object C = new Object();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line highlighted only if there is a assertion statement

Also at least default constructor has line of class declaration and part of static initializer generated for enums.

if we ignore the return statement if there is no other code

This will also exclude line of explicitly written empty static initializer in class with assert statements:

//
class Example { // line 2
    static {
    } // line 4

    void example(boolean b) {
        assert(b);
    }
}
  Example();
    descriptor: ()V
    flags: (0x0000)
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokespecial #1                  // Method java/lang/Object."<init>":()V
         4: return
      LineNumberTable:
        line 2: 0

  static {};
    descriptor: ()V
    flags: (0x0008) ACC_STATIC
    Code:
      stack=1, locals=0, args_size=0
         0: ldc           #5                  // class Example
         2: invokevirtual #6                  // Method java/lang/Class.desiredAssertionStatus:()Z
         5: ifne          12
         8: iconst_1
         9: goto          13
        12: iconst_0
        13: putstatic     #2                  // Field $assertionsDisabled:Z
        16: return
      LineNumberTable:
        line 2: 0
        line 4: 16

I believe that our users are more concerned about branch that without this change is displayed on a line of class declaration. And not sure whether they will be concerned about just highlighting of this line or about absense of highlighting for explicitly written empty static initializers.

Should we then also for consistency also filter explicitly written empty static initializers in absence of assert statements? Is it worth it? I'm not against both, just wondering how deep this rabbit hole can go to not overlook something - wondering if there might be other cases that we currently overlooking?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, looks like we will end up with too many corner cases. We leave it like it is!


private AssertTarget() {
}

public static void main(String[] args) {
assert t() : "msg"; // assertPartlyCovered(1, 1)
}

}
Expand Up @@ -137,6 +137,42 @@ public void nextIsVar() {
matcher.nextIsVar(Opcodes.ILOAD, "name");
}

@Test
public void nextIsField() {
m.visitInsn(Opcodes.NOP);
m.visitFieldInsn(Opcodes.PUTSTATIC, "owner", "name", "Z");

// should set cursor to null when opcode mismatch
matcher.cursor = m.instructions.getFirst();
matcher.nextIsField(Opcodes.GETSTATIC, "owner", "name", "Z");
assertNull(matcher.cursor);

// should set cursor to null when owner mismatch
matcher.cursor = m.instructions.getFirst();
matcher.nextIsField(Opcodes.PUTSTATIC, "another_owner", "name", "Z");
assertNull(matcher.cursor);

// should set cursor to null when name mismatch
matcher.cursor = m.instructions.getFirst();
matcher.nextIsField(Opcodes.PUTSTATIC, "owner", "another_name", "Z");
assertNull(matcher.cursor);

// should set cursor to null when descriptor mismatch
matcher.cursor = m.instructions.getFirst();
matcher.nextIsField(Opcodes.PUTSTATIC, "owner", "name",
"another_descriptor");
assertNull(matcher.cursor);

// should set cursor to next instruction when match
matcher.cursor = m.instructions.getFirst();
matcher.nextIsField(Opcodes.PUTSTATIC, "owner", "name", "Z");
assertSame(m.instructions.getLast(), matcher.cursor);

// should not do anything when cursor is null
matcher.cursor = null;
matcher.nextIsField(Opcodes.PUTSTATIC, "owner", "name", "Z");
}

@Test
public void nextIsInvoke() {
m.visitInsn(Opcodes.NOP);
Expand Down
@@ -0,0 +1,145 @@
/*******************************************************************************
* Copyright (c) 2009, 2021 Mountainminds GmbH & Co. KG and Contributors
* This program and the accompanying materials are made available under
* the terms of the Eclipse Public License 2.0 which is available at
* http://www.eclipse.org/legal/epl-2.0
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* Evgeny Mandrikov - initial API and implementation
*
*******************************************************************************/
package org.jacoco.core.internal.analysis.filter;

import org.jacoco.core.internal.instr.InstrSupport;
import org.junit.Test;
import org.objectweb.asm.Label;
import org.objectweb.asm.Opcodes;
import org.objectweb.asm.Type;
import org.objectweb.asm.tree.MethodNode;

/**
* Unit tests for {@link AssertFilter}.
*/
public class AssertFilterTest extends FilterTestBase {

private final AssertFilter filter = new AssertFilter();

/**
* <code><pre>
* class Example {
* void example(boolean b) {
* ...
* assert b : "message";
* ...
* }
* }
* </pre></code>
*/
@Test
public void should_filter_static_initializer() {
context.className = "Example";
final MethodNode m = new MethodNode(InstrSupport.ASM_API_VERSION,
Opcodes.ACC_STATIC, "<clinit>", "()V", null, null);

m.visitLdcInsn(Type.getType("LExample;"));
final Range range = new Range();
range.fromInclusive = m.instructions.getLast();
m.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "java/lang/Class",
"desiredAssertionStatus", "()Z", false);
final Label label1 = new Label();
final Label label2 = new Label();
m.visitJumpInsn(Opcodes.IFNE, label1);
m.visitInsn(Opcodes.ICONST_1);
m.visitJumpInsn(Opcodes.GOTO, label2);
m.visitLabel(label1);
m.visitInsn(Opcodes.ICONST_0);
m.visitLabel(label2);
m.visitFieldInsn(Opcodes.PUTSTATIC, "Example", "$assertionsDisabled",
"Z");
range.toInclusive = m.instructions.getLast();
m.visitInsn(Opcodes.RETURN);

filter.filter(m, context, output);

assertIgnored(range);
}

/**
* <code><pre>
* class Example {
* static final f;
*
* static {
* f = !Example.class.desiredAssertionStatus();
* }
* }
* </pre></code>
*/
@Test
public void should_not_filter_static_initializer_when_field_name_does_not_match() {
context.className = "Example";
final MethodNode m = new MethodNode(InstrSupport.ASM_API_VERSION,
Opcodes.ACC_STATIC, "<clinit>", "()V", null, null);

m.visitLdcInsn(Type.getType("LExample;"));
m.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "java/lang/Class",
"desiredAssertionStatus", "()Z", false);
final Label label1 = new Label();
final Label label2 = new Label();
m.visitJumpInsn(Opcodes.IFNE, label1);
m.visitInsn(Opcodes.ICONST_1);
m.visitJumpInsn(Opcodes.GOTO, label2);
m.visitLabel(label1);
m.visitInsn(Opcodes.ICONST_0);
m.visitLabel(label2);
m.visitFieldInsn(Opcodes.PUTSTATIC, "Foo", "f", "Z");
m.visitInsn(Opcodes.RETURN);

filter.filter(m, context, output);

assertIgnored();
}

/**
* <code><pre>
* class Example {
* void example(boolean b) {
* ...
* assert b : "message";
* ...
* }
* }
* </pre></code>
*/
@Test
public void should_filter_assert() {
context.className = "Example";
final MethodNode m = new MethodNode(InstrSupport.ASM_API_VERSION, 0,
"example", "()V", null, null);

m.visitInsn(Opcodes.NOP);
m.visitFieldInsn(Opcodes.GETSTATIC, "Example", "$assertionsDisabled",
"Z");
final Label label = new Label();
m.visitJumpInsn(Opcodes.IFNE, label);
final Range range = new Range(m.instructions.getLast(),
m.instructions.getLast());
m.visitVarInsn(Opcodes.ILOAD, 1);
m.visitJumpInsn(Opcodes.IFNE, label);
m.visitTypeInsn(Opcodes.NEW, "java/lang/AssertionError");
m.visitInsn(Opcodes.DUP);
m.visitLdcInsn("message");
m.visitMethodInsn(Opcodes.INVOKESPECIAL, "java/lang/AssertionError",
"<init>", "(Ljava/lang/Object;)V", false);
m.visitInsn(Opcodes.ATHROW);
m.visitLabel(label);
m.visitInsn(Opcodes.NOP);

filter.filter(m, context, output);

assertIgnored(range);
}

}
Expand Up @@ -71,6 +71,7 @@ public void setup() throws Exception {

private ExecutionDataStore execute() throws Exception {
loader = new InstrumentingLoader(target);
loader.setDefaultAssertionStatus(true);
run(loader.loadClass(target.getName()));
return loader.collect();
}
Expand Down
Expand Up @@ -17,6 +17,7 @@

import org.objectweb.asm.Opcodes;
import org.objectweb.asm.tree.AbstractInsnNode;
import org.objectweb.asm.tree.FieldInsnNode;
import org.objectweb.asm.tree.MethodInsnNode;
import org.objectweb.asm.tree.MethodNode;
import org.objectweb.asm.tree.TypeInsnNode;
Expand Down Expand Up @@ -76,6 +77,25 @@ final void nextIsInvoke(final int opcode, final String owner,
cursor = null;
}

/**
* Moves {@link #cursor} to next instruction if it is {@link FieldInsnNode}
* with given opcode, owner, name and descriptor, otherwise sets it to
* <code>null</code>.
*/
final void nextIsField(final int opcode, final String owner,
final String name, final String descriptor) {
nextIs(opcode);
if (cursor == null) {
return;
}
final FieldInsnNode f = (FieldInsnNode) cursor;
if (owner.equals(f.owner) && name.equals(f.name)
&& descriptor.equals(f.desc)) {
return;
}
cursor = null;
}

final void nextIsVar(final int opcode, final String name) {
nextIs(opcode);
if (cursor == null) {
Expand Down
@@ -0,0 +1,66 @@
/*******************************************************************************
* Copyright (c) 2009, 2021 Mountainminds GmbH & Co. KG and Contributors
* This program and the accompanying materials are made available under
* the terms of the Eclipse Public License 2.0 which is available at
* http://www.eclipse.org/legal/epl-2.0
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* Evgeny Mandrikov - initial API and implementation
*
*******************************************************************************/
package org.jacoco.core.internal.analysis.filter;

import org.objectweb.asm.Opcodes;
import org.objectweb.asm.tree.AbstractInsnNode;
import org.objectweb.asm.tree.MethodNode;

/**
* Filters code that is generated for an <code>assert</code> statement.
*/
final class AssertFilter implements IFilter {

public void filter(final MethodNode methodNode,
final IFilterContext context, final IFilterOutput output) {
final Matcher matcher = new Matcher();
if ("<clinit>".equals(methodNode.name)) {
for (final AbstractInsnNode i : methodNode.instructions) {
matcher.matchSet(context.getClassName(), i, output);
}
}
for (final AbstractInsnNode i : methodNode.instructions) {
matcher.matchGet(context.getClassName(), i, output);
}
}

private static class Matcher extends AbstractMatcher {
public void matchSet(final String className,
final AbstractInsnNode start, final IFilterOutput output) {
cursor = start;
nextIsInvoke(Opcodes.INVOKEVIRTUAL, "java/lang/Class",
"desiredAssertionStatus", "()Z");
nextIs(Opcodes.IFNE);
nextIs(Opcodes.ICONST_1);
nextIs(Opcodes.GOTO);
nextIs(Opcodes.ICONST_0);
nextIsField(Opcodes.PUTSTATIC, className, "$assertionsDisabled",
"Z");
if (cursor != null) {
output.ignore(start, cursor);
}
}

public void matchGet(final String className,
final AbstractInsnNode start, final IFilterOutput output) {
cursor = start;
nextIsField(Opcodes.GETSTATIC, className, "$assertionsDisabled",
"Z");
nextIs(Opcodes.IFNE);
if (cursor != null) {
output.ignore(cursor, cursor);
Copy link

@morganga morganga Oct 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be ignoring the entire assert test here?
output.ignore(cursor, cursor) it's only ignoring the branch instruction, not the assert test.

I still get a "1 of 2 branches missed" for my assert statements. The only way to cover both branches is to run the test twice, once with assertions disabled, then again with assertions enabled, all in the same test suite.

How about output.ignore(cursor, ((JumpInsnNode) cursor).label);

I've tested the above code fragment and it marks the assert line as green, without complaining about 1 of 2 branches missed. (it also doesn't think the line has multiple branches either)

}
}
}

}
Expand Up @@ -37,7 +37,7 @@ public static IFilter all() {
new TryWithResourcesJavac11Filter(),
new TryWithResourcesJavacFilter(),
new TryWithResourcesEcjFilter(), new FinallyFilter(),
new PrivateEmptyNoArgConstructorFilter(),
new PrivateEmptyNoArgConstructorFilter(), new AssertFilter(),
new StringSwitchJavacFilter(), new StringSwitchFilter(),
new EnumEmptyConstructorFilter(), new RecordsFilter(),
new AnnotationGeneratedFilter(), new KotlinGeneratedFilter(),
Expand Down
7 changes: 7 additions & 0 deletions org.jacoco.doc/docroot/doc/changes.html
Expand Up @@ -20,6 +20,13 @@ <h1>Change History</h1>

<h2>Snapshot Build @qualified.bundle.version@ (@build.date@)</h2>

<h3>New Features</h3>
<ul>
<li>Part of bytecode generated by the Java compilers for <code>assert</code>
statement is filtered out during generation of report
(GitHub <a href="https://github.com/jacoco/jacoco/issues/1196">#1196</a>).</li>
</ul>

<h3>Fixed bugs</h3>
<ul>
<li>Fixed <code>NullPointerException</code> during filtering
Expand Down