Skip to content

Commit 9dcbcc9

Browse files
authored
Bug fix in parent scope rules (#778)
**Summary:** There is a bug in piranha core that prevents parent rules from being match only. Parent scope is forcibly applying edits without checking whether the rule is match only. This PR fixes that **Test plan:** I added a test that fails without the fix to illustrate the problem, as well as an E2E test
1 parent 43abbb4 commit 9dcbcc9

File tree

3 files changed

+154
-4
lines changed

3 files changed

+154
-4
lines changed

crates/core/src/models/edit.rs

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -139,17 +139,23 @@ impl SourceCodeUnit {
139139
// we apply the rules in the order they are provided to each ancestor in the context
140140
for rule in &next_rules[PARENT] {
141141
for ancestor in &context() {
142-
if let Some(edit) = self.get_edit(rule, rules_store, *ancestor, false) {
143-
return Some(edit);
142+
// Skip match-only rules - they should not create edits for parent scope
143+
if !rule.rule().is_match_only_rule() {
144+
if let Some(edit) = self.get_edit(rule, rules_store, *ancestor, false) {
145+
return Some(edit);
146+
}
144147
}
145148
}
146149
}
147150

148151
// we apply the rules to each ancestor in the context in the order they are provided
149152
for ancestor in &context() {
150153
for rule in &next_rules[PARENT_ITERATIVE] {
151-
if let Some(edit) = self.get_edit(rule, rules_store, *ancestor, false) {
152-
return Some(edit);
154+
// Skip match-only rules - they should not create edits for parent scope
155+
if !rule.rule().is_match_only_rule() {
156+
if let Some(edit) = self.get_edit(rule, rules_store, *ancestor, false) {
157+
return Some(edit);
158+
}
153159
}
154160
}
155161
}

crates/core/src/models/unit_tests/rule_test.rs

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,67 @@ fn test_satisfies_filter_not_enclosing_node_negative() {
384384
);
385385
}
386386

387+
/// Test that match-only parent rules do not create edits (regression test for bug fix)
388+
#[test]
389+
fn test_match_only_parent_rule_no_edit() {
390+
// Create a match-only rule (no replace_node specified)
391+
let _match_only_rule = piranha_rule! {
392+
name = "match_only_parent",
393+
query = "(
394+
(binary_expression
395+
left : (_)* @lhs
396+
operator:\"&&\"
397+
right: [(true) (parenthesized_expression (true))]
398+
)
399+
@binary_expression)"
400+
// Note: no replace_node specified, making this a match-only rule
401+
};
402+
let match_only_rule = InstantiatedRule::new(&_match_only_rule, &HashMap::new());
403+
404+
// Verify it's actually a match-only rule
405+
assert!(match_only_rule.rule().is_match_only_rule());
406+
407+
let source_code = "class A {
408+
boolean f = something && true;
409+
}";
410+
411+
let mut rule_store = RuleStore::default();
412+
let args = PiranhaArgumentsBuilder::default()
413+
.paths_to_codebase(vec![UNUSED_CODE_PATH.to_string()])
414+
.language(PiranhaLanguage::from(JAVA))
415+
.build();
416+
let mut parser = args.language().parser();
417+
418+
let source_code_unit = SourceCodeUnit::new(
419+
&mut parser,
420+
source_code.to_string(),
421+
&HashMap::new(),
422+
PathBuf::new().as_path(),
423+
&args,
424+
);
425+
426+
// Range pointing to part of the binary expression that would trigger parent rule matching
427+
let range = Range {
428+
start_byte: 41_usize,
429+
end_byte: 44_usize,
430+
..Default::default()
431+
};
432+
433+
// Set up match-only rule as a parent rule
434+
let next_rules = HashMap::from([
435+
(String::from(PARENT), vec![match_only_rule.clone()]),
436+
(String::from(PARENT_ITERATIVE), vec![match_only_rule]),
437+
]);
438+
439+
// Before the fix: this would return Some(edit) that deletes content
440+
// After the fix: this should return None because match-only rules shouldn't create edits
441+
let edit = source_code_unit.get_edit_for_ancestors(&range, &mut rule_store, &next_rules);
442+
assert!(
443+
edit.is_none(),
444+
"Match-only parent rules should not create edits"
445+
);
446+
}
447+
387448
/// Tests whether comments are preserved when delete_comments is set to false
388449
#[test]
389450
fn test_rule_delete_comments() {

tests/test_piranha.py

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -378,3 +378,86 @@ def test_kotlin_boolean_simplification():
378378
}
379379
return true
380380
}"""
381+
382+
383+
def test_match_only_parent_rule_bug_fix():
384+
"""
385+
Regression test for bug where match-only parent rules were incorrectly deleting code.
386+
387+
Before the fix: match-only parent rules would create deletion edits
388+
After the fix: match-only parent rules should only match without transforming code
389+
"""
390+
code = """
391+
if (obj.isLocEnabled() || x > 0) {
392+
// do something
393+
x = obj.isLocEnabled();
394+
if (other_check) { }
395+
} else {
396+
// do something else!
397+
}
398+
"""
399+
400+
# Seed rule that replaces method calls with true
401+
seed_rule = Rule(
402+
name="replace_method",
403+
query="cs :[x].isLocEnabled()",
404+
replace_node="*",
405+
replace="true",
406+
is_seed_rule=True
407+
)
408+
409+
# Match-only parent rule - should NOT delete the assignment
410+
# This rule has no replace_node, making it match-only
411+
match_only_parent_rule = Rule(
412+
name="find_assignment_parent",
413+
query="cs :[x] = true;"
414+
# Note: no replace_node specified = match-only rule
415+
)
416+
417+
# This rule should only be triggered after the match-only parent rule
418+
logic_triggered_only_after_match_only_parent_rule = Rule(
419+
name="logic_triggered_only_after_match_only_parent_rule",
420+
query='cs if (other_check) { }',
421+
replace_node="*",
422+
)
423+
424+
425+
# Parent edge: after replacing method call, look for assignments in parent scope
426+
parent_edge = OutgoingEdges(
427+
"replace_method",
428+
["find_assignment_parent"],
429+
"Parent"
430+
)
431+
file_edge = OutgoingEdges(
432+
"find_assignment_parent",
433+
["logic_triggered_only_after_match_only_parent_rule"],
434+
"File"
435+
)
436+
437+
rule_graph = RuleGraph(
438+
rules=[seed_rule, match_only_parent_rule, logic_triggered_only_after_match_only_parent_rule],
439+
edges=[parent_edge, file_edge]
440+
)
441+
442+
args = PiranhaArguments(
443+
code_snippet=code,
444+
language="java",
445+
rule_graph=rule_graph
446+
)
447+
448+
output_summaries = execute_piranha(args)
449+
assert len(output_summaries) == 1
450+
451+
result_code = output_summaries[0].content
452+
453+
# The seed rule should replace isLocEnabled() with true
454+
assert "obj.isLocEnabled()" not in result_code
455+
assert "x = true;" in result_code
456+
457+
# CRITICAL: The match-only parent rule should NOT delete the assignment
458+
# Before the bug fix, this line would be deleted
459+
# After the bug fix, this line should remain
460+
assert "x = true;" in result_code, "Match-only parent rule incorrectly deleted the assignment"
461+
462+
# The if condition should also be deleted
463+
assert "if (other_check) { }" not in result_code

0 commit comments

Comments
 (0)