Skip to content

Fix a crucial bug in the Configure relevance predicate - #174

Merged
pmbittner merged 30 commits into
developfrom
fix-configure
Nov 5, 2025
Merged

Fix a crucial bug in the Configure relevance predicate#174
pmbittner merged 30 commits into
developfrom
fix-configure

Conversation

@pmbittner

@pmbittnerpmbittner commented Oct 13, 2025•
edited
Loading

Copy link
Copy Markdown
Member

This PR fixes a bug reported by @piameier.

The Configure relevance predicate for generating views falsely removed ELSE and ELIF branches. The reason was that the optimized implementation to mark nodes as relevant discarded an entire subtree as soon as that subtree's root was irrelevant (i.e., that root conflicted a given partial configuration). However, in such a case, we still have to check and test any ELSE or ELIF branches.

I implemented the first ever unit tests for views on trees and diffs to check that the bug is now fixed. The new test cases checks the implementation of Configure against a naive implementation called ConfigureSpec.

In the future, it might be great to

  • add more test cases
  • add specifications for other relevance predicates as well

This PR also contributes the new EliminateEmptyAlternatives transformer and required refactorings.

@pmbittner
pmbittner requested a review from ibbemOctober 13, 2025 15:46
@pmbittnerpmbittner self-assigned this Oct 13, 2025
@pmbittnerpmbittner added the bug Something isn't working label Oct 13, 2025
@pmbittner
pmbittner changed the base branch from main to developOctober 13, 2025 15:46
@pmbittner
refactor: extract Transformer super interface ...
8c63c20
... from VariationDiffTransformer
@pmbittner
VariationTreeTransformer interface
9dfbfb2
@pmbittner
VariationTreeNode::setFormula
fa62e30
@pmbittner
feat: traversing variation trees in post-order
6b2bcb0
@pmbittner
feat: DiffNode::stealChildrenOf at time
a48b4fd
@pmbittner
fix: VariationNode::stealChildrenOf
fd0d96e
Previously, this method always crashed because addChildren assumes the
given children to have no parent. Yet, the children always had 'other'
as parent because we only removed them afterwards.
@pmbittner
feat: EliminateEmptyAlternatives transformer
1939f50
@pmbittner
fix: javadoc error in Transformer.java
7f70843
@pmbittner
fix: javadoc error in EliminateEmptyAlternatives
db2fccf
@pmbittner
fix: VariationNode::stealChildrenOf once again
7daeff0
We have to create a copy of the children list because it is cleared by
some implementations of removeAllChildren(). So effectively, no children
were added via addChildren.
ibbem
ibbem approved these changes Oct 24, 2025
View reviewed changes

@ibbemibbem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good work. I did not find any real issues.

Unfortunate that there are commits with failing CI in between. However (as discussed in a meeting), changing that would break downstream work that is already based on this. Hence, we can keep it like that.

Kind of unrelated: Do we check anywhere that a DiffNode has at most one ELIF/ELSE? I just checked and didn't find anything. I have a change ready but am kind of surprised that we do not check this yet.

Comment thread...in/java/org/variantsync/diffdetective/variation/diff/transform/VariationTreeTransformer.java Outdated
Comment threadsrc/main/java/org/variantsync/diffdetective/variation/tree/view/relevance/Configure.java Outdated
@pmbittner@ibbem
Simplify Assertion in Configure
2c6d651
Co-authored-by: ibbem <benjamin.moosherr@uni-ulm.de>
@pmbittner

pmbittner commented Oct 26, 2025

Copy link
Copy Markdown
MemberAuthor

Thank you for your review. I do not remember whether we check the ELSE/ELIF validity anywhere. If you have a fix, I am happy to take it. I guess that DiffNode::assertConsistency would be a good place? I was also a bit surprised seeing that we do not have VariationNode::assertConsistency.

@pmbittner
remove boilerplate Variation(Tree|Diff)Transformer
8197880
These where interfaces without a real purpose.
@ibbem

ibbem commented Oct 26, 2025

Copy link
Copy Markdown
Collaborator

If you have a fix, I am happy to take it.

I'll open a separate PR.

I was also a bit surprised seeing that we do not have VariationNode::assertConsistency.

We do have VariationNode::assertConsistency

@ibbemibbem mentioned this pull request Oct 26, 2025
Merged
@pmbittnerpmbittner mentioned this pull request Nov 3, 2025
Merged
2 tasks
@ibbem
ibbem self-requested a review November 4, 2025 08:56
@pmbittnerpmbittner added enhancement New feature or request Refactoring labels Nov 5, 2025
@pmbittner
pmbittner merged commit 2ee3a19 into developNov 5, 2025
2 checks passed
@pmbittner
pmbittner deleted the fix-configure branch April 14, 2026 05:47
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ibbemibbemibbem approved these changes

Assignees

@pmbittnerpmbittner

Labels

bugSomething isn't workingenhancementNew feature or requestRefactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@pmbittner@ibbem