From ac8b4536d3958e511e18b18869f0b53f94141e5f Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sun, 8 Sep 2024 18:12:13 +0200 Subject: [PATCH] ERA001: Fix skipping over script-comments with multiple end-tags --- .../test/fixtures/eradicate/ERA001.py | 13 +- .../eradicate/rules/commented_out_code.rs | 190 +++++++++++++++--- ...s__eradicate__tests__ERA001_ERA001.py.snap | 35 ++++ 3 files changed, 211 insertions(+), 27 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/eradicate/ERA001.py b/crates/ruff_linter/resources/test/fixtures/eradicate/ERA001.py index e64ea4e409a76..08a211d2d569b 100644 --- a/crates/ruff_linter/resources/test/fixtures/eradicate/ERA001.py +++ b/crates/ruff_linter/resources/test/fixtures/eradicate/ERA001.py @@ -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" diff --git a/crates/ruff_linter/src/rules/eradicate/rules/commented_out_code.rs b/crates/ruff_linter/src/rules/eradicate/rules/commented_out_code.rs index fc0b0c1c99606..b87dd9b8bfa82 100644 --- a/crates/ruff_linter/src/rules/eradicate/rules/commented_out_code.rs +++ b/crates/ruff_linter/src/rules/eradicate/rules/commented_out_code.rs @@ -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; @@ -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. @@ -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( + script_start: TextRange, + comments: &mut std::iter::Peekable, + locator: &Locator, +) -> bool +where + I: Iterator, +{ + 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 = """ + // > # /// + // > # /// text + // > # /// + // > # /// + // > # 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() { @@ -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: -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"); + } } diff --git a/crates/ruff_linter/src/rules/eradicate/snapshots/ruff_linter__rules__eradicate__tests__ERA001_ERA001.py.snap b/crates/ruff_linter/src/rules/eradicate/snapshots/ruff_linter__rules__eradicate__tests__ERA001_ERA001.py.snap index 7dc46d961fda6..826b94dffbfa6 100644 --- a/crates/ruff_linter/src/rules/eradicate/snapshots/ruff_linter__rules__eradicate__tests__ERA001_ERA001.py.snap +++ b/crates/ruff_linter/src/rules/eradicate/snapshots/ruff_linter__rules__eradicate__tests__ERA001_ERA001.py.snap @@ -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 |-# ]