Skip to content

Conversation

Godin
Copy link
Member

@Godin Godin commented Dec 12, 2024

This is required for the resolution of

(see explanations in the tickets)


Fixes #1772
Fixes #1773


Also current implementation does not preserve branch indexes in bytecode traversal order - see

if (!b.coveredBranches.isEmpty()) {
result.coveredBranches.set(idx++);

ie for example case when only first replacement target is executed becomes indistinguishable from case when only last replacement target is executed - in both cases resulting instruction will have branch 0 as executed, whereas indexes are important for

I believe that this change is sufficient to cover this need too.

Comment on lines +549 to +551
method.visitLdcInsn("");
// probe[3]
method.visitLabel(l2);
// probe[4]
method.visitInsn(Opcodes.ARETURN);
Copy link
Member Author

Choose a reason for hiding this comment

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

As a side note:

Every time I touch MethodAnalyzerTest I struggle to recall/realize how/where probes are inserted. Maybe it becomes easier after every touch, but still. That's why I started to add comments in the form // probe[id] - they at least help to recall, but unfortunately not all tests have them, and unfortunately they do not appear magically for new tests 😅

IMO it would be nice to have a way to automatically show the places of probes or even better - assert them. Moreover MethodAnalyzerTest is already indirectly testing LabelFlowAnalyzer and MethodProbesAdapter and IMO contains more interesting (and far more) scenarios than their tests.

Set<AbstractInsnNode> newTargets);

/**
* {@link #instruction} and index of its {@link #branch}.
Copy link
Member Author

Choose a reason for hiding this comment

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

It will be nice to add a comment about branch indexes for JumpInsnNode, TableSwitchInsnNode, LookupSwitchInsnNode and others.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 348bfc0

Comment on lines 162 to 185
Instruction i1 = new Instruction(1);
Instruction i2 = new Instruction(2);
Instruction i3 = new Instruction(3);
Copy link
Member Author

@Godin Godin Dec 13, 2024

Choose a reason for hiding this comment

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

At least this violates Instruction contract - see comment about use of Math.max https://github.com/jacoco/jacoco/pull/1813/files#r1883065587

final HashMap<AbstractInsnNode, Collection<InstructionBranch>> replacements = new HashMap<AbstractInsnNode, Collection<InstructionBranch>>();
for (final AbstractInsnNode target : newTargets) {
final ArrayList<InstructionBranch> list = new ArrayList<InstructionBranch>();
final int branches = Math.max(
Copy link
Member Author

@Godin Godin Dec 13, 2024

Choose a reason for hiding this comment

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

As a side note:

Math.max is used here because already existing unit tests

  • InstructionTeest.replaceBranches_should_calculate_coverage_on_new_branches
  • ClassAnalyzerTest.testCalculateFragments
  • ClassAnalyzerTest.should_add_non_empty_methods

violate contract of Instruction that states that after CFG construction each instruction has at least one branch.

Comment on lines 107 to 121
private void applyReplacements() {
for (final Entry<AbstractInsnNode, Set<AbstractInsnNode>> entry : replacements
for (final Entry<AbstractInsnNode, Iterable<Collection<InstructionBranch>>> entry : replacements
.entrySet()) {
final Set<AbstractInsnNode> replacements = entry.getValue();
final List<Instruction> newBranches = new ArrayList<Instruction>(
replacements.size());
for (final AbstractInsnNode b : replacements) {
newBranches.add(instructions.get(b));
final Iterable<Collection<InstructionBranch>> targets = entry
.getValue();
int i = 0;
for (final Collection<InstructionBranch> list : targets) {
i += list.size();
}
final int[] branches = new int[i];
final Instruction[] fromInstructions = new Instruction[i];
final int[] fromBranches = new int[i];

i = 0;
int b = 0;
for (final Collection<InstructionBranch> list : targets) {
for (final InstructionBranch ib : list) {
branches[i] = b;
fromInstructions[i] = instructions.get(ib.instruction);
fromBranches[i] = ib.branch;
i++;
}
b++;
}

final AbstractInsnNode node = entry.getKey();
instructions.put(node,
instructions.get(node).replaceBranches(newBranches));
instructions.put(node, instructions.get(node)
.replaceBranches(branches, fromInstructions, fromBranches));
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I already did not like that the old code was doing allocations merely to pass the data between different interfaces, but the code was small enough, and now it become bigger 😞

IMO allocations can be avoided and code can be simplified by introduction of "mapper" interface similar to java.util.function.Function<AbstractInstructionNode, Instruction>

mapper = (AbstractInsnNode node) -> instructions.get(node); mapper.apply(node).replaceBranches(mapper, targets); 
Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 12778c3

@Godin Godin marked this pull request as ready for review January 15, 2025 07:58
@Godin Godin requested review from leveretka and marchof January 15, 2025 07:59
@Godin
Copy link
Member Author

Godin commented Jan 25, 2025

@marchof @leveretka could you please review?

Copy link
Collaborator

@leveretka leveretka left a comment

Choose a reason for hiding this comment

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

Very impressive work that will unblock many Kotlin features. Thanks for working on that.

So far the core change looks very straightforward (thanks for splitting it into commits was much easier to review).

I will probably need a second look on the updated filters. But so far the change looks nice.

I would probably recommend splitting the PR into several commits. In the first you add a new API for the replaceBranches, and in the other ones you migrate filters. You already have it in several commits, maybe we can also merge it separately (Just an idea)

I still would like to hear @marchof opinion, as you change some internals.

@Godin Godin modified the milestones: 0.8.13, 0.8.14 Apr 1, 2025
@leveretka leveretka self-requested a review April 1, 2025 21:07
Copy link
Collaborator

@leveretka leveretka left a comment

Choose a reason for hiding this comment

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

Since I don't see any concerns mentioned here, I think we can merge it

Copy link
Member

@marchof marchof left a comment

Choose a reason for hiding this comment

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

I have to admit that I wasn't able to wrap my mind around the functionality implemented here. So I left only minor comments regarding the code structure. @Godin please feel free to just ignore them if you don't agree.

@Godin
Copy link
Member Author

Godin commented Apr 8, 2025

I have to admit that I wasn't able to wrap my mind around the functionality implemented here.

@marchof I'm wondering if you read descriptions of #1772 and #1773 ? 😉 IMO they contain description/motivation for the main change here in org.jacoco.core.internal.analysis.Instruction#replaceBranches

 * Creates a copy of this instruction where all outgoing branches are - * replaced with the given instructions. The coverage status of the new - * instruction is derived from the status of the given instructions. + * replaced. The coverage statuses of the branches of the new instruction + * are derived from the statuses of the given branches of the given + * instructions.
@Godin Godin merged commit f823d43 into jacoco:master Apr 11, 2025
35 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Filtering Apr 11, 2025
@Godin Godin deleted the replaceBranches branch April 11, 2025 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment