From aa8cd5f6b097058843e32c2cd77153cf3314860f Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Tue, 19 Sep 2023 16:29:51 +0100 Subject: [PATCH 1/6] Allow todos --- crates/ruff/src/rules/eradicate/detection.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/crates/ruff/src/rules/eradicate/detection.rs b/crates/ruff/src/rules/eradicate/detection.rs index 77be87c048f1b..a4834d38921dd 100644 --- a/crates/ruff/src/rules/eradicate/detection.rs +++ b/crates/ruff/src/rules/eradicate/detection.rs @@ -6,7 +6,7 @@ use ruff_python_parser::parse_suite; static ALLOWLIST_REGEX: Lazy = Lazy::new(|| { Regex::new( - r"^(?i)(?:pylint|pyright|noqa|nosec|region|endregion|type:\s*ignore|fmt:\s*(on|off)|isort:\s*(on|off|skip|skip_file|split|dont-add-imports(:\s*\[.*?])?)|mypy:|SPDX-License-Identifier:)" + r"^(?i)(?:pylint|pyright|noqa|nosec|region|endregion|type:\s*ignore|fmt:\s*(on|off)|isort:\s*(on|off|skip|skip_file|split|dont-add-imports(:\s*\[.*?])?)|mypy:|SPDX-License-Identifier:|(?i)TODO(?:\([^)]*\))?:)" ).unwrap() }); static BRACKET_REGEX: Lazy = Lazy::new(|| Regex::new(r"^[()\[\]{}\s]+$").unwrap()); @@ -279,4 +279,19 @@ mod tests { &["XXX".to_string()] )); } + + #[test] + fn comment_contains_todo() { + assert!(comment_contains_code("# TODO(tom)", &[])); + assert!(comment_contains_code("# todo(tom)", &[])); + assert!(comment_contains_code("# TODO()", &[])); + assert!(comment_contains_code("# todo()", &[])); + + assert!(!comment_contains_code("# TODO(tom): Something", &[])); + assert!(!comment_contains_code("# todo(tom): Something", &[])); + assert!(!comment_contains_code("# TODO: Something", &[])); + assert!(!comment_contains_code("# todo: Something", &[])); + assert!(!comment_contains_code("# TODO(): Something", &[])); + assert!(!comment_contains_code("# todo(): Something", &[])); + } } From 0818e772e0acb0cf777ae0e0cc29f19d5fb13522 Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Tue, 19 Sep 2023 16:38:51 +0100 Subject: [PATCH 2/6] Add known problems --- .../ruff/src/rules/eradicate/rules/commented_out_code.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/ruff/src/rules/eradicate/rules/commented_out_code.rs b/crates/ruff/src/rules/eradicate/rules/commented_out_code.rs index 69e451fa557f7..6c0bae114e374 100644 --- a/crates/ruff/src/rules/eradicate/rules/commented_out_code.rs +++ b/crates/ruff/src/rules/eradicate/rules/commented_out_code.rs @@ -15,13 +15,19 @@ use super::super::detection::comment_contains_code; /// Commented-out code is dead code, and is often included inadvertently. /// It should be removed. /// +/// ## Known problems +/// Prone to false positives when checking comments that resemble Python code, +/// but are not actually Python code ([#4845]). +/// /// ## Example /// ```python -/// # print('foo') +/// # print("Hello, world!") /// ``` /// /// ## Options /// - `task-tags` +/// +/// [#4845]: https://github.com/astral-sh/ruff/issues/4845 #[violation] pub struct CommentedOutCode; From 8bb33769de2dc41c4fe7a52954af26a4c73c2a90 Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Tue, 19 Sep 2023 17:01:52 +0100 Subject: [PATCH 3/6] Extend to allow @author TODO tags --- crates/ruff/src/rules/eradicate/detection.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/ruff/src/rules/eradicate/detection.rs b/crates/ruff/src/rules/eradicate/detection.rs index a4834d38921dd..a836aacb29523 100644 --- a/crates/ruff/src/rules/eradicate/detection.rs +++ b/crates/ruff/src/rules/eradicate/detection.rs @@ -6,7 +6,7 @@ use ruff_python_parser::parse_suite; static ALLOWLIST_REGEX: Lazy = Lazy::new(|| { Regex::new( - r"^(?i)(?:pylint|pyright|noqa|nosec|region|endregion|type:\s*ignore|fmt:\s*(on|off)|isort:\s*(on|off|skip|skip_file|split|dont-add-imports(:\s*\[.*?])?)|mypy:|SPDX-License-Identifier:|(?i)TODO(?:\([^)]*\))?:)" + r"^(?i)(?:pylint|pyright|noqa|nosec|region|endregion|type:\s*ignore|fmt:\s*(on|off)|isort:\s*(on|off|skip|skip_file|split|dont-add-imports(:\s*\[.*?])?)|mypy:|SPDX-License-Identifier:|(?i)TODO(\([^\)]*\)| @\w+)?:)", ).unwrap() }); static BRACKET_REGEX: Lazy = Lazy::new(|| Regex::new(r"^[()\[\]{}\s]+$").unwrap()); @@ -293,5 +293,6 @@ mod tests { assert!(!comment_contains_code("# todo: Something", &[])); assert!(!comment_contains_code("# TODO(): Something", &[])); assert!(!comment_contains_code("# todo(): Something", &[])); + assert!(!comment_contains_code("# TODO @tom: Something", &[])); } } From 09669265a609e30e987edd113ebaed3ba44681aa Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Thu, 21 Sep 2023 07:53:13 +0100 Subject: [PATCH 4/6] Check for tags --- crates/ruff/src/rules/eradicate/detection.rs | 112 +++++++++++++++---- 1 file changed, 93 insertions(+), 19 deletions(-) diff --git a/crates/ruff/src/rules/eradicate/detection.rs b/crates/ruff/src/rules/eradicate/detection.rs index a836aacb29523..b7b78820deb15 100644 --- a/crates/ruff/src/rules/eradicate/detection.rs +++ b/crates/ruff/src/rules/eradicate/detection.rs @@ -6,7 +6,7 @@ use ruff_python_parser::parse_suite; static ALLOWLIST_REGEX: Lazy = Lazy::new(|| { Regex::new( - r"^(?i)(?:pylint|pyright|noqa|nosec|region|endregion|type:\s*ignore|fmt:\s*(on|off)|isort:\s*(on|off|skip|skip_file|split|dont-add-imports(:\s*\[.*?])?)|mypy:|SPDX-License-Identifier:|(?i)TODO(\([^\)]*\)| @\w+)?:)", + r"^(?i)(?:pylint|pyright|noqa|nosec|region|endregion|type:\s*ignore|fmt:\s*(on|off)|isort:\s*(on|off|skip|skip_file|split|dont-add-imports(:\s*\[.*?])?)|mypy:|SPDX-License-Identifier:)", ).unwrap() }); static BRACKET_REGEX: Lazy = Lazy::new(|| Regex::new(r"^[()\[\]{}\s]+$").unwrap()); @@ -39,6 +39,16 @@ pub(crate) fn comment_contains_code(line: &str, task_tags: &[String]) -> bool { return false; }; + // Ignore comments that are just task tags (e.g., "# TODO: Do something."). + if let Some(first) = line.split(&[' ', ':', '(']).next() { + if task_tags + .iter() + .any(|tag| tag == first.to_uppercase().as_str()) + { + return false; + } + } + // Ignore non-comment related hashes (e.g., "# Issue #999"). if HASH_NUMBER.is_match(line) { return false; @@ -49,12 +59,6 @@ pub(crate) fn comment_contains_code(line: &str, task_tags: &[String]) -> bool { return false; } - if let Some(first) = line.split(&[' ', ':']).next() { - if task_tags.iter().any(|tag| tag == first) { - return false; - } - } - if CODING_COMMENT_REGEX.is_match(line) { return false; } @@ -102,6 +106,7 @@ fn multiline_case(line: &str) -> bool { #[cfg(test)] mod tests { use super::comment_contains_code; + use crate::settings::defaults::TASK_TAGS; #[test] fn comment_contains_code_basic() { @@ -282,17 +287,86 @@ mod tests { #[test] fn comment_contains_todo() { - assert!(comment_contains_code("# TODO(tom)", &[])); - assert!(comment_contains_code("# todo(tom)", &[])); - assert!(comment_contains_code("# TODO()", &[])); - assert!(comment_contains_code("# todo()", &[])); - - assert!(!comment_contains_code("# TODO(tom): Something", &[])); - assert!(!comment_contains_code("# todo(tom): Something", &[])); - assert!(!comment_contains_code("# TODO: Something", &[])); - assert!(!comment_contains_code("# todo: Something", &[])); - assert!(!comment_contains_code("# TODO(): Something", &[])); - assert!(!comment_contains_code("# todo(): Something", &[])); - assert!(!comment_contains_code("# TODO @tom: Something", &[])); + // Test with default TASK_TAGS. + let binding = TASK_TAGS + .iter() + .map(ToString::to_string) + .collect::>(); + let default_task_tags: &[String] = &binding; + + assert!(!comment_contains_code( + "# TODO(tom): Rewrite in Rust", + default_task_tags + )); + assert!(!comment_contains_code( + "# TODO: Rewrite in Rust", + default_task_tags + )); + assert!(!comment_contains_code( + "# TODO:Rewrite in Rust", + default_task_tags + )); + assert!(!comment_contains_code( + "# todo(tom): Rewrite in Rust", + default_task_tags + )); + assert!(!comment_contains_code( + "# todo: Rewrite in Rust", + default_task_tags + )); + assert!(!comment_contains_code( + "# todo:Rewrite in Rust", + default_task_tags + )); + + assert!(!comment_contains_code( + "# FIXME(tom): Rewrite in Rust", + default_task_tags + )); + assert!(!comment_contains_code( + "# FIXME: Rewrite in Rust", + default_task_tags + )); + assert!(!comment_contains_code( + "# FIXME:Rewrite in Rust", + default_task_tags + )); + assert!(!comment_contains_code( + "# fixme(tom): Rewrite in Rust", + default_task_tags + )); + assert!(!comment_contains_code( + "# fixme: Rewrite in Rust", + default_task_tags + )); + assert!(!comment_contains_code( + "# fixme:Rewrite in Rust", + default_task_tags + )); + + assert!(!comment_contains_code( + "# XXX(tom): Rewrite in Rust", + default_task_tags + )); + assert!(!comment_contains_code( + "# XXX: Rewrite in Rust", + default_task_tags + )); + assert!(!comment_contains_code( + "# XXX:Rewrite in Rust", + default_task_tags + )); + assert!(!comment_contains_code( + "# xxx(tom): Rewrite in Rust", + default_task_tags + )); + assert!(!comment_contains_code( + "# xxx: Rewrite in Rust", + default_task_tags + )); + assert!(!comment_contains_code( + "# xxx:Rewrite in Rust", + default_task_tags + )); } } From a35c979cabf5de9b86b874643929282a6e1c6a16 Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Thu, 21 Sep 2023 08:04:26 +0100 Subject: [PATCH 5/6] Improve comment and deal with conflicts --- crates/ruff_linter/src/rules/eradicate/detection.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/eradicate/detection.rs b/crates/ruff_linter/src/rules/eradicate/detection.rs index b7b78820deb15..db9b7e1615277 100644 --- a/crates/ruff_linter/src/rules/eradicate/detection.rs +++ b/crates/ruff_linter/src/rules/eradicate/detection.rs @@ -39,7 +39,7 @@ pub(crate) fn comment_contains_code(line: &str, task_tags: &[String]) -> bool { return false; }; - // Ignore comments that are just task tags (e.g., "# TODO: Do something."). + // Ignore task tag comments (e.g., "# TODO(tom): Refactor foo"). if let Some(first) = line.split(&[' ', ':', '(']).next() { if task_tags .iter() @@ -106,7 +106,7 @@ fn multiline_case(line: &str) -> bool { #[cfg(test)] mod tests { use super::comment_contains_code; - use crate::settings::defaults::TASK_TAGS; + use crate::settings::TASK_TAGS; #[test] fn comment_contains_code_basic() { From c2fd0d483df54a168bccfee4d79a3f5aecce2c53 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 28 Sep 2023 19:04:01 -0400 Subject: [PATCH 6/6] Remove to_uppercase --- .../src/rules/eradicate/detection.rs | 82 ++++--------------- 1 file changed, 17 insertions(+), 65 deletions(-) diff --git a/crates/ruff_linter/src/rules/eradicate/detection.rs b/crates/ruff_linter/src/rules/eradicate/detection.rs index db9b7e1615277..ac9c6e0e4cddd 100644 --- a/crates/ruff_linter/src/rules/eradicate/detection.rs +++ b/crates/ruff_linter/src/rules/eradicate/detection.rs @@ -39,14 +39,13 @@ pub(crate) fn comment_contains_code(line: &str, task_tags: &[String]) -> bool { return false; }; - // Ignore task tag comments (e.g., "# TODO(tom): Refactor foo"). - if let Some(first) = line.split(&[' ', ':', '(']).next() { - if task_tags - .iter() - .any(|tag| tag == first.to_uppercase().as_str()) - { - return false; - } + // Ignore task tag comments (e.g., "# TODO(tom): Refactor"). + if line + .split(&[' ', ':', '(']) + .next() + .is_some_and(|first| task_tags.iter().any(|tag| tag == first)) + { + return false; } // Ignore non-comment related hashes (e.g., "# Issue #999"). @@ -287,86 +286,39 @@ mod tests { #[test] fn comment_contains_todo() { - // Test with default TASK_TAGS. - let binding = TASK_TAGS + let task_tags = TASK_TAGS .iter() .map(ToString::to_string) .collect::>(); - let default_task_tags: &[String] = &binding; assert!(!comment_contains_code( "# TODO(tom): Rewrite in Rust", - default_task_tags + &task_tags )); assert!(!comment_contains_code( "# TODO: Rewrite in Rust", - default_task_tags - )); - assert!(!comment_contains_code( - "# TODO:Rewrite in Rust", - default_task_tags - )); - assert!(!comment_contains_code( - "# todo(tom): Rewrite in Rust", - default_task_tags - )); - assert!(!comment_contains_code( - "# todo: Rewrite in Rust", - default_task_tags - )); - assert!(!comment_contains_code( - "# todo:Rewrite in Rust", - default_task_tags + &task_tags )); + assert!(!comment_contains_code("# TODO:Rewrite in Rust", &task_tags)); assert!(!comment_contains_code( "# FIXME(tom): Rewrite in Rust", - default_task_tags + &task_tags )); assert!(!comment_contains_code( "# FIXME: Rewrite in Rust", - default_task_tags + &task_tags )); assert!(!comment_contains_code( "# FIXME:Rewrite in Rust", - default_task_tags - )); - assert!(!comment_contains_code( - "# fixme(tom): Rewrite in Rust", - default_task_tags - )); - assert!(!comment_contains_code( - "# fixme: Rewrite in Rust", - default_task_tags - )); - assert!(!comment_contains_code( - "# fixme:Rewrite in Rust", - default_task_tags + &task_tags )); assert!(!comment_contains_code( "# XXX(tom): Rewrite in Rust", - default_task_tags - )); - assert!(!comment_contains_code( - "# XXX: Rewrite in Rust", - default_task_tags - )); - assert!(!comment_contains_code( - "# XXX:Rewrite in Rust", - default_task_tags - )); - assert!(!comment_contains_code( - "# xxx(tom): Rewrite in Rust", - default_task_tags - )); - assert!(!comment_contains_code( - "# xxx: Rewrite in Rust", - default_task_tags - )); - assert!(!comment_contains_code( - "# xxx:Rewrite in Rust", - default_task_tags + &task_tags )); + assert!(!comment_contains_code("# XXX: Rewrite in Rust", &task_tags)); + assert!(!comment_contains_code("# XXX:Rewrite in Rust", &task_tags)); } }