Skip to content

Commit

Permalink
Add cell indexes to all diagnostics (#9387)
Browse files Browse the repository at this point in the history
## Summary

Ensures that any lint rules that include line locations render them as
relative to the cell (and include the cell number) when inside a Jupyter
notebook.

Closes #6672.

## Test Plan

`cargo test`
  • Loading branch information
charliermarsh authored Jan 4, 2024
1 parent f0d43da commit 328262b
Show file tree
Hide file tree
Showing 10 changed files with 141 additions and 25 deletions.
60 changes: 60 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pyflakes/F402.ipynb
Original file line number Diff line number Diff line change
@@ -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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
));
Expand Down Expand Up @@ -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(),
);
Expand Down
24 changes: 22 additions & 2 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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(),
}
}
Expand Down Expand Up @@ -198,6 +202,20 @@ 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 = self.locator.compute_line_index(offset);

if let Some(notebook_index) = self.notebook_index {
let cell = notebook_index.cell(line).unwrap_or(OneIndexed::MIN);
let line = notebook_index.cell_row(line).unwrap_or(OneIndexed::MIN);
SourceRow::Notebook { cell, line }
} else {
SourceRow::SourceFile { line }
}
}

/// The [`Locator`] for the current file, which enables extraction of source code from byte
/// offsets.
pub(crate) const fn locator(&self) -> &'a Locator<'a> {
Expand Down Expand Up @@ -1984,6 +2002,7 @@ pub(crate) fn check_ast(
package: Option<&Path>,
source_type: PySourceType,
cell_offsets: Option<&CellOffsets>,
notebook_index: Option<&NotebookIndex>,
) -> Vec<Diagnostic> {
let module_path = package.and_then(|package| to_module_path(package, path));
let module = Module {
Expand Down Expand Up @@ -2013,6 +2032,7 @@ pub(crate) fn check_ast(
Importer::new(python_ast, locator, stylist),
source_type,
cell_offsets,
notebook_index,
);
checker.bind_builtins();

Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/linter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -161,6 +162,7 @@ pub fn check_path(
package,
source_type,
cell_offsets,
notebook_index,
));
}
if use_imports {
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/pyflakes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"))]
Expand Down
8 changes: 4 additions & 4 deletions crates/ruff_linter/src/rules/pyflakes/rules/imports.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use ruff_diagnostics::Violation;
use ruff_macros::{derive_message_formats, violation};
use ruff_source_file::OneIndexed;
use ruff_source_file::SourceRow;

/// ## What it does
/// Checks for import bindings that are shadowed by loop variables.
Expand Down Expand Up @@ -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")
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use ruff_diagnostics::Violation;
use ruff_macros::{derive_message_formats, violation};
use ruff_source_file::OneIndexed;
use ruff_source_file::SourceRow;

/// ## What it does
/// Checks for variable definitions that redefine (or "shadow") unused
Expand All @@ -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}")
}
}
Original file line number Diff line number Diff line change
@@ -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
|


Original file line number Diff line number Diff line change
Expand Up @@ -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::SourceRow;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
Expand Down Expand Up @@ -43,27 +43,25 @@ 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}")
}
}

/// PLE0118
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(),
));
Expand Down
19 changes: 18 additions & 1 deletion crates/ruff_source_file/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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")]
Expand Down Expand Up @@ -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 { cell: OneIndexed, line: OneIndexed },
/// A row within a source file.
SourceFile { line: 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}"),
}
}
}

0 comments on commit 328262b

Please sign in to comment.