Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove blank lines instead of emptying it's contents #667

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/models/concrete_syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,13 @@ pub(crate) fn get_all_matches_for_concrete_syntax(
};

match_map.insert(replace_node_key, replace_node_match.clone());

matches.push(Match {
matched_string: replace_node_match.text,
range: replace_node_match.range,
matches: match_map.into_iter().map(|(k, v)| (k, v.text)).collect(),
associated_comma: None,
associated_comments: Vec::new(),
associated_leading_empty_lines: Vec::new(),
});
}
if recursive {
Expand Down
44 changes: 43 additions & 1 deletion src/models/matches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ pub struct Match {
#[get_mut]
#[serde(skip)]
pub(crate) associated_comments: Vec<Range>,
#[get]
#[get_mut]
#[serde(skip)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not just empty lines right? All leading white spaes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only empty lines, we match leading white spaces that starts with a new line character.
https://github.com/uber/piranha/pull/667/files#diff-b157ce35dcfb24fa5ff9bfb7323302e655455ad2a78bb484fdd2e855d7e740f4R143

pub(crate) associated_leading_empty_lines: Vec<Range>,
}

#[pymethods]
Expand All @@ -74,6 +78,7 @@ impl Match {
matches,
associated_comma: None,
associated_comments: Vec::new(),
associated_leading_empty_lines: Vec::new(),
}
}

