Skip to content

KotlinSafeCallOperatorFilter should filter "unoptimized" chains #1818

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.

Sign up for GitHub

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

Merged
Godin merged 5 commits into from
Jan 6, 2025
Merged

KotlinSafeCallOperatorFilter should filter "unoptimized" chains #1818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ffd4cc4
(WIP) add validation test
Godin Dec 16, 2024
ec814c9
(WIP) add unit test
Godin Dec 16, 2024
9a88bcc
(WIP) fix
Godin Dec 17, 2024
3a70e1c
(WIP) update changelog
Godin Dec 17, 2024
f4e676a
Merge branch 'master' into kotlin_safe_call_chain
Godin Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions ...kotlin/src/org/jacoco/core/test/validation/kotlin/targets/KotlinSafeCallOperatorTarget.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
*******************************************************************************/
package org.jacoco.core.test.validation.kotlin.targets

import org.jacoco.core.test.validation.targets.Stubs.nop

/**
* Test target for [safe call operator (`?.`)](https://kotlinlang.org/docs/null-safety.html#safe-call-operator).
*/
Expand Down Expand Up @@ -52,10 +54,33 @@ object KotlinSafeCallOperatorTarget {
fullCoverage(A(B("")))
}

private fun safeCallChainMultiline() {
fun nullOnly(a: A?): String? =
a?.also { // assertPartlyCovered(1, 1)
nop(it) // assertNotCovered()
}?.b?.c // assertPartlyCovered(1, 1)

fun nonNullOnly(a: A?): String? =
a?.also { // assertPartlyCovered(1, 1)
nop(it) // assertFullyCovered()
}?.b?.c // assertFullyCovered(1, 1)

fun fullCoverage(a: A?): String? =
a?.also { // assertFullyCovered(0, 2)
nop(it) // assertFullyCovered()
}?.b?.c // assertFullyCovered(0, 2)

nullOnly(null)
nonNullOnly(A(B("")))
fullCoverage(null)
fullCoverage(A(B("")))
}

