Skip to content

Commit b0f8e23

Browse files
GodinGodin
authored
KotlinSafeCallOperatorFilter should filter "unoptimized" safe call followed by elvis (#1954)
1 parent c7bd3f4 commit b0f8e23

File tree

4 files changed

+228
-11
lines changed

4 files changed

+228
-11
lines changed

‎org.jacoco.core.test.validation.kotlin/src/org/jacoco/core/test/validation/kotlin/targets/KotlinSafeCallOperatorTarget.kt‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,28 @@ object KotlinSafeCallOperatorTarget {
126126
fullCoverage(B(""))
127127
}
128128

129+
private fun safeCallFollowedByElvisMultiline() {
130+
fun nullOnly(b: B?): String =
131+
b // assertPartlyCovered(1, 1)
132+
?.c // assertPartlyCovered(1, 1)
133+
?: "" // assertFullyCovered()
134+
135+
fun nonNullOnly(b: B?): String =
136+
b // assertFullyCovered(1, 1)
137+
?.c // assertFullyCovered(1, 1)
138+
?: "" // assertPartlyCovered()
139+
140+
fun fullCoverage(b: B?): String =
141+
b // assertFullyCovered(0, 2)
142+
?.c // assertFullyCovered(0, 2)
143+
?: "" // assertFullyCovered()
144+
145+
nullOnly(null)
146+
nonNullOnly(B(""))
147+
fullCoverage(null)
148+
fullCoverage(B(""))
149+
}
150+
129151
private fun safeCallChainFollowedByElvis() {
130152
fun nullOnly(a: A?): String =
131153
a?.b?.c ?: "" // assertPartlyCovered(3, 3)
@@ -142,6 +164,31 @@ object KotlinSafeCallOperatorTarget {
142164
fullCoverage(A(B("")))
143165
}
144166

167+
private fun safeCallChainFollowedByElvisMultiline() {
168+
fun nullOnly(a: A?): String =
169+
a // assertPartlyCovered(1, 1)
170+
?.b // assertPartlyCovered(1, 1)
171+
?.c // assertPartlyCovered(1, 1)
172+
?: "" // assertFullyCovered()
173+
174+
fun nonNullOnly(a: A?): String =
175+
a // assertFullyCovered(1, 1)
176+
?.b // assertFullyCovered(1, 1)
177+
?.c // assertFullyCovered(1, 1)
178+
?: "" // assertPartlyCovered()
179+
180+
fun fullCoverage(a: A?): String =
181+
a // assertFullyCovered(0, 2)
182+
?.b // assertFullyCovered(0, 2)
183+
?.c // assertFullyCovered(0, 2)
184+
?: "" // assertFullyCovered()
185+
186+
nullOnly(null)
187+
nonNullOnly(A(B("")))
188+
fullCoverage(null)
189+
fullCoverage(A(B("")))
190+
}
191+
145192
@JvmStatic
146193
fun main(args: Array<String>) {
147194
safeCall()
@@ -150,7 +197,9 @@ object KotlinSafeCallOperatorTarget {
150197
safeCallChainMultiline2()
151198
safeCallChainException()
152199
safeCallFollowedByElvis()
200+
safeCallFollowedByElvisMultiline()
153201
safeCallChainFollowedByElvis()
202+
safeCallChainFollowedByElvisMultiline()
154203
}
155204

156205
}

‎org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/KotlinSafeCallOperatorFilterTest.java‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,4 +233,107 @@ public void should_filter_safe_call_chain_followed_by_elvis() {
233233
assertReplacedBranches(m, replacements);
234234
}
235235

236+
/**
237+
* <pre>
238+
* data class B(val c: String)
239+
* fun example(b: B?): String =
240+
* b
241+
* ?.c
242+
* ?: ""
243+
* </pre>
244+
*/
245+
@Test
246+
public void should_filter_unoptimized_safe_call_followed_by_elvis() {
247+
final MethodNode m = new MethodNode(InstrSupport.ASM_API_VERSION, 0,
248+
"example", "(LB;)Ljava/lang/String;", null, null);
249+
m.visitVarInsn(Opcodes.ALOAD, 0);
250+
final Label label = new Label();
251+
m.visitJumpInsn(Opcodes.IFNULL, label);
252+
final AbstractInsnNode ifNullInstruction1 = m.instructions.getLast();
253+
m.visitVarInsn(Opcodes.ALOAD, 0);
254+
m.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "B", "getC",
255+
"()Ljava/lang/String;", false);
256+
m.visitVarInsn(Opcodes.ASTORE, 1);
257+
m.visitVarInsn(Opcodes.ALOAD, 1);
258+
m.visitJumpInsn(Opcodes.IFNULL, label);
259+
final AbstractInsnNode ifNullInstruction2 = m.instructions.getLast();
260+
m.visitVarInsn(Opcodes.ALOAD, 1);
261+
final Label next = new Label();
262+
m.visitJumpInsn(Opcodes.GOTO, next);
263+
m.visitLabel(label);
264+
m.visitLdcInsn("");
265+
final AbstractInsnNode nullTarget = m.instructions.getLast();
266+
m.visitLabel(next);
267+
m.visitInsn(Opcodes.ARETURN);
268+
269+
filter.filter(m, context, output);
270+
271+
assertIgnored(m);
272+
final HashMap<AbstractInsnNode, List<Replacement>> replacements = new HashMap<AbstractInsnNode, List<Replacement>>();
273+
replacements.put(ifNullInstruction1, Arrays.asList( //
274+
new Replacement(0, ifNullInstruction1, 0),
275+
new Replacement(1, nullTarget, 0)));
276+
replacements.put(ifNullInstruction2, Arrays.asList( //
277+
new Replacement(0, ifNullInstruction2, 0),
278+
new Replacement(1, nullTarget, 0)));
279+
assertReplacedBranches(m, replacements);
280+
}
281+
282+
/**
283+
* <pre>
284+
* data class A(val b: B)
285+
* data class B(val c: String)
286+
* fun example(a: A?): String? =
287+
* a
288+
* ?.b
289+
* ?.c
290+
* ?: ""
291+
* </pre>
292+
*/
293+
@Test
294+
public void should_filter_unoptimized_safe_call_chain_followed_by_elvis() {
295+
final MethodNode m = new MethodNode(InstrSupport.ASM_API_VERSION, 0,
296+
"example", "(LA;)Ljava/lang/String;", null, null);
297+
m.visitVarInsn(Opcodes.ALOAD, 0);
298+
final Label label = new Label();
299+
m.visitJumpInsn(Opcodes.IFNULL, label);
300+
final AbstractInsnNode ifNullInstruction1 = m.instructions.getLast();
301+
m.visitVarInsn(Opcodes.ALOAD, 0);
302+
m.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "A", "getB", "()LB;", false);
303+
m.visitVarInsn(Opcodes.ASTORE, 1);
304+
m.visitVarInsn(Opcodes.ALOAD, 1);
305+
m.visitJumpInsn(Opcodes.IFNULL, label);
306+
final AbstractInsnNode ifNullInstruction2 = m.instructions.getLast();
307+
m.visitVarInsn(Opcodes.ALOAD, 1);
308+
m.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "B", "getC",
309+
"()Ljava/lang/String;", false);
310+
m.visitVarInsn(Opcodes.ASTORE, 2);
311+
m.visitVarInsn(Opcodes.ALOAD, 2);
312+
m.visitJumpInsn(Opcodes.IFNULL, label);
313+
final AbstractInsnNode ifNullInstruction3 = m.instructions.getLast();
314+
m.visitVarInsn(Opcodes.ALOAD, 2);
315+
final Label next = new Label();
316+
m.visitJumpInsn(Opcodes.GOTO, next);
317+
m.visitLabel(label);
318+
m.visitLdcInsn("");
319+
final AbstractInsnNode nullTarget = m.instructions.getLast();
320+
m.visitLabel(next);
321+
m.visitInsn(Opcodes.ARETURN);
322+
323+
filter.filter(m, context, output);
324+
325+
assertIgnored(m);
326+
final HashMap<AbstractInsnNode, List<Replacement>> replacements = new HashMap<AbstractInsnNode, List<Replacement>>();
327+
replacements.put(ifNullInstruction1, Arrays.asList( //
328+
new Replacement(0, ifNullInstruction1, 0),
329+
new Replacement(1, nullTarget, 0)));
330+
replacements.put(ifNullInstruction2, Arrays.asList( //
331+
new Replacement(0, ifNullInstruction2, 0),
332+
new Replacement(1, nullTarget, 0)));
333+
replacements.put(ifNullInstruction3, Arrays.asList( //
334+
new Replacement(0, ifNullInstruction3, 0),
335+
new Replacement(1, nullTarget, 0)));
336+
assertReplacedBranches(m, replacements);
337+
}
338+
236339
}

