From ddd5d6d5c4aa055e534a4b64840695cba06c8a8c Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 3 Jan 2024 21:43:21 -0500 Subject: [PATCH] Add cell indexes to all diagnostics --- .../test/fixtures/pyflakes/F402.ipynb | 60 +++++++++++++++++++ .../checkers/ast/analyze/deferred_scopes.rs | 9 +-- crates/ruff_linter/src/checkers/ast/mod.rs | 31 +++++++++- crates/ruff_linter/src/linter.rs | 2 + crates/ruff_linter/src/rules/pyflakes/mod.rs | 1 + .../src/rules/pyflakes/rules/imports.rs | 8 +-- .../pyflakes/rules/redefined_while_unused.rs | 8 +-- ...les__pyflakes__tests__F402_F402.ipynb.snap | 23 +++++++ .../rules/load_before_global_declaration.rs | 12 ++-- crates/ruff_source_file/src/lib.rs | 19 +++++- 10 files changed, 148 insertions(+), 25 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/pyflakes/F402.ipynb create mode 100644 crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F402_F402.ipynb.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pyflakes/F402.ipynb b/crates/ruff_linter/resources/test/fixtures/pyflakes/F402.ipynb new file mode 100644 index 00000000000000..04aaa8d8c498aa --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pyflakes/F402.ipynb @@ -0,0 +1,60 @@ +{ + "cells": [ + { + "cell_type": "code", + "execution_count": null, + "id": "33faf7ad-a3fd-4ac4-a0c3-52e507ed49df", + "metadata": {}, + "outputs": [], + "source": [ + "import os\n", + "import os.path as path" + ] + }, + { + "cell_type": "code", + "execution_count": null, + "id": "481fb4bf-c1b9-47da-927f-3cfdfe4b49ec", + "metadata": {}, + "outputs": [], + "source": [ + "for os in range(3):\n", + " pass" + ] + }, + { + "cell_type": "code", + "execution_count": null, + "outputs": [], + "source": [ + "for path in range(3):\n", + " pass" + ], + "metadata": { + "collapsed": false + }, + "id": "2f0c65a5-0a0e-4080-afce-5a8ed0d706df" + } + ], + "metadata": { + "kernelspec": { + "display_name": "Python (ruff-playground)", + "language": "python", + "name": "ruff-playground" + }, + "language_info": { + "codemirror_mode": { + "name": "ipython", + "version": 3 + }, + "file_extension": ".py", + "mimetype": "text/x-python", + "name": "python", + "nbconvert_exporter": "python", + "pygments_lexer": "ipython3", + "version": "3.11.3" + } + }, + "nbformat": 4, + "nbformat_minor": 5 +} diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs index fad4926774219b..4c7a6bd999cd0b 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs @@ -151,13 +151,10 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { continue; } - #[allow(deprecated)] - let line = checker.locator.compute_line_index(shadowed.start()); - checker.diagnostics.push(Diagnostic::new( pyflakes::rules::ImportShadowedByLoopVar { name: name.to_string(), - line, + row: checker.compute_source_row(shadowed.start()), }, binding.range(), )); @@ -243,12 +240,10 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { continue; } - #[allow(deprecated)] - let line = checker.locator.compute_line_index(shadowed.start()); let mut diagnostic = Diagnostic::new( pyflakes::rules::RedefinedWhileUnused { name: (*name).to_string(), - line, + row: checker.compute_source_row(shadowed.start()), }, binding.range(), ); diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index 93396178d488d9..18e8f2f31d2370 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -37,7 +37,7 @@ use ruff_python_ast::{ use ruff_text_size::{Ranged, TextRange, TextSize}; use ruff_diagnostics::{Diagnostic, IsolationLevel}; -use ruff_notebook::CellOffsets; +use ruff_notebook::{CellOffsets, NotebookIndex}; use ruff_python_ast::all::{extract_all_names, DunderAllFlags}; use ruff_python_ast::helpers::{ collect_import_from_member, extract_handled_exceptions, to_module_path, @@ -56,7 +56,7 @@ use ruff_python_semantic::{ StarImport, SubmoduleImport, }; use ruff_python_stdlib::builtins::{IPYTHON_BUILTINS, MAGIC_GLOBALS, PYTHON_BUILTINS}; -use ruff_source_file::Locator; +use ruff_source_file::{Locator, OneIndexed, SourceRow}; use crate::checkers::ast::annotation::AnnotationContext; use crate::checkers::ast::deferred::Deferred; @@ -83,6 +83,8 @@ pub(crate) struct Checker<'a> { pub(crate) source_type: PySourceType, /// The [`CellOffsets`] for the current file, if it's a Jupyter notebook. cell_offsets: Option<&'a CellOffsets>, + /// The [`NotebookIndex`] for the current file, if it's a Jupyter notebook. + notebook_index: Option<&'a NotebookIndex>, /// The [`flags::Noqa`] for the current analysis (i.e., whether to respect suppression /// comments). noqa: flags::Noqa, @@ -128,6 +130,7 @@ impl<'a> Checker<'a> { importer: Importer<'a>, source_type: PySourceType, cell_offsets: Option<&'a CellOffsets>, + notebook_index: Option<&'a NotebookIndex>, ) -> Checker<'a> { Checker { settings, @@ -146,6 +149,7 @@ impl<'a> Checker<'a> { diagnostics: Vec::default(), flake8_bugbear_seen: Vec::default(), cell_offsets, + notebook_index, last_stmt_end: TextSize::default(), } } @@ -198,6 +202,22 @@ impl<'a> Checker<'a> { } } + /// Returns the [`SourceRow`] for the given offset. + pub(crate) fn compute_source_row(&self, offset: TextSize) -> SourceRow { + #[allow(deprecated)] + let line_index = self.locator.compute_line_index(offset); + + if let Some(notebook_index) = self.notebook_index { + let cell = notebook_index.cell(line_index).unwrap_or(OneIndexed::MIN); + let row = notebook_index + .cell_row(line_index) + .unwrap_or(OneIndexed::MIN); + SourceRow::Notebook(cell, row) + } else { + SourceRow::SourceFile(line_index) + } + } + /// The [`Locator`] for the current file, which enables extraction of source code from byte /// offsets. pub(crate) const fn locator(&self) -> &'a Locator<'a> { @@ -240,6 +260,11 @@ impl<'a> Checker<'a> { self.cell_offsets } + /// The [`NotebookIndex`] for the current file, if it's a Jupyter notebook. + pub(crate) const fn notebook_index(&self) -> Option<&'a NotebookIndex> { + self.notebook_index + } + /// Returns whether the given rule should be checked. #[inline] pub(crate) const fn enabled(&self, rule: Rule) -> bool { @@ -1984,6 +2009,7 @@ pub(crate) fn check_ast( package: Option<&Path>, source_type: PySourceType, cell_offsets: Option<&CellOffsets>, + notebook_index: Option<&NotebookIndex>, ) -> Vec { let module_path = package.and_then(|package| to_module_path(package, path)); let module = Module { @@ -2013,6 +2039,7 @@ pub(crate) fn check_ast( Importer::new(python_ast, locator, stylist), source_type, cell_offsets, + notebook_index, ); checker.bind_builtins(); diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index 9ce637f3c48359..a81f8f89f08312 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -148,6 +148,7 @@ pub fn check_path( match tokens.into_ast_source(source_kind, source_type) { Ok(python_ast) => { let cell_offsets = source_kind.as_ipy_notebook().map(Notebook::cell_offsets); + let notebook_index = source_kind.as_ipy_notebook().map(Notebook::index); if use_ast { diagnostics.extend(check_ast( &python_ast, @@ -161,6 +162,7 @@ pub fn check_path( package, source_type, cell_offsets, + notebook_index, )); } if use_imports { diff --git a/crates/ruff_linter/src/rules/pyflakes/mod.rs b/crates/ruff_linter/src/rules/pyflakes/mod.rs index 3a19dba6ca7cb6..9eb50868e6ad95 100644 --- a/crates/ruff_linter/src/rules/pyflakes/mod.rs +++ b/crates/ruff_linter/src/rules/pyflakes/mod.rs @@ -54,6 +54,7 @@ mod tests { #[test_case(Rule::UnusedImport, Path::new("F401_19.py"))] #[test_case(Rule::UnusedImport, Path::new("F401_20.py"))] #[test_case(Rule::ImportShadowedByLoopVar, Path::new("F402.py"))] + #[test_case(Rule::ImportShadowedByLoopVar, Path::new("F402.ipynb"))] #[test_case(Rule::UndefinedLocalWithImportStar, Path::new("F403.py"))] #[test_case(Rule::LateFutureImport, Path::new("F404_0.py"))] #[test_case(Rule::LateFutureImport, Path::new("F404_1.py"))] diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/imports.rs b/crates/ruff_linter/src/rules/pyflakes/rules/imports.rs index 1170b4e1148648..1d570e3af8665f 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/imports.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/imports.rs @@ -1,6 +1,6 @@ use ruff_diagnostics::Violation; use ruff_macros::{derive_message_formats, violation}; -use ruff_source_file::OneIndexed; +use ruff_source_file::{OneIndexed, SourceRow}; /// ## What it does /// Checks for import bindings that are shadowed by loop variables. @@ -32,14 +32,14 @@ use ruff_source_file::OneIndexed; #[violation] pub struct ImportShadowedByLoopVar { pub(crate) name: String, - pub(crate) line: OneIndexed, + pub(crate) row: SourceRow, } impl Violation for ImportShadowedByLoopVar { #[derive_message_formats] fn message(&self) -> String { - let ImportShadowedByLoopVar { name, line } = self; - format!("Import `{name}` from line {line} shadowed by loop variable") + let ImportShadowedByLoopVar { name, row } = self; + format!("Import `{name}` from {row} shadowed by loop variable") } } diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs b/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs index f8af8854abec84..e0e08cd0d36229 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs @@ -1,6 +1,6 @@ use ruff_diagnostics::Violation; use ruff_macros::{derive_message_formats, violation}; -use ruff_source_file::OneIndexed; +use ruff_source_file::{OneIndexed, SourceRow}; /// ## What it does /// Checks for variable definitions that redefine (or "shadow") unused @@ -25,13 +25,13 @@ use ruff_source_file::OneIndexed; #[violation] pub struct RedefinedWhileUnused { pub name: String, - pub line: OneIndexed, + pub row: SourceRow, } impl Violation for RedefinedWhileUnused { #[derive_message_formats] fn message(&self) -> String { - let RedefinedWhileUnused { name, line } = self; - format!("Redefinition of unused `{name}` from line {line}") + let RedefinedWhileUnused { name, row } = self; + format!("Redefinition of unused `{name}` from {row}") } } diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F402_F402.ipynb.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F402_F402.ipynb.snap new file mode 100644 index 00000000000000..cbb1ce305a800c --- /dev/null +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F402_F402.ipynb.snap @@ -0,0 +1,23 @@ +--- +source: crates/ruff_linter/src/rules/pyflakes/mod.rs +--- +F402.ipynb:3:5: F402 Import `os` from cell 1, line 1 shadowed by loop variable + | +1 | import os +2 | import os.path as path +3 | for os in range(3): + | ^^ F402 +4 | pass +5 | for path in range(3): + | + +F402.ipynb:5:5: F402 Import `path` from cell 1, line 2 shadowed by loop variable + | +3 | for os in range(3): +4 | pass +5 | for path in range(3): + | ^^^^ F402 +6 | pass + | + + diff --git a/crates/ruff_linter/src/rules/pylint/rules/load_before_global_declaration.rs b/crates/ruff_linter/src/rules/pylint/rules/load_before_global_declaration.rs index da980e90ec8c4d..a3befdc0d42dfc 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/load_before_global_declaration.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/load_before_global_declaration.rs @@ -2,7 +2,7 @@ use ruff_python_ast::Expr; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_source_file::OneIndexed; +use ruff_source_file::{OneIndexed, SourceRow}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -43,14 +43,14 @@ use crate::checkers::ast::Checker; #[violation] pub struct LoadBeforeGlobalDeclaration { name: String, - line: OneIndexed, + row: SourceRow, } impl Violation for LoadBeforeGlobalDeclaration { #[derive_message_formats] fn message(&self) -> String { - let LoadBeforeGlobalDeclaration { name, line } = self; - format!("Name `{name}` is used prior to global declaration on line {line}") + let LoadBeforeGlobalDeclaration { name, row } = self; + format!("Name `{name}` is used prior to global declaration on {row}") } } @@ -58,12 +58,10 @@ impl Violation for LoadBeforeGlobalDeclaration { pub(crate) fn load_before_global_declaration(checker: &mut Checker, name: &str, expr: &Expr) { if let Some(stmt) = checker.semantic().global(name) { if expr.start() < stmt.start() { - #[allow(deprecated)] - let location = checker.locator().compute_source_location(stmt.start()); checker.diagnostics.push(Diagnostic::new( LoadBeforeGlobalDeclaration { name: name.to_string(), - line: location.row, + row: checker.compute_source_row(stmt.start()), }, expr.range(), )); diff --git a/crates/ruff_source_file/src/lib.rs b/crates/ruff_source_file/src/lib.rs index 457158bb8ba07d..15f15f051ce21c 100644 --- a/crates/ruff_source_file/src/lib.rs +++ b/crates/ruff_source_file/src/lib.rs @@ -1,5 +1,5 @@ use std::cmp::Ordering; -use std::fmt::{Debug, Formatter}; +use std::fmt::{Debug, Display, Formatter}; use std::sync::Arc; #[cfg(feature = "serde")] @@ -253,3 +253,20 @@ impl Debug for SourceLocation { .finish() } } + +#[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] +pub enum SourceRow { + /// A row within a cell in a Jupyter Notebook. + Notebook(OneIndexed, OneIndexed), + /// A row within a source file. + SourceFile(OneIndexed), +} + +impl Display for SourceRow { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + match self { + SourceRow::Notebook(cell, line) => write!(f, "cell {cell}, line {line}"), + SourceRow::SourceFile(line) => write!(f, "line {line}"), + } + } +}