Skip to content

Commit 9f2c50d

Browse files
committed
fix: Configure falsely removed ELSE and ELIF nodes
This bug was reported by @piameier.
1 parent a51ad75 commit 9f2c50d

1 file changed

Lines changed: 60 additions & 9 deletions

File tree

  • src/main/java/org/variantsync/diffdetective/variation/tree/view/relevance

src/main/java/org/variantsync/diffdetective/variation/tree/view/relevance/Configure.java

Lines changed: 60 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,17 @@
11
package org.variantsync.diffdetective.variation.tree.view.relevance;
22

3+
import java.util.List;
4+
import java.util.Map;
5+
import java.util.Map.Entry;
6+
import java.util.function.Consumer;
7+
38
import org.prop4j.Node;
49
import org.prop4j.NodeWriter;
10+
import org.variantsync.diffdetective.util.Assert;
511
import org.variantsync.diffdetective.util.fide.FixTrueFalse;
612
import org.variantsync.diffdetective.util.fide.FixTrueFalse.Formula;
13+
import org.variantsync.diffdetective.variation.NodeType;
714
import org.variantsync.diffdetective.variation.tree.VariationNode;
8-
9-
import java.util.Map;
10-
import java.util.Map.Entry;
11-
import java.util.function.Consumer;
1215
import org.variantsync.diffdetective.variation.tree.view.relevance.spec.ConfigureSpec;
1316

1417
/**
@@ -75,18 +78,66 @@ public Configure(final Map<String, Boolean> assignment) {
7578
public boolean test(VariationNode<?, ?> v) {
7679
return ConfigureSpec.test(configuration, v);
7780
}
81+
82+
private <TreeNode extends VariationNode<TreeNode, ?>> void computeViewNodes(List<TreeNode> vs, Consumer<TreeNode> markRelevant) {
83+
for (final TreeNode c : vs) {
84+
computeViewNodes(c, markRelevant);
85+
}
7886
}
7987

8088
@Override
8189
public <TreeNode extends VariationNode<TreeNode, ?>> void computeViewNodes(TreeNode v, Consumer<TreeNode> markRelevant) {
82-
markRelevant.accept(v);
90+
// The implementation of this method must be semantically equivalent to the default implementation Relevance.super.computeViewNodes(v, markRelevant);
91+
// The implementation of this method is supposed to be optimized to avoid redundant SAT calls when possible.
92+
// This requirement is modelled explicitly in terms of the ConfigureSpec class.
8393

84-
for (final TreeNode c : v.getChildren()) {
85-
// If the child is an artifact it has the same presence condition as we do, so it is also included in the view.
86-
if (c.isArtifact() || test(c)) {
87-
computeViewNodes(c, markRelevant);
94+
// If the child is an artifact, it has the same presence condition as v does, so it is also included in the view.
95+
// The root must be in any view to ensure consistency.
96+
if (v.isArtifact() || v.isRoot()) {
97+
markRelevant.accept(v);
98+
computeViewNodes(v.getChildren(), markRelevant);
99+
} else {
100+
computeViewNodesOfElifChain(v, markRelevant);
101+
}
102+
}
103+
104+
/**
105+
* @return true if the given node was marked relevant
106+
*/
107+
private <TreeNode extends VariationNode<TreeNode, ?>> boolean computeViewNodesOfElifChain(TreeNode v, Consumer<TreeNode> markRelevant) {
108+
final NodeType vt = v.getNodeType();
109+
Assert.assertTrue(vt == NodeType.IF || vt == NodeType.ELSE || vt == NodeType.ELIF);
110+
111+
if (test(v)) {
112+
markRelevant.accept(v);
113+
computeViewNodes(v.getChildren(), markRelevant);
114+
return true;
115+
} else {
116+
// If a partial configuration excludes the node v (i.e, test(v) == false),
117+
// then any ELIF or ELSE branches might still be included.
118+
for (final TreeNode c : v.getChildren()) {
119+
NodeType ct = c.getNodeType();
120+
121+
// We can skip all children that are not ELSE or ELIF nodes because these are excluded because v is exluded.
122+
// If our node v was deemed irrelevant and it has an ELSE node c, that ELSE node c must be relevant.
123+
if (ct == NodeType.ELSE) {
124+
markRelevant.accept(v);
125+
markRelevant.accept(c);
126+
computeViewNodes(c.getChildren(), markRelevant);
127+
return true;
128+
}
129+
130+
// An ELIF might hold any formula which we have to test.
131+
// We handle ELIF nodes recursively because ELIFs might be nested.
132+
// If at least one branch of the ELIF chain is included, we must include also v in the view to retain consistency of the tree.
133+
if (ct == NodeType.ELIF && computeViewNodesOfElifChain(c, markRelevant)) {
134+
markRelevant.accept(v);
135+
return true;
136+
}
88137
}
89138
}
139+
140+
return false;
90141
}
91142

92143
@Override

0 commit comments

Comments
 (0)