Skip to content

Commit

Permalink
fix(matching_handlers): Incorrect calculation of matching score for m…
Browse files Browse the repository at this point in the history 
…ethod declarations (#45)

An issue arose when calculating matching scores with method signatures
that had more subtle type arguments. In the failing test before this
change, the final keyword caused the matching score calculator to break
because the naive implementation was fetching the first child of the
formal_parameter node. The implementation has been altered so that all
children are considered except for the identifier one, which should not
be taken into account anyway.
  • Loading branch information
@jpedroh
jpedroh authored Mar 12, 2024
1 parent 69d27fd commit 9151173
Show file tree
Hide file tree
Showing 5 changed files with 200 additions and 3 deletions.
31 changes: 31 additions & 0 deletions bin/tests/scenarios/fancy_argument_types_matching_issue/base.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package de.fosd.jdime.common;

import AST.*;

public class ASTNodeArtifact extends Artifact<ASTNodeArtifact> {
private ASTNodeArtifact(final ASTNode<?> astnode) {
assert (astnode != null);
this.astnode = astnode;

this.initializeChildren();
}

public ASTNodeArtifact(final FileArtifact artifact) {
assert (artifact != null);

setRevision(artifact.getRevision());

ASTNode<?> astnode;
if (artifact.isEmpty()) {
astnode = new ASTNode<>();
} else {
Program p = initProgram();
p.addSourceFile(artifact.getPath());
astnode = p;
}

this.astnode = astnode;
this.initializeChildren();
renumberTree();
}
}
32 changes: 32 additions & 0 deletions bin/tests/scenarios/fancy_argument_types_matching_issue/left.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package de.fosd.jdime.common;

import AST.*;
import de.fosd.jdime.common.operations.ConflictOperation;

public class ASTNodeArtifact extends Artifact<ASTNodeArtifact> {
private ASTNodeArtifact(final ASTNode<?> astnode) {
assert (astnode != null);
this.astnode = astnode;

this.initializeChildren();
}

public ASTNodeArtifact(final FileArtifact artifact) {
assert (artifact != null);

setRevision(artifact.getRevision());

ASTNode<?> astnode;
if (artifact.isEmpty()) {
astnode = new ASTNode<>();
} else {
Program p = initProgram();
p.addSourceFile(artifact.getPath());
astnode = p;
}

this.astnode = astnode;
this.initializeChildren();
renumberTree();
}
}
1 change: 1 addition & 0 deletions bin/tests/scenarios/fancy_argument_types_matching_issue/merge.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
package de . fosd . jdime . common ; import de . fosd . jdime . common . operations . AddOperation ; import AST . * ; import de . fosd . jdime . common . operations . ConflictOperation ; public class ASTNodeArtifact extends Artifact < ASTNodeArtifact > { private ASTNodeArtifact ( final ASTNode < ? > astnode ) { assert ( astnode != null ) ; this . astnode = astnode ; this . initializeChildren ( ) ; } public ASTNodeArtifact ( final FileArtifact artifact ) { assert ( artifact != null ) ; setRevision ( artifact . getRevision ( ) ) ; ASTNode < ? > astnode ; if ( artifact . isEmpty ( ) ) { astnode = new ASTNode < > ( ) ; } else { Program p = initProgram ( ) ; p . addSourceFile ( artifact . getPath ( ) ) ; astnode = p ; } this . astnode = astnode ; this . initializeChildren ( ) ; renumberTree ( ) ; } }
32 changes: 32 additions & 0 deletions bin/tests/scenarios/fancy_argument_types_matching_issue/right.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package de.fosd.jdime.common;

import de.fosd.jdime.common.operations.AddOperation;
import AST.*;

public class ASTNodeArtifact extends Artifact<ASTNodeArtifact> {
private ASTNodeArtifact(final ASTNode<?> astnode) {
assert (astnode != null);
this.astnode = astnode;

this.initializeChildren();
}

public ASTNodeArtifact(final FileArtifact artifact) {
assert (artifact != null);

setRevision(artifact.getRevision());

ASTNode<?> astnode;
if (artifact.isEmpty()) {
astnode = new ASTNode<>();
} else {
Program p = initProgram();
p.addSourceFile(artifact.getPath());
astnode = p;
}

this.astnode = astnode;
this.initializeChildren();
renumberTree();
}
}
107 changes: 104 additions & 3 deletions matching_handlers/src/java/method_declaration.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,15 @@ fn extract_argument_types_from_formal_parameters(node: &CSTNode) -> Vec<String>
.filter(|inner_node| inner_node.kind() == "formal_parameter")
.filter_map(|inner_node| match inner_node {
CSTNode::Terminal(_) => None,
CSTNode::NonTerminal(non_terminal) => {
non_terminal.children.first().map(|v| v.contents())
}
CSTNode::NonTerminal(non_terminal) => Some(
non_terminal
.children
.iter()
.filter(|node| node.kind() != "identifier")
.fold(String::new(), |acc, cur| {
format!("{} {}", acc, cur.contents())
}),
),
})
.collect(),
}
Expand Down Expand Up @@ -112,6 +118,101 @@ mod tests {
assert_eq!(0, matching_score);
}

#[test]
fn for_matching_formal_parameters_it_takes_into_consideration_all_children_except_identifier() {
let node_a = make_method_declaration_node(
"ASTNodeArtifact",
CSTNode::NonTerminal(NonTerminal {
kind: "formal_parameter",
children: vec![
CSTNode::NonTerminal(NonTerminal {
kind: "modifiers",
children: vec![CSTNode::Terminal(Terminal {
kind: "final",
value: "final",
..Default::default()
})],
..Default::default()
}),
CSTNode::NonTerminal(NonTerminal {
kind: "generic_type",
children: vec![
CSTNode::Terminal(Terminal {
kind: "type_identifier",
value: "ASTNode",
..Default::default()
}),
CSTNode::NonTerminal(NonTerminal {
kind: "type_arguments",
children: vec![
CSTNode::Terminal(Terminal {
kind: "<",
value: "<",
..Default::default()
}),
CSTNode::NonTerminal(NonTerminal {
kind: "wildcard",
children: vec![CSTNode::Terminal(Terminal {
kind: "?",
value: "?",
..Default::default()
})],
..Default::default()
}),
CSTNode::Terminal(Terminal {
kind: ">",
value: ">",
..Default::default()
}),
],
..Default::default()
}),
],
..Default::default()
}),
CSTNode::Terminal(Terminal {
kind: "identifier",
value: "astnode",
..Default::default()
}),
],
..Default::default()
}),
);

let node_b = make_method_declaration_node(
"ASTNodeArtifact",
CSTNode::NonTerminal(NonTerminal {
kind: "formal_parameter",
children: vec![
CSTNode::NonTerminal(NonTerminal {
kind: "modifiers",
children: vec![CSTNode::Terminal(Terminal {
kind: "final",
value: "final",
..Default::default()
})],
..Default::default()
}),
CSTNode::Terminal(Terminal {
kind: "type_identifier",
value: "FileArtifact",
..Default::default()
}),
CSTNode::Terminal(Terminal {
kind: "identifier",
value: "astnode",
..Default::default()
}),
],
..Default::default()
}),
);

let result = compute_matching_score_for_method_declaration(&node_a, &node_b);
assert_eq!(0, result);
}

fn make_method_declaration_node<'a>(
identifier: &'a str,
parameter: CSTNode<'a>,
Expand Down

0 comments on commit 9151173

Please sign in to comment.