‎org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/KotlinSafeCallOperatorFilter.java‎

Lines changed: 74 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,31 @@ public void filter(final MethodNode methodNode,
9090
* label:
9191
* ACONST_NULL
9292
* </pre>
93+
*
94+
* "unoptimized" safe call operator(s) followed by elvis operator:
95+
*
96+
* <pre>
97+
* ALOAD v0
98+
* IFNULL nullCase
99+
* ... // call 0
100+
*
101+
* ASTORE v1
102+
* ALOAD v1
103+
* IFNULL nullCase
104+
* ... // call 1
105+
*
106+
* ...
107+
*
108+
* ASTORE vN
109+
* ALOAD vN
110+
* IFNULL nullCase
111+
* ALOAD vN
112+
* GOTO nonNullCase
113+
* nullCase:
114+
* ... // right hand side of elvis operator
115+
* nonNullCase:
116+
* ...
117+
* </pre>
93118
*/
94119
private static Collection<ArrayList<JumpInsnNode>> findChains(
95120
final MethodNode methodNode) {
@@ -108,18 +133,19 @@ private static Collection<ArrayList<JumpInsnNode>> findChains(
108133
continue;
109134
}
110135
} else if (target.getOpcode() == Opcodes.ACONST_NULL) {
111-
final AbstractInsnNode p1 = preceding(i);
112-
if (p1.getOpcode() != Opcodes.ALOAD) {
136+
final AbstractInsnNode p1 = preceding(i, Opcodes.ALOAD);
137+
if (p1 == null) {
113138
continue;
114139
}
115140
if (chain != null) {
116-
final AbstractInsnNode p2 = preceding(p1);
117-
if (p2 == null || p2.getOpcode() != Opcodes.ASTORE
141+
final AbstractInsnNode p2 = preceding(p1, Opcodes.ASTORE);
142+
if (p2 == null
118143
|| ((VarInsnNode) p1).var != ((VarInsnNode) p2).var) {
119144
continue;
120145
}
121146
}
122-
} else {
147+
} else if (!isUnoptimizedSafeCallFollowedByElvis(jump, target,
148+
chain)) {
123149
continue;
124150
}
125151
if (chain == null) {
@@ -131,19 +157,57 @@ private static Collection<ArrayList<JumpInsnNode>> findChains(
131157
return chains.values();
132158
}
133159

160+
private static boolean isUnoptimizedSafeCallFollowedByElvis(
161+
final JumpInsnNode jump, final AbstractInsnNode target,
162+
final ArrayList<JumpInsnNode> chain) {
163+
if (target.getType() == AbstractInsnNode.JUMP_INSN
164+
|| target.getType() == AbstractInsnNode.TABLESWITCH_INSN
165+
|| target.getType() == AbstractInsnNode.LOOKUPSWITCH_INSN) {
166+
return false;
167+
}
168+
final AbstractInsnNode p1 = preceding(jump, Opcodes.ALOAD);
169+
if (p1 == null) {
170+
return false;
171+
} else if (chain == null) {
172+
final AbstractInsnNode gotoInstruction = preceding(jump.label,
173+
Opcodes.GOTO);
174+
final AbstractInsnNode loadInstruction1 = preceding(gotoInstruction,
175+
Opcodes.ALOAD);
176+
final AbstractInsnNode ifNullInstruction = preceding(
177+
loadInstruction1, Opcodes.IFNULL);
178+
final AbstractInsnNode loadInstruction2 = preceding(
179+
ifNullInstruction, Opcodes.ALOAD);
180+
final AbstractInsnNode storeInstruction = preceding(
181+
loadInstruction2, Opcodes.ASTORE);
182+
return storeInstruction != null
183+
&& ((JumpInsnNode) ifNullInstruction).label == jump.label
184+
&& ((VarInsnNode) loadInstruction1).var == ((VarInsnNode) loadInstruction2).var
185+
&& ((VarInsnNode) loadInstruction1).var == ((VarInsnNode) storeInstruction).var;
186+
} else {
187+
final AbstractInsnNode p2 = preceding(p1, Opcodes.ASTORE);
188+
return p2 != null
189+
&& ((VarInsnNode) p1).var == ((VarInsnNode) p2).var;
190+
}
191+
}
192+
134193
/**
135-
* @return non pseudo-instruction preceding given
194+
* @return non pseudo-instruction preceding given if it has given opcode,
195+
* {@code null} otherwise
136196
*/
137-
private static AbstractInsnNode preceding(AbstractInsnNode i) {
197+
private static AbstractInsnNode preceding(AbstractInsnNode i,
198+
final int opcode) {
138199
if (i == null) {
139200
return null;
140201
}
141202
do {
142203
i = i.getPrevious();
143-
} while (i != null && (i.getType() == AbstractInsnNode.LABEL
204+
if (i == null) {
205+
return null;
206+
}
207+
} while (i.getType() == AbstractInsnNode.LABEL
144208
|| i.getType() == AbstractInsnNode.LINE
145-
|| i.getType() == AbstractInsnNode.FRAME));
146-
return i;
209+
|| i.getType() == AbstractInsnNode.FRAME);
210+
return i.getOpcode() == opcode ? i : null;
147211
}
148212

149213
}

‎org.jacoco.doc/docroot/doc/changes.html‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,8 @@ <h3>New Features</h3>
3131
(GitHub <a href="https://github.com/jacoco/jacoco/issues/1655">#1655</a>).</li>
3232
<li>Part of bytecode generated by the Kotlin compiler for elvis operator that
3333
follows safe call operator is filtered out during generation of report
34-
(GitHub <a href="https://github.com/jacoco/jacoco/issues/1814">#1814</a>).</li>
34+
(GitHub <a href="https://github.com/jacoco/jacoco/issues/1814">#1814</a>,
35+
<a href="https://github.com/jacoco/jacoco/issues/1954">#1954</a>).</li>
3536
<li>Part of bytecode generated by the Kotlin compiler for more cases of chained
3637
safe call operators is filtered out during generation of report
3738
(GitHub <a href="https://github.com/jacoco/jacoco/issues/1956">#1956</a>).</li>

0 commit comments

Comments
 (0)