Improved Unparser #169
Merged
Improved Unparser #169
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Note that some printed checks failed expectedly (those that didn't ignore whitespace or didn't compare diffs semantically) and thus are removed without replacement.
This kind of includes a breaking change: Keeping trailing empty lines and interpreting them as unchanged trailing empty lines. Although multiple empty lines should never occur in well formatted diffs (because each line contains at least the diff symbol), this is likely not intended and thus a bug. Note that preventing a new line in the special case of an empty output seems unnecessary but was explicitly done in the old version so the refactored version does this as well.
Given a `BadVDiff` with at least one node, `Show.baddiff` uses `VariationTree.toVariationDiff` which previously required all created nodes to have `DiffType.NON`. However, `Show.baddiff` supplies nodes with `DiffType.ADD` and `DiffType.REM` which triggered an assert in `DiffNode.addChild` during the `VariationTree.toVariationDiff` construction process.
It seems like `VariationDiffParser` never generates a variation diff which triggers the bug in `BadVDiff` that doesn't preserve the child order. Note that it should be possible to generate such variation diffs with a tree matcher such as GumTree.
This is necessary because when `endif`s are fully integrated (included in deep copies, assertConsistency and comparisons) some assumptions are broken (which fields are compare by `DiffNode`s) which is especially annoying in `BadVDiff`. By storing the `endif`s in the label instead of the nodes directly, no user needs to be updated except if the label is actually modified (there seems to be no instance of that in this code base). This also generalized the concept to trailing lines instead of limiting the concept to endifs. This is also kind of necessary because not all labels know the node type. Depending on the user of `VariationDiffParser`, this might be a breaking change because we now consider the `#endif` line as part of the label. Hence, if the `#endif` line changed (e.g., a comment is added) the node can no longer be `DiffType.NON` but we instead split the node into two `DiffType.REM` and `DiffType.ADD` nodes. Artifacts which are below such a split node are thus classified as refactored instead of unchanged.
pmbittner
approved these changes
Aug 26, 2025
src/main/java/org/variantsync/diffdetective/variation/VariationUnparser.java
Outdated
Show resolved
Hide resolved
src/main/java/org/variantsync/diffdetective/variation/diff/DiffNode.java
Outdated
Show resolved
Hide resolved
src/main/java/org/variantsync/diffdetective/variation/diff/DiffNode.java
Show resolved
Hide resolved
src/main/java/org/variantsync/diffdetective/variation/diff/DiffNode.java
Outdated
Show resolved
Hide resolved
src/main/java/org/variantsync/diffdetective/variation/diff/DiffType.java
Show resolved
Hide resolved
pmbittner
added
enhancement
Closed
This makes the assumption of single character `DiffSymbol`s explicit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a rebase of #152 with some additional fixes and refactorings.
During the rebase I applied the following changes on each commit:
eshul <eugen-shulimov@web.de>instead ofDARIA-ACER\eshul <shulimov@mail.uni-paderborn.de>which was only used in the beginning)Except two commits, which became empty, all commits where kept.
The following was addressed after the rebase in separate commits:
endifparsing (a8065d9)Note that all tests cases, even the ones that where removed during the rebase, pass the test suit after this fix.
VariationUnparser(fbd6f3c and 68d8131)Show.baddiffwhich I discovered while debugging the next item in this list (cbf504b)BadVDiffwhich I discovered after implementing the next item in this list (c860a88)endifs in the label instead of the nodes directly (de10f75)Notably, I didn't touch the experiment except changing the package name and formatting the code.
There are two things that I'm still unsure about:
ignoreEmptyLines = true?I don't know how relevant this was to the goal of the thesis. Just judging from the code, it doesn't make sense to test. We could probably get rid of all the
removeWhitespacebusiness and make the tests much tighter forignoreEmptyLines = false.printSourceCodeorunparseTree?It turns out that @pmbittner already implemented a variation tree unparser for the views paper. Currently, I removed the stack based implementation because the recursive implementation is more intuitive to me. However, should we keep both interfaces or remove one of the two methods?