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

Skip LibCST parsing for standard dedent adjustments #9769

Merged
merged 2 commits into from
Feb 2, 2024
Merged
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
10 changes: 10 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pyupgrade/UP036_0.py
Original file line number Diff line number Diff line change
Expand Up @@ -215,3 +215,13 @@ def g():

if sys.version_info[:3] > (3,13):
print("py3")

if sys.version_info > (3,0):
f"this is\
allowed too"

f"""the indentation on
this line is significant"""

"this is\
allowed too"
43 changes: 31 additions & 12 deletions crates/ruff_linter/src/fix/edits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use ruff_python_ast::{self as ast, Arguments, ExceptHandler, Stmt};
use ruff_python_ast::{AnyNodeRef, ArgOrKeyword};
use ruff_python_codegen::Stylist;
use ruff_python_index::Indexer;
use ruff_python_trivia::textwrap::dedent_to;
use ruff_python_trivia::{
has_leading_content, is_python_whitespace, CommentRanges, PythonWhitespace, SimpleTokenKind,
SimpleTokenizer,
Expand Down Expand Up @@ -169,29 +170,47 @@ pub(crate) fn add_argument(
}

/// Safely adjust the indentation of the indented block at [`TextRange`].
///
/// The [`TextRange`] is assumed to represent an entire indented block, including the leading
/// indentation of that block. For example, to dedent the body here:
/// ```python
/// if True:
/// print("Hello, world!")
/// ```
///
/// The range would be the entirety of ` print("Hello, world!")`.
pub(crate) fn adjust_indentation(
range: TextRange,
indentation: &str,
locator: &Locator,
indexer: &Indexer,
stylist: &Stylist,
) -> Result<String> {
let contents = locator.slice(range);
// If the range includes a multi-line string, use LibCST to ensure that we don't adjust the
// whitespace _within_ the string.
if indexer.multiline_ranges().intersects(range) || indexer.fstring_ranges().intersects(range) {
let contents = locator.slice(range);

let module_text = format!("def f():{}{contents}", stylist.line_ending().as_str());
let module_text = format!("def f():{}{contents}", stylist.line_ending().as_str());

let mut tree = match_statement(&module_text)?;
let mut tree = match_statement(&module_text)?;

let embedding = match_function_def(&mut tree)?;
let embedding = match_function_def(&mut tree)?;

let indented_block = match_indented_block(&mut embedding.body)?;
indented_block.indent = Some(indentation);
let indented_block = match_indented_block(&mut embedding.body)?;
indented_block.indent = Some(indentation);

let module_text = indented_block.codegen_stylist(stylist);
let module_text = module_text
.strip_prefix(stylist.line_ending().as_str())
.unwrap()
.to_string();
Ok(module_text)
let module_text = indented_block.codegen_stylist(stylist);
let module_text = module_text
.strip_prefix(stylist.line_ending().as_str())
.unwrap()
.to_string();
Ok(module_text)
} else {
// Otherwise, we can do a simple adjustment ourselves.
let contents = locator.slice(range);
Ok(dedent_to(contents, indentation))
}
}

/// Determine if a vector contains only one, specific element.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -852,6 +852,7 @@ fn remove_else(
TextRange::new(else_colon_end, elif_else.end()),
desired_indentation,
locator,
indexer,
stylist,
)?;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ pub(crate) fn trailing_whitespace(
diagnostic.set_fix(Fix::applicable_edit(
Edit::range_deletion(range),
// Removing trailing whitespace is not safe inside multiline strings.
if indexer.multiline_ranges().intersects(range) {
if indexer.multiline_ranges().contains_range(range) {
Applicability::Unsafe
} else {
Applicability::Safe
Expand Down
14 changes: 12 additions & 2 deletions crates/ruff_linter/src/rules/pylint/rules/collapsible_else_if.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, ElifElseClause, Stmt};
use ruff_python_codegen::Stylist;
use ruff_python_index::Indexer;
use ruff_source_file::Locator;
use ruff_text_size::{Ranged, TextRange};

Expand Down Expand Up @@ -84,8 +85,15 @@ pub(crate) fn collapsible_else_if(checker: &mut Checker, stmt: &Stmt) {
CollapsibleElseIf,
TextRange::new(else_clause.start(), first.start()),
);
diagnostic
.try_set_fix(|| convert_to_elif(first, else_clause, checker.locator(), checker.stylist()));
diagnostic.try_set_fix(|| {
convert_to_elif(
first,
else_clause,
checker.locator(),
checker.indexer(),
checker.stylist(),
)
});
checker.diagnostics.push(diagnostic);
}

Expand All @@ -94,6 +102,7 @@ fn convert_to_elif(
first: &Stmt,
else_clause: &ElifElseClause,
locator: &Locator,
indexer: &Indexer,
stylist: &Stylist,
) -> Result<Fix> {
let inner_if_line_start = locator.line_start(first.start());
Expand All @@ -109,6 +118,7 @@ fn convert_to_elif(
TextRange::new(inner_if_line_start, inner_if_line_end),
indentation,
locator,
indexer,
stylist,
)?;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::identifier;
use ruff_python_ast::{self as ast, ExceptHandler, MatchCase, Stmt};
use ruff_python_codegen::Stylist;
use ruff_python_index::Indexer;
use ruff_source_file::Locator;
use ruff_text_size::{Ranged, TextRange};

Expand Down Expand Up @@ -81,6 +82,7 @@ pub(crate) fn useless_else_on_loop(
orelse,
else_range,
checker.locator(),
checker.indexer(),
checker.stylist(),
)
});
Expand Down Expand Up @@ -134,6 +136,7 @@ fn remove_else(
orelse: &[Stmt],
else_range: TextRange,
locator: &Locator,
indexer: &Indexer,
stylist: &Stylist,
) -> Result<Fix> {
let Some(start) = orelse.first() else {
Expand Down Expand Up @@ -164,6 +167,7 @@ fn remove_else(
),
desired_indentation,
locator,
indexer,
stylist,
)?;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,7 @@ fn fix_always_false_branch(
),
indentation,
checker.locator(),
checker.indexer(),
checker.stylist(),
)
.ok()
Expand Down Expand Up @@ -376,6 +377,7 @@ fn fix_always_true_branch(
TextRange::new(checker.locator().line_start(start.start()), end.end()),
indentation,
checker.locator(),
checker.indexer(),
checker.stylist(),
)
.ok()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -774,4 +774,32 @@ UP036_0.py:210:4: UP036 [*] Version block is outdated for minimum Python version
213 212 | if sys.version_info[:2] > (3,13):
214 213 | print("py3")

UP036_0.py:219:4: UP036 [*] Version block is outdated for minimum Python version
|
217 | print("py3")
218 |
219 | if sys.version_info > (3,0):
| ^^^^^^^^^^^^^^^^^^^^^^^^ UP036
220 | f"this is\
221 | allowed too"
|
= help: Remove outdated version block

ℹ Unsafe fix
216 216 | if sys.version_info[:3] > (3,13):
217 217 | print("py3")
218 218 |
219 |-if sys.version_info > (3,0):
220 |- f"this is\
219 |+f"this is\
221 220 | allowed too"
222 221 |
223 |- f"""the indentation on
222 |+f"""the indentation on
224 223 | this line is significant"""
225 224 |
226 |- "this is\
225 |+"this is\
227 226 | allowed too"


8 changes: 8 additions & 0 deletions crates/ruff_python_index/src/fstring_ranges.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@ pub struct FStringRanges {
}

impl FStringRanges {
/// Returns `true` if the given range intersects with any f-string range.
pub fn intersects(&self, target: TextRange) -> bool {
self.raw
.values()
.take_while(|range| range.start() < target.end())
.any(|range| target.intersect(*range).is_some())
}

/// Return the [`TextRange`] of the innermost f-string at the given offset.
pub fn innermost(&self, offset: TextSize) -> Option<TextRange> {
self.raw
Expand Down
17 changes: 16 additions & 1 deletion crates/ruff_python_index/src/multiline_ranges.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ pub struct MultilineRanges {

impl MultilineRanges {
/// Returns `true` if the given range is inside a multiline string.
pub fn intersects(&self, target: TextRange) -> bool {
pub fn contains_range(&self, target: TextRange) -> bool {
self.ranges
.binary_search_by(|range| {
if range.contains_range(target) {
Expand All @@ -22,6 +22,21 @@ impl MultilineRanges {
})
.is_ok()
}

/// Returns `true` if the given range intersects with any multiline string.
pub fn intersects(&self, target: TextRange) -> bool {
self.ranges
.binary_search_by(|range| {
if target.intersect(*range).is_some() {
std::cmp::Ordering::Equal
} else if range.end() < target.start() {
std::cmp::Ordering::Less
} else {
std::cmp::Ordering::Greater
}
})
.is_ok()
}
}

#[derive(Default)]
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_python_trivia/src/comment_ranges.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@ impl CommentRanges {
Self { raw: ranges }
}

/// Returns `true` if the given range includes a comment.
/// Returns `true` if the given range intersects with any comment range.
pub fn intersects(&self, target: TextRange) -> bool {
self.raw
.binary_search_by(|range| {
if target.contains_range(*range) {
if target.intersect(*range).is_some() {
std::cmp::Ordering::Equal
} else if range.end() < target.start() {
std::cmp::Ordering::Less
Expand Down
79 changes: 78 additions & 1 deletion crates/ruff_python_trivia/src/textwrap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ pub fn indent<'a>(text: &'a str, prefix: &str) -> Cow<'a, str> {
/// Removes common leading whitespace from each line.
///
/// This function will look at each non-empty line and determine the
/// maximum amount of whitespace that can be removed from all lines:
/// maximum amount of whitespace that can be removed from all lines.
///
/// Lines that consist solely of whitespace are trimmed to a blank line.
///
/// ```
/// # use ruff_python_trivia::textwrap::dedent;
Expand Down Expand Up @@ -122,6 +124,51 @@ pub fn dedent(text: &str) -> Cow<'_, str> {
Cow::Owned(result)
}

/// Reduce a block's indentation to match the provided indentation.
///
/// This function looks at the first line in the block to determine the
/// current indentation, then removes whitespace from each line to
/// match the provided indentation.
///
/// Lines that are indented by _less_ than the indent of the first line
/// are left unchanged.
///
/// Lines that consist solely of whitespace are trimmed to a blank line.
///
/// # Panics
/// If the first line is indented by less than the provided indent.
pub fn dedent_to(text: &str, indent: &str) -> String {
// Look at the indentation of the first line, to determine the "baseline" indentation.
let existing_indent_len = text
.universal_newlines()
.next()
.map_or(0, |line| line.len() - line.trim_start().len());
Comment on lines +141 to +145
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Why not indentation_at_offset?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the text here is the "full line", not the start of the content.


// Determine the amount of indentation to remove.
let dedent_len = existing_indent_len - indent.len();

let mut result = String::with_capacity(text.len() + indent.len());
for line in text.universal_newlines() {
let trimmed = line.trim_whitespace_start();
if trimmed.is_empty() {
if let Some(line_ending) = line.line_ending() {
charliermarsh marked this conversation as resolved.
Show resolved Hide resolved
result.push_str(&line_ending);
}
} else {
// Determine the current indentation level.
let current_indent_len = line.len() - trimmed.len();
if current_indent_len < existing_indent_len {
// If the current indentation level is less than the baseline, keep it as is.
result.push_str(line.as_full_str());
} else {
// Otherwise, reduce the indentation level.
result.push_str(&line.as_full_str()[dedent_len..]);
}
}
}
result
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -344,4 +391,34 @@ mod tests {
]";
assert_eq!(dedent(text), text);
}

#[test]
#[rustfmt::skip]
fn adjust_indent() {
let x = [
" foo",
" bar",
" ",
" baz"
].join("\n");
let y = [
" foo",
" bar",
"",
" baz"
].join("\n");
assert_eq!(dedent_to(&x, " "), y);

let x = [
" foo",
" bar",
" baz",
].join("\n");
let y = [
"foo",
" bar",
"baz"
].join("\n");
assert_eq!(dedent_to(&x, ""), y);
}
}
Loading