Skip to content

Commit 8184223

Browse files
authored
Fixing bug after previous fix was not complete (#779)
The previous [bug fix](#778) was partial. It would work for single application of rules, but not until fix point because I forgot to add the edit to the queue. I've modified the test to reflect the correct behavior and fixed the bug.
1 parent d3ee7a3 commit 8184223

File tree

3 files changed

+49
-17
lines changed

3 files changed

+49
-17
lines changed

crates/core/src/models/edit.rs

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -139,23 +139,43 @@ 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-
// 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);
142+
if rule.rule().is_match_only_rule() {
143+
// For match-only rules, get matches and create identity edit if found
144+
let matches = self.get_matches(rule, rules_store, *ancestor, false);
145+
if let Some(first_match) = matches.first() {
146+
// Create identity edit - same range for old and new (no code change)
147+
let identity_edit = Edit::new(
148+
first_match.clone(),
149+
first_match.matched_string().clone(), // Replace with same content = identity
150+
rule.name(),
151+
self.code(),
152+
);
153+
return Some(identity_edit);
146154
}
155+
} else if let Some(edit) = self.get_edit(rule, rules_store, *ancestor, false) {
156+
return Some(edit);
147157
}
148158
}
149159
}
150160

151161
// we apply the rules to each ancestor in the context in the order they are provided
152162
for ancestor in &context() {
153163
for rule in &next_rules[PARENT_ITERATIVE] {
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);
164+
if rule.rule().is_match_only_rule() {
165+
// For match-only rules, get matches and create identity edit if found
166+
let matches = self.get_matches(rule, rules_store, *ancestor, false);
167+
if let Some(first_match) = matches.first() {
168+
// Create identity edit - same range for old and new (no code change)
169+
let identity_edit = Edit::new(
170+
first_match.clone(),
171+
first_match.matched_string().clone(), // Replace with same content = identity
172+
rule.name(),
173+
self.code(),
174+
);
175+
return Some(identity_edit);
158176
}
177+
} else if let Some(edit) = self.get_edit(rule, rules_store, *ancestor, false) {
178+
return Some(edit);
159179
}
160180
}
161181
}

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

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -437,11 +437,19 @@ fn test_match_only_parent_rule_no_edit() {
437437
]);
438438

439439
// 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
440+
// After the fix: this should return Some(identity_edit) that preserves content
441441
let edit = source_code_unit.get_edit_for_ancestors(&range, &mut rule_store, &next_rules);
442442
assert!(
443-
edit.is_none(),
444-
"Match-only parent rules should not create edits"
443+
edit.is_some(),
444+
"Match-only parent rules should create identity edits"
445+
);
446+
447+
let edit = edit.unwrap();
448+
// Verify it's an identity edit (replacement equals original content)
449+
assert_eq!(
450+
edit.replacement_string(),
451+
edit.p_match().matched_string(),
452+
"Match-only parent rules should create identity edits (no content change)"
445453
);
446454
}
447455

tests/test_piranha.py

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -391,6 +391,8 @@ def test_match_only_parent_rule_bug_fix():
391391
if (obj.isLocEnabled() || x > 0) {
392392
// do something
393393
x = obj.isLocEnabled();
394+
x = obj.isLocEnabled();
395+
x = obj.isLocEnabled();
394396
if (other_check) { }
395397
} else {
396398
// do something else!
@@ -410,7 +412,8 @@ def test_match_only_parent_rule_bug_fix():
410412
# This rule has no replace_node, making it match-only
411413
match_only_parent_rule = Rule(
412414
name="find_assignment_parent",
413-
query="cs :[x] = true;"
415+
query="cs :[x] = true;",
416+
is_seed_rule=False
414417
# Note: no replace_node specified = match-only rule
415418
)
416419

@@ -419,6 +422,7 @@ def test_match_only_parent_rule_bug_fix():
419422
name="logic_triggered_only_after_match_only_parent_rule",
420423
query='cs if (other_check) { }',
421424
replace_node="*",
425+
is_seed_rule=False
422426
)
423427

424428

@@ -455,9 +459,9 @@ def test_match_only_parent_rule_bug_fix():
455459
assert "x = true;" in result_code
456460

457461
# 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"
462+
# Before the bug fix, this line would be deleted (replaced with empty string)
463+
# After the bug fix, this line should remain (identity edit preserves content)
464+
assert "x = true;" in result_code, "Match-only parent rule must preserve the assignment"
461465

462-
# The if condition should also be deleted
463-
assert "if (other_check) { }" not in result_code
466+
# The subsequent rule should still be triggered and delete the if statement
467+
assert "if (other_check) { }" not in result_code, "Subsequent rules should still be triggered"

0 commit comments

Comments
 (0)