Skip to content

Commit

Permalink
Address suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
@snowsignal
snowsignal committed Feb 27, 2024
1 parent 5ab92d8 commit 5b80da0
Show file tree
Hide file tree
Showing 6 changed files with 416 additions and 439 deletions.
18 changes: 16 additions & 2 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@ def fmt_off_between_lists():
3,
]


# note: the second `fmt: skip`` should be OK
def fmt_skip_on_own_line():
# fmt: skip
pass # fmt: skip
pass # fmt: skip


@fmt_skip_on_own_line
Expand All @@ -18,12 +19,13 @@ def fmt_skip_on_own_line():
def fmt_off_between_decorators():
pass


@fmt_off_between_decorators
# fmt: off
@fmt_off_between_lists
class FmtOffBetweenClassDecorators:
...


def fmt_off_in_else():
x = [1, 2, 3]
for val in x:
Expand All @@ -44,6 +46,18 @@ def fmt_off_in_else():
else:
print("expected")


class Test:
@classmethod
# fmt: off
def cls_method_a(
# fmt: off
cls,
) -> None:
# noqa: test # fmt: skip
pass


def fmt_on_trailing():
# fmt: off
val = 5 # fmt: on
Expand Down
76 changes: 23 additions & 53 deletions crates/ruff_linter/src/rules/ruff/rules/invalid_formatter_suppression_comment.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ use crate::checkers::ast::Checker;
use crate::fix::edits::delete_comment;

use super::suppression_comment_visitor::{
own_line_comment_indentation, CaptureSuppressionComment, SuppressionComment,
SuppressionCommentData, SuppressionCommentVisitor,
CaptureSuppressionComment, SuppressionComment, SuppressionCommentData,
SuppressionCommentVisitor,
};

