Skip to content

Commit

Permalink
fix(matching): Early return on matching if nodes have different kinds (
Browse files Browse the repository at this point in the history
…#35)

This is a bug that was discovered thanks to running the tool in jdime
repository. Basically, the matching algorithm was running even if the
nodes have different kinds, which was introducing incorrect behavior in
the merge step: leading into trying merging two nodes that have
different kinds, which is logically inconsistent.
  • Loading branch information
jpedroh authored Mar 1, 2024
1 parent be11ade commit 38ad55b
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 0 deletions.
21 changes: 21 additions & 0 deletions bin/tests/scenarios/jdime_matching_issue/base.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package de.fosd.jdime.artifact;

import java.security.MessageDigest;

public abstract class Artifact<T extends Artifact<T>> implements Comparable<T>, StatisticsInterface {
public boolean hasChanges(Revision revision) {
if (this.revision.equals(revision)) {
return false;
}

if (!hasMatching(revision)) {
return true;
}

T match = getMatching(revision).getMatchingArtifact(this);

return getTreeSize() != match.getTreeSize() || Artifacts.bfsStream(self()).anyMatch(a -> {
return !a.hasMatching(revision) || !a.getMatching(revision).getMatchingArtifact(a).hashId().equals(a.hashId());
});
}
}
22 changes: 22 additions & 0 deletions bin/tests/scenarios/jdime_matching_issue/left.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package de.fosd.jdime.artifact;

import java.security.MessageDigest;

public abstract class Artifact<T extends Artifact<T>> implements Comparable<T>, StatisticsInterface {
public boolean hasChanges(Revision revision) {

if (this.revision.equals(revision)) {
return false;
}

if (!hasMatching(revision)) {
return true;
}

T match = getMatching(revision).getMatchingArtifact(this);

return getTreeSize() != match.getTreeSize() || Artifacts.bfsStream(self()).anyMatch(a -> {
return !a.hasMatching(revision) || !a.getMatching(revision).getMatchingArtifact(a).hashId().equals(a.hashId());
});
}
}
1 change: 1 addition & 0 deletions bin/tests/scenarios/jdime_matching_issue/merge.java
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
package de . fosd . jdime . artifact ; import java . security . MessageDigest ; public abstract class Artifact < T extends Artifact < T > > implements Comparable < T > , StatisticsInterface { public boolean hasChanges ( Revision revision ) { if ( this . revision . equals ( revision ) ) { return false ; } if ( ! hasMatching ( revision ) ) { return true ; } T match = getMatching ( revision ) . getMatchingArtifact ( this ) ; return getTreeSize ( ) != match . getTreeSize ( ) || ! getTreeHash ( ) . equals ( match . getTreeHash ( ) ) ; } }
20 changes: 20 additions & 0 deletions bin/tests/scenarios/jdime_matching_issue/right.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package de.fosd.jdime.artifact;

import java.security.MessageDigest;

public abstract class Artifact<T extends Artifact<T>> implements Comparable<T>, StatisticsInterface {
public boolean hasChanges(Revision revision) {

if (this.revision.equals(revision)) {
return false;
}

if (!hasMatching(revision)) {
return true;
}

T match = getMatching(revision).getMatchingArtifact(this);

return getTreeSize() != match.getTreeSize() || !getTreeHash().equals(match.getTreeHash());
}
}
8 changes: 8 additions & 0 deletions matching/src/ordered_tree_matching.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,14 @@ pub fn ordered_tree_matching<'a>(
) => {
let root_matching: usize = (kind_left == kind_right).into();

// Node kinds do not match, so we return a matching with a score of 0
if root_matching == 0 {
return Matchings::from_single(
UnorderedPair(left, right),
MatchingEntry::new(0, false),
);
}

let m = children_left.len();
let n = children_right.len();

Expand Down

0 comments on commit 38ad55b

Please sign in to comment.