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

ERA001: Ignore script-comments with multiple end-tags #13283

Merged
merged 1 commit into from
Sep 9, 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
13 changes: 12 additions & 1 deletion crates/ruff_linter/resources/test/fixtures/eradicate/ERA001.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,18 @@ class A():
# ]
# ///

# Script tag without a closing tag (OK)
# Script tag with multiple closing tags (OK)
# /// script
# [tool.uv]
# extra-index-url = ["https://pypi.org/simple", """\
# https://example.com/
# ///
# """
# ]
# ///
print(1)

# Script tag without a closing tag (Error)

# /// script
# requires-python = ">=3.11"
Expand Down
190 changes: 164 additions & 26 deletions crates/ruff_linter/src/rules/eradicate/rules/commented_out_code.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use crate::settings::LinterSettings;
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_trivia::CommentRanges;
use ruff_source_file::Locator;

use crate::settings::LinterSettings;
use ruff_source_file::{Locator, UniversalNewlineIterator};
use ruff_text_size::TextRange;

use super::super::detection::comment_contains_code;

Expand Down Expand Up @@ -50,27 +50,15 @@ pub(crate) fn commented_out_code(
comment_ranges: &CommentRanges,
settings: &LinterSettings,
) {
// Skip comments within `/// script` tags.
let mut in_script_tag = false;

let mut comments = comment_ranges.into_iter().peekable();
// Iterate over all comments in the document.
for range in comment_ranges {
let line = locator.lines(range);
while let Some(range) = comments.next() {
let line = locator.line(range.start());

// Detect `/// script` tags.
if in_script_tag {
if is_script_tag_end(line) {
in_script_tag = false;
if is_script_tag_start(line) {
if skip_script_comments(range, &mut comments, locator) {
continue;
}
} else {
if is_script_tag_start(line) {
in_script_tag = true;
}
}

// Skip comments within `/// script` tags.
if in_script_tag {
continue;
}

// Verify that the comment is on its own line, and that it contains code.
Expand All @@ -84,6 +72,88 @@ pub(crate) fn commented_out_code(
}
}

/// Parses the rest of a [PEP 723](https://peps.python.org/pep-0723/)
/// script comment and moves `comments` past the script comment's end unless
/// the script comment is invalid.
///
/// Returns `true` if it is a valid script comment.
fn skip_script_comments<I>(
script_start: TextRange,
comments: &mut std::iter::Peekable<I>,
locator: &Locator,
) -> bool
where
I: Iterator<Item = TextRange>,
{
let line_end = locator.full_line_end(script_start.end());
let rest = locator.after(line_end);
let mut end_offset = None;
let mut lines = UniversalNewlineIterator::with_offset(rest, line_end).peekable();

while let Some(line) = lines.next() {
let Some(content) = script_line_content(&line) else {
break;
};

if content == "///" {
// > Precedence for an ending line # /// is given when the next line is not a valid
// > embedded content line as described above.
// > For example, the following is a single fully valid block:
// > ```python
// > # /// some-toml
// > # embedded-csharp = """
// > # /// <summary>
// > # /// text
// > # ///
// > # /// </summary>
// > # public class MyClass { }
// > # """
// > # ///
// ````
if lines.next().is_some_and(|line| is_valid_script_line(&line)) {
continue;
}
end_offset = Some(line.full_end());
break;
}
}

// > Unclosed blocks MUST be ignored.
let Some(end_offset) = end_offset else {
return false;
};

// Skip over all script-comments.
while let Some(comment) = comments.peek() {
if comment.start() >= end_offset {
break;
}

comments.next();
}

true
}

fn script_line_content(line: &str) -> Option<&str> {
let Some(rest) = line.strip_prefix('#') else {
// Not a comment
return None;
};

// An empty line
if rest.is_empty() {
return Some("");
}

// > If there are characters after the # then the first character MUST be a space.
rest.strip_prefix(' ')
}

fn is_valid_script_line(line: &str) -> bool {
script_line_content(line).is_some()
}

/// Returns `true` if line contains an own-line comment.
fn is_own_line_comment(line: &str) -> bool {
for char in line.chars() {
Expand All @@ -104,9 +174,77 @@ fn is_script_tag_start(line: &str) -> bool {
line == "# /// script"
}

/// Returns `true` if the line appears to start a script tag.
///
/// See: <https://peps.python.org/pep-0723/>
fn is_script_tag_end(line: &str) -> bool {
line == "# ///"
#[cfg(test)]
mod tests {
use crate::rules::eradicate::rules::commented_out_code::skip_script_comments;
use ruff_python_parser::parse_module;
use ruff_python_trivia::CommentRanges;
use ruff_source_file::Locator;
use ruff_text_size::TextSize;
#[test]
fn script_comment() {
let code = r#"
# /// script
# requires-python = ">=3.11"
# dependencies = [
# "requests<3",
# "rich",
# ]
# ///

a = 10 # abc
"#;

let parsed = parse_module(code).unwrap();
let locator = Locator::new(code);

let comments = CommentRanges::from(parsed.tokens());
let mut comments = comments.into_iter().peekable();

let script_start = code.find("# /// script").unwrap();
let script_start_range = locator.full_line_range(TextSize::try_from(script_start).unwrap());

let valid = skip_script_comments(script_start_range, &mut comments, &Locator::new(code));

assert!(valid);

let next_comment = comments.next();

assert!(next_comment.is_some());
assert_eq!(&code[next_comment.unwrap()], "# abc");
}

#[test]
fn script_comment_end_precedence() {
let code = r#"
# /// script
# [tool.uv]
# extra-index-url = ["https://pypi.org/simple", """\
# https://example.com/
# ///
# """
# ]
# ///

a = 10 # abc
"#;

let parsed = parse_module(code).unwrap();
let locator = Locator::new(code);

let comments = CommentRanges::from(parsed.tokens());
let mut comments = comments.into_iter().peekable();

let script_start = code.find("# /// script").unwrap();
let script_start_range = locator.full_line_range(TextSize::try_from(script_start).unwrap());

let valid = skip_script_comments(script_start_range, &mut comments, &Locator::new(code));

assert!(valid);

let next_comment = comments.next();

assert!(next_comment.is_some());
assert_eq!(&code[next_comment.unwrap()], "# abc");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -321,3 +321,38 @@ ERA001.py:47:1: ERA001 Found commented-out code
48 47 | # ///
49 48 |
50 49 | # Script tag (OK)

ERA001.py:75:1: ERA001 Found commented-out code
|
73 | # /// script
74 | # requires-python = ">=3.11"
75 | # dependencies = [
| ^^^^^^^^^^^^^^^^^^ ERA001
76 | # "requests<3",
77 | # "rich",
|
= help: Remove commented-out code

ℹ Display-only fix
72 72 |
73 73 | # /// script
74 74 | # requires-python = ">=3.11"
75 |-# dependencies = [
76 75 | # "requests<3",
77 76 | # "rich",
78 77 | # ]

ERA001.py:78:1: ERA001 Found commented-out code
|
76 | # "requests<3",
77 | # "rich",
78 | # ]
| ^^^ ERA001
|
= help: Remove commented-out code

ℹ Display-only fix
75 75 | # dependencies = [
76 76 | # "requests<3",
77 77 | # "rich",
78 |-# ]
Loading