/// ## What it does
Expand Down Expand Up @@ -127,17 +127,15 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> {
// check if the comment is inside of an expression.
if comment
.enclosing
.map(AnyNodeRef::is_expression)
.map(|n| !AnyNodeRef::is_statement(n))
.unwrap_or_default()
{
return Err(IgnoredReason::InsideExpression);
return Err(IgnoredReason::InNonStatement);
}

// check if a skip comment is at the end of a line
if comment.kind == SuppressionKind::Skip && !comment.line_position.is_end_of_line() {
if !comment.line_position.is_end_of_line() {
return Err(IgnoredReason::SkipHasToBeTrailing);
}
return Err(IgnoredReason::SkipHasToBeTrailing);
}

if comment.kind == SuppressionKind::Off || comment.kind == SuppressionKind::On {
Expand All @@ -155,9 +153,9 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> {
) = comment.enclosing
{
if comment.line_position.is_own_line() && comment.range.start() < name.start() {
if let Some(decorator) = decorator_list.last() {
if decorator.start() > comment.range.end() {
return Err(IgnoredReason::BetweenDecorators);
if let Some(decorator) = decorator_list.first() {
if decorator.end() < comment.range.start() {
return Err(IgnoredReason::AfterDecorator);
}
}
}
Expand All @@ -168,10 +166,13 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> {
if let (Some(enclosing), Some(preceding), Some(following)) =
(comment.enclosing, comment.preceding, comment.following)
{
if is_first_statement_in_alternate_body(following, enclosing) {
if following.is_first_statement_in_alternate_body(enclosing) {
// check indentation
let comment_indentation =
own_line_comment_indentation(preceding, comment.range, self.locator);
let comment_indentation = AnyNodeRef::comment_indentation_after(
preceding,
comment.range,
self.locator,
);

let preceding_indentation =
indentation_at_offset(preceding.start(), self.locator)
Expand Down Expand Up @@ -214,44 +215,10 @@ impl<'src, 'loc> CaptureSuppressionComment<'src> for UselessSuppressionComments<
}
}

/// Returns `true` if `statement` is the first statement in an alternate `body` (e.g. the else of an if statement)
fn is_first_statement_in_alternate_body(statement: AnyNodeRef, has_body: AnyNodeRef) -> bool {
match has_body {
AnyNodeRef::StmtFor(ast::StmtFor { orelse, .. })
| AnyNodeRef::StmtWhile(ast::StmtWhile { orelse, .. }) => {
are_same_optional(statement, orelse.first())
}

AnyNodeRef::StmtTry(ast::StmtTry {
handlers,
orelse,
finalbody,
..
}) => {
are_same_optional(statement, handlers.first())
|| are_same_optional(statement, orelse.first())
|| are_same_optional(statement, finalbody.first())
}

AnyNodeRef::StmtIf(ast::StmtIf {
elif_else_clauses, ..
}) => are_same_optional(statement, elif_else_clauses.first()),
_ => false,
}
}

/// Returns `true` if `right` is `Some` and `left` and `right` are referentially equal.
fn are_same_optional<'a, T>(left: AnyNodeRef, right: Option<T>) -> bool
where
T: Into<AnyNodeRef<'a>>,
{
right.is_some_and(|right| left.ptr_eq(right.into()))
}

#[derive(Clone, Copy, Debug, PartialEq, Eq)]
enum IgnoredReason {
InsideExpression,
BetweenDecorators,
InNonStatement,
AfterDecorator,
SkipHasToBeTrailing,
FmtOnCannotBeTrailing,
FmtOffAboveBlock,
Expand All @@ -260,9 +227,12 @@ enum IgnoredReason {
impl Display for IgnoredReason {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Self::InsideExpression => write!(f, "it cannot be inside an expression"),
Self::BetweenDecorators => {
write!(f, "it cannot be between decorators")
Self::InNonStatement => write!(
f,
"it cannot be in an expression, pattern, argument list, or other non-statement"
),
Self::AfterDecorator => {
write!(f, "it cannot be after a decorator")
}
Self::SkipHasToBeTrailing => {
write!(f, "it cannot be on its own line")
Expand All @@ -271,7 +241,7 @@ impl Display for IgnoredReason {
write!(f, "it cannot be at the end of a line")
}
Self::FmtOffAboveBlock => {
write!(f, "it cannot suppress formatting for an ambiguous region")
write!(f, "it cannot be directly above an alternate body")
}
}
}
Expand Down
98 changes: 15 additions & 83 deletions crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ use ruff_python_ast::{
AnyNodeRef, Suite,
};
use ruff_python_trivia::{
indentation_at_offset, CommentLinePosition, SimpleTokenKind, SimpleTokenizer, SuppressionKind,
indentation_at_offset, CommentLinePosition, SimpleTokenizer, SuppressionKind,
};
use ruff_source_file::Locator;
use ruff_text_size::{Ranged, TextLen, TextRange, TextSize};
use ruff_text_size::{Ranged, TextRange, TextSize};

#[derive(Clone, Copy, Debug)]
pub(super) struct SuppressionComment {
Expand Down Expand Up @@ -124,16 +124,17 @@ where
break;
}
}
let comment_indent = indentation_at_offset(range.start(), self.locator)
.unwrap_or_default()
.len();
let node_indent = indentation_at_offset(node.start(), self.locator)
.unwrap_or_default()
.len();
let comment_indent =
AnyNodeRef::comment_indentation_after(node, range, self.locator);
let node_indent = TextSize::of(
indentation_at_offset(node.start(), self.locator).unwrap_or_default(),
);
if node_indent >= comment_indent {
break;
}
} else {
// If the end-of-line comment is not on the same line, we can assume that there's a node in between
// the end of this node and this comment.
if self.locator.line_start(range.start())
!= self.locator.line_start(node.start())
{
Expand Down Expand Up @@ -182,91 +183,22 @@ where

#[derive(Clone, Debug)]
pub(super) struct SuppressionCommentData<'src> {
/// If `enclosing` is `None`, this comment is top-level
/// The AST node that encloses the comment. If `enclosing` is `None`, this comment is a top-level statement.
pub(super) enclosing: Option<AnyNodeRef<'src>>,
/// An AST node that comes directly before the comment. A child of `enclosing`.
pub(super) preceding: Option<AnyNodeRef<'src>>,
/// An AST node that comes directly after the comment. A child of `enclosing`.
pub(super) following: Option<AnyNodeRef<'src>>,

/// The line position of the comment - it can either be on its own line, or at the end of a line.
pub(super) line_position: CommentLinePosition,
/// Whether this comment is `fmt: off`, `fmt: on`, or `fmt: skip` (or `yapf disable` / `yapf enable`)
pub(super) kind: SuppressionKind,
/// The range of text that makes up the comment. This includes the `#` prefix.
pub(super) range: TextRange,
}

pub(super) trait CaptureSuppressionComment<'src> {
/// This is the entrypoint for the capturer to analyze the next comment.
fn capture(&mut self, comment: SuppressionCommentData<'src>);
}

/// Determine the indentation level of an own-line comment, defined as the minimum indentation of
/// all comments between the preceding node and the comment, including the comment itself. In
/// other words, we don't allow successive comments to ident _further_ than any preceding comments.
///
/// For example, given:
/// ```python
/// if True:
/// pass
/// # comment
/// ```
///
/// The indentation would be 4, as the comment is indented by 4 spaces.
///
/// Given:
/// ```python
/// if True:
/// pass
/// # comment
/// else:
/// pass
/// ```
///
/// The indentation would be 0, as the comment is not indented at all.
///
/// Given:
/// ```python
/// if True:
/// pass
/// # comment
/// # comment
/// ```
///
/// Both comments would be marked as indented at 4 spaces, as the indentation of the first comment
/// is used for the second comment.
///
/// This logic avoids pathological cases like:
/// ```python
/// try:
/// if True:
/// if True:
/// pass
///
/// # a
/// # b
/// # c
/// except Exception:
/// pass
/// ```
///
/// If we don't use the minimum indentation of any preceding comments, we would mark `# b` as
/// indented to the same depth as `pass`, which could in turn lead to us treating it as a trailing
/// comment of `pass`, despite there being a comment between them that "resets" the indentation.
pub(super) fn own_line_comment_indentation(
preceding: AnyNodeRef,
comment_range: TextRange,
locator: &Locator,
) -> TextSize {
let tokenizer = SimpleTokenizer::new(
locator.contents(),
TextRange::new(locator.full_line_end(preceding.end()), comment_range.end()),
);

tokenizer
.filter_map(|token| {
if token.kind() == SimpleTokenKind::Comment {
indentation_at_offset(token.start(), locator).map(TextLen::text_len)
} else {
None
}
})
.min()
.unwrap_or_default()
}

0 comments on commit 5b80da0

Please sign in to comment.