Expand All @@ -85,6 +90,7 @@ impl Match {
let associated_ranges = [
self.associated_comma().iter().collect_vec(),
self.associated_comments().iter().collect_vec(),
self.associated_leading_empty_lines().iter().collect_vec(),
]
.concat()
.iter()
Expand Down Expand Up @@ -117,8 +123,45 @@ impl Match {
) {
self.get_associated_elements(node, code, piranha_arguments, true);
self.get_associated_elements(node, code, piranha_arguments, false);
self.get_associated_leading_empty_lines(node, code);
}

fn get_associated_leading_empty_lines(&mut self, matched_node: &Node, code: &String) -> () {
if let Some(empty_range) = self.find_empty_line_range(code, matched_node.start_byte()) {
let skipped_range = Range {
start_byte: empty_range.start,
end_byte: empty_range.end,
start_point: position_for_offset(code.as_bytes(), empty_range.start),
end_point: position_for_offset(code.as_bytes(), empty_range.end),
};
self
.associated_leading_empty_lines_mut()
.push(skipped_range);
}
}

fn find_empty_line_range(&self, code: &str, start_byte: usize) -> Option<std::ops::Range<usize>> {
let mut end_byte = start_byte;
let code_bytes = code.as_bytes();
let mut found = false;
while end_byte > 0 {
let prev_char = code_bytes[end_byte - 1] as char;
if prev_char.is_whitespace() {
end_byte -= 1;
if prev_char == '\n' {
found = true;
break;
}
} else {
break;
}
}
if found {
Some(end_byte..start_byte)
} else {
None
}
}
fn found_comma(&self) -> bool {
self.associated_comma().is_some()
}
Expand All @@ -143,7 +186,6 @@ impl Match {
current_node.prev_sibling()
} {
// Check if the sibling is a comma

if !self.found_comma() && self.is_comma_safe_to_delete(&sibling, code, trailing) {
// Add the comma to the associated matches
self.associated_comma = Some(sibling.range().into());
Expand Down
76 changes: 75 additions & 1 deletion src/models/unit_tests/source_code_unit_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use tree_sitter::Parser;
use crate::{
filter,
models::{
default_configs::{JAVA, UNUSED_CODE_PATH},
default_configs::{JAVA, RUBY, UNUSED_CODE_PATH},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
default_configs::{JAVA, RUBY, UNUSED_CODE_PATH},
default_configs::{JAVA, RUBY, SWIFT, UNUSED_CODE_PATH},

filter::Filter,
language::PiranhaLanguage,
matches::{Point, Range},
Expand Down Expand Up @@ -699,3 +699,77 @@ fn test_satisfies_outermost_enclosing_node() {

assert!(source_code_unit.is_satisfied(*node, &rule_positive, &HashMap::new(), &mut rule_store,));
}

#[test]
fn test_removes_blank_lines_after_inline_cleanup() {
let inline_cleanup_rule = piranha_rule! {
name= "inline_cleanup_rule",
query= "
(
(if_modifier
body : ((_) @body)
[
condition: (false)
condition: (parenthesized_statements (false))
]
)@if_modifier
)
",
replace_node = "if_modifier",
replace = ""
};

let inline_rule = InstantiatedRule::new(&inline_cleanup_rule, &HashMap::new());

let source_code = r#"
def method_name
do_something if false
end
"#
.trim();

let piranha_arguments = PiranhaArgumentsBuilder::default()
.paths_to_codebase(vec![UNUSED_CODE_PATH.to_string()])
.language(PiranhaLanguage::from(RUBY))
.build();

let mut rule_store = RuleStore::new(&piranha_arguments);
let mut parser = piranha_arguments.language().parser();

let source_code_unit = SourceCodeUnit::new(
&mut parser,
source_code.to_string(),
&HashMap::new(),
PathBuf::new().as_path(),
&piranha_arguments,
);
let matches = source_code_unit.get_matches(
&inline_rule,
&mut rule_store,
source_code_unit.root_node(),
true,
);
assert_eq!(matches.len(), 1);
assert_eq!(
matches
.first()
.unwrap()
.associated_leading_empty_lines
.len(),
1
);
assert_eq!(
*matches
.first()
.unwrap()
.associated_leading_empty_lines
.first()
.unwrap(),
Range {
start_byte: 15,
end_byte: 24,
start_point: Point { row: 0, column: 15 },
end_point: Point { row: 1, column: 8 }
}
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Bennet-Sunder ,

Thank you for your contribution! I tested this internally and noticed that the change might break some rewrites. The issue is that in some grammars (particularly Swift), whitespace and newlines are part of the node. If you delete them, you might accidentally delete parts of a sibling node unrelated to the node you're deleting.

Please accept this test suggestion to see what I mean. The test should fail with your change (make sure to add Swift at the top). My proposed solution is to delete only whitespace or newlines until you find the end_byte of the previous sibling node. Let me know if you need help with this.

Suggested change
}
}
#[test]
fn test_switch_entry_blank_lines() {
let inline_cleanup_rule = piranha_rule! {
name= "inline_cleanup_rule",
query= "
(
(switch_entry
(switch_pattern) @p1
(switch_pattern) @p2
(switch_pattern) @p3
(switch_pattern) @p4
(switch_pattern) @p5
(switch_pattern) @p6
) @custom_entry
(#eq? @p1 \".case_zeta_first\")
)
",
replace_node = "custom_entry",
replace = ""
};
let inline_rule = InstantiatedRule::new(&inline_cleanup_rule, &HashMap::new());
let source_code = r#"
public var namespace: ParameterNamespace {
switch self {
case .case_random_word_alpha,
.case_random_word_beta,
.case_random_word_gamma,
.case_random_word_delta:
return .namespace_group_alpha
case .case_zeta_first,
.case_zeta_second,
.case_zeta_third,
.case_zeta_fourth,
.case_zeta_fifth,
.case_zeta_sixth:
return .namespace_group_beta
case .case_random_word_omega:
return .namespace_group_gamma
}
}
"#
.trim();
let piranha_arguments = PiranhaArgumentsBuilder::default()
.paths_to_codebase(vec![UNUSED_CODE_PATH.to_string()])
.language(PiranhaLanguage::from(SWIFT))
.build();
let mut rule_store = RuleStore::new(&piranha_arguments);
let mut parser = piranha_arguments.language().parser();
let mut source_code_unit = SourceCodeUnit::new(
&mut parser,
source_code.to_string(),
&HashMap::new(),
PathBuf::new().as_path(),
&piranha_arguments,
);
source_code_unit.apply_rule(inline_rule, &mut rule_store, &mut parser, &None);
let transformed_code = source_code_unit.code();
let expected_code = r#"
public var namespace: ParameterNamespace {
switch self {
case .case_random_word_alpha,
.case_random_word_beta,
.case_random_word_gamma,
.case_random_word_delta:
return .namespace_group_alpha
case .case_random_word_omega:
return .namespace_group_gamma
}
}
"#
.trim();
assert_eq!(transformed_code, expected_code);
}