From de9ceb2fe15cbcadfe0c74c36fc7f1cfdd112e7a Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 22 Sep 2022 13:09:02 -0400 Subject: [PATCH] Only enforce multi-line noqa directives for strings (#258) --- resources/test/fixtures/M001.py | 7 --- src/check_lines.rs | 63 +++----------------- src/linter.rs | 2 +- src/noqa.rs | 57 ++++++++++-------- src/snapshots/ruff__linter__tests__m001.snap | 6 -- 5 files changed, 43 insertions(+), 92 deletions(-) diff --git a/resources/test/fixtures/M001.py b/resources/test/fixtures/M001.py index b213adff27..e9e1609bc8 100644 --- a/resources/test/fixtures/M001.py +++ b/resources/test/fixtures/M001.py @@ -29,10 +29,3 @@ _ = """Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. """ # noqa: E501, F841 - -_ = """Lorem ipsum dolor sit amet. - - https://github.com/PyCQA/pycodestyle/pull/258/files#diff-841c622497a8033d10152bfdfb15b20b92437ecdea21a260944ea86b77b51533 - -Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. # noqa: E501 -""" # noqa: E501 diff --git a/src/check_lines.rs b/src/check_lines.rs index a90f945722..2c3d875c1b 100644 --- a/src/check_lines.rs +++ b/src/check_lines.rs @@ -39,8 +39,6 @@ pub fn check_lines( let lines: Vec<&str> = contents.lines().collect(); for (lineno, line) in lines.iter().enumerate() { - let mut did_insert: bool = false; - // Grab the noqa (logical) line number for the current (physical) line. // If there are newlines at the end of the file, they won't be represented in // `noqa_line_for`, so fallback to the current line. @@ -49,45 +47,19 @@ pub fn check_lines( .map(|lineno| lineno - 1) .unwrap_or(lineno); - if enforce_noqa && !did_insert { - // Try the current physical line. + if enforce_noqa { noqa_directives - .entry(lineno) - .or_insert_with(|| (noqa::extract_noqa_directive(lines[lineno]), vec![])); - // Try the current logical line. - if lineno != noqa_lineno { - noqa_directives - .entry(noqa_lineno) - .or_insert_with(|| (noqa::extract_noqa_directive(lines[noqa_lineno]), vec![])); - } - did_insert = true; + .entry(noqa_lineno) + .or_insert_with(|| (noqa::extract_noqa_directive(lines[noqa_lineno]), vec![])); } // Remove any ignored checks. // TODO(charlie): Only validate checks for the current line. for (index, check) in checks.iter().enumerate() { if check.location.row() == lineno + 1 { - if !did_insert { - // Try the current physical line. - noqa_directives - .entry(lineno) - .or_insert_with(|| (noqa::extract_noqa_directive(lines[lineno]), vec![])); - // Try the current logical line. - if lineno != noqa_lineno { - noqa_directives.entry(noqa_lineno).or_insert_with(|| { - (noqa::extract_noqa_directive(lines[noqa_lineno]), vec![]) - }); - } - did_insert = true; - } - - let noqa = if lineno != noqa_lineno - && matches!(noqa_directives.get(&lineno).unwrap(), (Directive::None, _)) - { - noqa_directives.get_mut(&noqa_lineno).unwrap() - } else { - noqa_directives.get_mut(&lineno).unwrap() - }; + let noqa = noqa_directives + .entry(noqa_lineno) + .or_insert_with(|| (noqa::extract_noqa_directive(lines[noqa_lineno]), vec![])); match noqa { (Directive::All(_), matches) => { @@ -109,26 +81,9 @@ pub fn check_lines( if enforce_line_too_long { let line_length = line.chars().count(); if should_enforce_line_length(line, line_length, settings.line_length) { - if !did_insert { - // Try the current physical line. - noqa_directives - .entry(lineno) - .or_insert_with(|| (noqa::extract_noqa_directive(lines[lineno]), vec![])); - // Try the current logical line. - if lineno != noqa_lineno { - noqa_directives.entry(noqa_lineno).or_insert_with(|| { - (noqa::extract_noqa_directive(lines[noqa_lineno]), vec![]) - }); - } - } - - let noqa = if lineno != noqa_lineno - && matches!(noqa_directives.get(&lineno).unwrap(), (Directive::None, _)) - { - noqa_directives.get_mut(&noqa_lineno).unwrap() - } else { - noqa_directives.get_mut(&lineno).unwrap() - }; + let noqa = noqa_directives + .entry(noqa_lineno) + .or_insert_with(|| (noqa::extract_noqa_directive(lines[noqa_lineno]), vec![])); let check = Check::new( CheckKind::LineTooLong(line_length, settings.line_length), diff --git a/src/linter.rs b/src/linter.rs index 1847159952..d6861ec41e 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -27,7 +27,7 @@ fn check_path( let lxr: Vec = lexer::make_tokenizer(contents).collect(); // Determine the noqa line for every line in the source. - let noqa_line_for = noqa::extract_line_map(&lxr); + let noqa_line_for = noqa::extract_noqa_line_for(&lxr); // Run the AST-based checks. if settings diff --git a/src/noqa.rs b/src/noqa.rs index 59046a1de7..1e0af4ae7d 100644 --- a/src/noqa.rs +++ b/src/noqa.rs @@ -37,9 +37,10 @@ pub fn extract_noqa_directive(line: &str) -> Directive { } } -pub fn extract_line_map(lxr: &[LexResult]) -> Vec { +pub fn extract_noqa_line_for(lxr: &[LexResult]) -> Vec { let mut line_map: Vec = vec![]; + let mut last_is_string = false; let mut last_seen = usize::MIN; let mut min_line = usize::MAX; let mut max_line = usize::MIN; @@ -53,7 +54,14 @@ pub fn extract_line_map(lxr: &[LexResult]) -> Vec { min_line = min(min_line, start.row()); max_line = max(max_line, start.row()); - line_map.extend(vec![max_line; (max_line + 1) - min_line]); + // For now, we only care about preserving noqa directives across multi-line strings. + if last_is_string { + line_map.extend(vec![max_line; (max_line + 1) - min_line]); + } else { + for i in (min_line - 1)..(max_line) { + line_map.push(i + 1); + } + } min_line = usize::MAX; max_line = usize::MIN; @@ -69,6 +77,7 @@ pub fn extract_line_map(lxr: &[LexResult]) -> Vec { max_line = max(max_line, end.row()); } last_seen = start.row(); + last_is_string = matches!(tok, Tok::String { .. }); } line_map @@ -80,7 +89,7 @@ mod tests { use rustpython_parser::lexer; use rustpython_parser::lexer::LexResult; - use crate::noqa::extract_line_map; + use crate::noqa::extract_noqa_line_for; #[test] fn line_map() -> Result<()> { @@ -90,50 +99,50 @@ y = 2 z = x + 1", ) .collect(); - println!("{:?}", extract_line_map(&lxr)); - assert_eq!(extract_line_map(&lxr), vec![1, 2, 3]); + println!("{:?}", extract_noqa_line_for(&lxr)); + assert_eq!(extract_noqa_line_for(&lxr), vec![1, 2, 3]); let lxr: Vec = lexer::make_tokenizer( " - x = 1 - y = 2 - z = x + 1", +x = 1 +y = 2 +z = x + 1", ) .collect(); - println!("{:?}", extract_line_map(&lxr)); - assert_eq!(extract_line_map(&lxr), vec![1, 2, 3, 4]); + println!("{:?}", extract_noqa_line_for(&lxr)); + assert_eq!(extract_noqa_line_for(&lxr), vec![1, 2, 3, 4]); let lxr: Vec = lexer::make_tokenizer( "x = 1 - y = 2 - z = x + 1 +y = 2 +z = x + 1 ", ) .collect(); - println!("{:?}", extract_line_map(&lxr)); - assert_eq!(extract_line_map(&lxr), vec![1, 2, 3]); + println!("{:?}", extract_noqa_line_for(&lxr)); + assert_eq!(extract_noqa_line_for(&lxr), vec![1, 2, 3]); let lxr: Vec = lexer::make_tokenizer( "x = 1 - y = 2 - z = x + 1 +y = 2 +z = x + 1 ", ) .collect(); - println!("{:?}", extract_line_map(&lxr)); - assert_eq!(extract_line_map(&lxr), vec![1, 2, 3, 4]); + println!("{:?}", extract_noqa_line_for(&lxr)); + assert_eq!(extract_noqa_line_for(&lxr), vec![1, 2, 3, 4]); let lxr: Vec = lexer::make_tokenizer( "x = '''abc - def - ghi - ''' - y = 2 - z = x + 1", +def +ghi +''' +y = 2 +z = x + 1", ) .collect(); - assert_eq!(extract_line_map(&lxr), vec![4, 4, 4, 4, 5, 6]); + assert_eq!(extract_noqa_line_for(&lxr), vec![4, 4, 4, 4, 5, 6]); Ok(()) } diff --git a/src/snapshots/ruff__linter__tests__m001.snap b/src/snapshots/ruff__linter__tests__m001.snap index 45e887677a..28dfd4e132 100644 --- a/src/snapshots/ruff__linter__tests__m001.snap +++ b/src/snapshots/ruff__linter__tests__m001.snap @@ -26,10 +26,4 @@ expression: checks row: 31 column: 6 fix: ~ -- kind: - UnusedNOQA: E501 - location: - row: 38 - column: 6 - fix: ~