@JvmStatic
fun main(args: Array<String>) {
safeCall()
safeCallChain()
safeCallChainMultiline()
}

}
53 changes: 52 additions & 1 deletion ...e.test/src/org/jacoco/core/internal/analysis/filter/KotlinSafeCallOperatorFilterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public class KotlinSafeCallOperatorFilterTest extends FilterTestBase {
* https://github.com/JetBrains/kotlin/commit/0a67ab54fec635f82e0507cbdd4299ae0dbe71b0
*/
@Test
public void should_filter_safe_call_chain() {
public void should_filter_optimized_safe_call_chain() {
context.classAnnotations
.add(KotlinGeneratedFilter.KOTLIN_METADATA_DESC);
final MethodNode m = new MethodNode(InstrSupport.ASM_API_VERSION, 0,
Expand Down Expand Up @@ -80,4 +80,55 @@ public void should_filter_safe_call_chain() {
assertReplacedBranches(expected);
}

/**
* <pre>
* data class A(val b: B)
* data class B(val c: String)
* fun example(a: A?): String? =
* a
* ?.b
* ?.c
* </pre>
*/
@Test
public void should_filter_unoptimized_safe_call_chain() {
context.classAnnotations
.add(KotlinGeneratedFilter.KOTLIN_METADATA_DESC);
final MethodNode m = new MethodNode(InstrSupport.ASM_API_VERSION, 0,
"example", "(LA;)Ljava/lang/String;", null, null);
m.visitVarInsn(Opcodes.ALOAD, 0);
final Label label1 = new Label();
final Label label2 = new Label();

m.visitJumpInsn(Opcodes.IFNULL, label1);
final AbstractInsnNode i1 = m.instructions.getLast();
m.visitVarInsn(Opcodes.ALOAD, 0);
m.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "A", "getB", "()LB;", false);

m.visitVarInsn(Opcodes.ASTORE, 1);
m.visitVarInsn(Opcodes.ALOAD, 1);
m.visitJumpInsn(Opcodes.IFNULL, label1);
final AbstractInsnNode i2 = m.instructions.getLast();
m.visitVarInsn(Opcodes.ALOAD, 1);
final HashSet<AbstractInsnNode> r = new HashSet<AbstractInsnNode>();
r.add(m.instructions.getLast());
m.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "B", "getC",
"()Ljava/lang/String;", false);

m.visitJumpInsn(Opcodes.GOTO, label2);

m.visitLabel(label1);
m.visitInsn(Opcodes.ACONST_NULL);
r.add(m.instructions.getLast());
m.visitLabel(label2);

filter.filter(m, context, output);

assertIgnored();
final HashMap<AbstractInsnNode, Set<AbstractInsnNode>> expected = new HashMap<AbstractInsnNode, Set<AbstractInsnNode>>();
expected.put(i1, r);
expected.put(i2, r);
assertReplacedBranches(expected);
}

}
60 changes: 49 additions & 11 deletions ...acoco.core/src/org/jacoco/core/internal/analysis/filter/KotlinSafeCallOperatorFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.objectweb.asm.tree.JumpInsnNode;
import org.objectweb.asm.tree.LabelNode;
import org.objectweb.asm.tree.MethodNode;
import org.objectweb.asm.tree.VarInsnNode;

/**
* Filters bytecode that Kotlin compiler generates for chains of safe call
Expand All @@ -46,6 +47,8 @@ public void filter(final MethodNode methodNode,
}

/**
* "optimized" chain:
*
* <pre>
* DUP
* IFNULL label
Expand All @@ -60,26 +63,61 @@ public void filter(final MethodNode methodNode,
* label:
* POP
* </pre>
*
* "unoptimized" chain:
*
* <pre>
* ALOAD v0
* IFNULL label
* ... // call 0
*
* ...
*
* ASTORE v1
* ALOAD v1
* IFNULL label
* ... // call N
*
* label:
* ACONST_NULL
* </pre>
*/
private static Collection<ArrayList<JumpInsnNode>> findChains(
final MethodNode methodNode) {
final HashMap<AbstractInsnNode, ArrayList<JumpInsnNode>> chains = new HashMap<AbstractInsnNode, ArrayList<JumpInsnNode>>();
for (final AbstractInsnNode i : methodNode.instructions) {
if (i.getOpcode() == Opcodes.IFNULL
&& i.getPrevious().getOpcode() == Opcodes.DUP) {
final JumpInsnNode jump = (JumpInsnNode) i;
final LabelNode label = jump.label;
if (AbstractMatcher.skipNonOpcodes(label.getNext())
.getOpcode() != Opcodes.POP) {
if (i.getOpcode() != Opcodes.IFNULL) {
continue;
}
final JumpInsnNode jump = (JumpInsnNode) i;
final LabelNode label = jump.label;
final AbstractInsnNode target = AbstractMatcher
.skipNonOpcodes(label);
ArrayList<JumpInsnNode> chain = chains.get(label);
if (target.getOpcode() == Opcodes.POP) {
if (i.getPrevious().getOpcode() != Opcodes.DUP) {
continue;
}
} else if (target.getOpcode() == Opcodes.ACONST_NULL) {
if (i.getPrevious().getOpcode() != Opcodes.ALOAD) {
continue;
}
ArrayList<JumpInsnNode> chain = chains.get(label);
if (chain == null) {
chain = new ArrayList<JumpInsnNode>();
chains.put(label, chain);
if (chain != null) {
final AbstractInsnNode p1 = i.getPrevious();
final AbstractInsnNode p2 = p1.getPrevious();
if (p2 == null || p2.getOpcode() != Opcodes.ASTORE
|| ((VarInsnNode) p1).var != ((VarInsnNode) p2).var) {
continue;
}
}
chain.add(jump);
} else {
continue;
}
if (chain == null) {
chain = new ArrayList<JumpInsnNode>();
chains.put(label, chain);
}
chain.add(jump);
}
return chains.values();
}
Expand Down
3 changes: 2 additions & 1 deletion org.jacoco.doc/docroot/doc/changes.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ <h3>New Features</h3>
(GitHub <a href="https://github.com/jacoco/jacoco/issues/1769">#1769</a>).</li>
<li>Part of bytecode generated by the Kotlin compiler for chains of safe call
operators is filtered out during generation of report
(GitHub <a href="https://github.com/jacoco/jacoco/issues/1810">#1810</a>).</li>
(GitHub <a href="https://github.com/jacoco/jacoco/issues/1810">#1810</a>,
<a href="https://github.com/jacoco/jacoco/issues/1818">#1818</a>).</li>
<li>Method <code>getEntries</code> generated by the Kotlin compiler for enum
classes is filtered out during generation of report
(GitHub <a href="https://github.com/jacoco/jacoco/issues/1625">#1625</a>).</li>
Expand Down