Skip to content

Commit

Permalink
Move has_comments to CommentRanges (#11495)
Browse files Browse the repository at this point in the history
## Summary

This PR moves the `has_comments` function from `Indexer` to
`CommentRanges`. The main motivation is that the `CommentRanges` will
now be built by the parser which is shared between the linter and the
formatter. Thus, the `CommentRanges` will be removed from the `Indexer`.

## Test Plan

`cargo test`
  • Loading branch information
dhruvmanila authored May 22, 2024
1 parent 5bb9720 commit f0046ab
Show file tree
Hide file tree
Showing 10 changed files with 65 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,11 @@ pub(crate) fn unittest_raises_assertion(
},
call.func.range(),
);
if !checker.indexer().has_comments(call, checker.locator()) {
if !checker
.indexer()
.comment_ranges()
.has_comments(call, checker.locator())
{
if let Some(args) = to_pytest_raises_args(checker, attr.as_str(), &call.arguments) {
diagnostic.try_set_fix(|| {
let (import_edit, binding) = checker.importer().get_or_import_symbol(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,11 @@ pub(crate) fn compare_with_tuple(checker: &mut Checker, expr: &Expr) {
}

// Avoid removing comments.
if checker.indexer().has_comments(expr, checker.locator()) {
if checker
.indexer()
.comment_ranges()
.has_comments(expr, checker.locator())
{
continue;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,11 @@ pub(crate) fn if_else_block_instead_of_dict_get(checker: &mut Checker, stmt_if:
},
stmt_if.range(),
);
if !checker.indexer().has_comments(stmt_if, checker.locator()) {
if !checker
.indexer()
.comment_ranges()
.has_comments(stmt_if, checker.locator())
{
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
contents,
stmt_if.range(),
Expand Down Expand Up @@ -295,7 +299,11 @@ pub(crate) fn if_exp_instead_of_dict_get(
},
expr.range(),
);
if !checker.indexer().has_comments(expr, checker.locator()) {
if !checker
.indexer()
.comment_ranges()
.has_comments(expr, checker.locator())
{
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
contents,
expr.range(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,11 @@ pub(crate) fn if_else_block_instead_of_if_exp(checker: &mut Checker, stmt_if: &a
},
stmt_if.range(),
);
if !checker.indexer().has_comments(stmt_if, checker.locator()) {
if !checker
.indexer()
.comment_ranges()
.has_comments(stmt_if, checker.locator())
{
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
contents,
stmt_if.range(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,11 @@ pub(crate) fn needless_bool(checker: &mut Checker, stmt: &Stmt) {
}

// Generate the replacement condition.
let condition = if checker.indexer().has_comments(&range, checker.locator()) {
let condition = if checker
.indexer()
.comment_ranges()
.has_comments(&range, checker.locator())
{
None
} else {
// If the return values are inverted, wrap the condition in a `not`.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,11 @@ pub(crate) fn suppressible_exception(
},
stmt.range(),
);
if !checker.indexer().has_comments(stmt, checker.locator()) {
if !checker
.indexer()
.comment_ranges()
.has_comments(stmt, checker.locator())
{
diagnostic.try_set_fix(|| {
// let range = statement_range(stmt, checker.locator(), checker.indexer());

Expand Down
6 changes: 5 additions & 1 deletion crates/ruff_linter/src/rules/pylint/rules/nested_min_max.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,11 @@ pub(crate) fn nested_min_max(
MinMax::try_from_call(func.as_ref(), keywords.as_ref(), checker.semantic()) == Some(min_max)
}) {
let mut diagnostic = Diagnostic::new(NestedMinMax { func: min_max }, expr.range());
if !checker.indexer().has_comments(expr, checker.locator()) {
if !checker
.indexer()
.comment_ranges()
.has_comments(expr, checker.locator())
{
let flattened_expr = Expr::Call(ast::ExprCall {
func: Box::new(func.clone()),
arguments: Arguments {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,11 @@ pub(crate) fn collection_literal_concatenation(checker: &mut Checker, expr: &Exp
},
expr.range(),
);
if !checker.indexer().has_comments(expr, checker.locator()) {
if !checker
.indexer()
.comment_ranges()
.has_comments(expr, checker.locator())
{
// This suggestion could be unsafe if the non-literal expression in the
// expression has overridden the `__add__` (or `__radd__`) magic methods.
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
Expand Down
19 changes: 0 additions & 19 deletions crates/ruff_python_index/src/indexer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,25 +112,6 @@ impl Indexer {
self.continuation_lines.binary_search(&line_start).is_ok()
}

/// Returns `true` if a statement or expression includes at least one comment.
pub fn has_comments<T>(&self, node: &T, locator: &Locator) -> bool
where
T: Ranged,
{
let start = if has_leading_content(node.start(), locator) {
node.start()
} else {
locator.line_start(node.start())
};
let end = if has_trailing_content(node.end(), locator) {
node.end()
} else {
locator.line_end(node.end())
};

self.comment_ranges().intersects(TextRange::new(start, end))
}

/// Given an offset at the end of a line (including newlines), return the offset of the
/// continuation at the end of that line.
fn find_continuation(&self, offset: TextSize, locator: &Locator) -> Option<TextSize> {
Expand Down
21 changes: 20 additions & 1 deletion crates/ruff_python_trivia/src/comment_ranges.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use ruff_source_file::Locator;

use ruff_text_size::{Ranged, TextRange, TextSize};

use crate::is_python_whitespace;
use crate::{has_leading_content, has_trailing_content, is_python_whitespace};

/// Stores the ranges of comments sorted by [`TextRange::start`] in increasing order. No two ranges are overlapping.
#[derive(Clone, Default)]
Expand Down Expand Up @@ -49,6 +49,25 @@ impl CommentRanges {
}
}

/// Returns `true` if a statement or expression includes at least one comment.
pub fn has_comments<T>(&self, node: &T, locator: &Locator) -> bool
where
T: Ranged,
{
let start = if has_leading_content(node.start(), locator) {
node.start()
} else {
locator.line_start(node.start())
};
let end = if has_trailing_content(node.end(), locator) {
node.end()
} else {
locator.line_end(node.end())
};

self.intersects(TextRange::new(start, end))
}

/// Given a [`CommentRanges`], determine which comments are grouped together
/// in "comment blocks". A "comment block" is a sequence of consecutive
/// own-line comments in which the comment hash (`#`) appears in the same
Expand Down

0 comments on commit f0046ab

Please sign in to comment.