diff --git a/crates/ruff_linter/src/logging.rs b/crates/ruff_linter/src/logging.rs index 6851680cfb0a7..d65acd9ad58f3 100644 --- a/crates/ruff_linter/src/logging.rs +++ b/crates/ruff_linter/src/logging.rs @@ -177,18 +177,15 @@ impl Display for DisplayParseError<'_> { f, "cell {cell}{colon}", cell = jupyter_index - .cell(source_location.row.get()) - .unwrap_or_default(), + .cell(source_location.row) + .unwrap_or(OneIndexed::MIN), colon = ":".cyan(), )?; SourceLocation { - row: OneIndexed::new( - jupyter_index - .cell_row(source_location.row.get()) - .unwrap_or(1) as usize, - ) - .unwrap(), + row: jupyter_index + .cell_row(source_location.row) + .unwrap_or(OneIndexed::MIN), column: source_location.column, } } else { diff --git a/crates/ruff_linter/src/message/diff.rs b/crates/ruff_linter/src/message/diff.rs index 4c487b1b2d0b4..6bfe8750c4a36 100644 --- a/crates/ruff_linter/src/message/diff.rs +++ b/crates/ruff_linter/src/message/diff.rs @@ -35,6 +35,7 @@ impl<'a> Diff<'a> { impl Display for Diff<'_> { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + // TODO(dhruvmanila): Add support for Notebook cells once it's user-facing let mut output = String::with_capacity(self.source_code.source_text().len()); let mut last_end = TextSize::default(); diff --git a/crates/ruff_linter/src/message/grouped.rs b/crates/ruff_linter/src/message/grouped.rs index df01b8a9a4107..0445c1746193b 100644 --- a/crates/ruff_linter/src/message/grouped.rs +++ b/crates/ruff_linter/src/message/grouped.rs @@ -125,18 +125,18 @@ impl Display for DisplayGroupedMessage<'_> { f, "cell {cell}{sep}", cell = jupyter_index - .cell(start_location.row.get()) - .unwrap_or_default(), + .cell(start_location.row) + .unwrap_or(OneIndexed::MIN), sep = ":".cyan() )?; ( jupyter_index - .cell_row(start_location.row.get()) - .unwrap_or(1) as usize, - start_location.column.get(), + .cell_row(start_location.row) + .unwrap_or(OneIndexed::MIN), + start_location.column, ) } else { - (start_location.row.get(), start_location.column.get()) + (start_location.row, start_location.column) }; writeln!( diff --git a/crates/ruff_linter/src/message/mod.rs b/crates/ruff_linter/src/message/mod.rs index 0c243871a40a6..0e5520b5a75ff 100644 --- a/crates/ruff_linter/src/message/mod.rs +++ b/crates/ruff_linter/src/message/mod.rs @@ -150,7 +150,8 @@ mod tests { use rustc_hash::FxHashMap; use ruff_diagnostics::{Diagnostic, DiagnosticKind, Edit, Fix}; - use ruff_source_file::SourceFileBuilder; + use ruff_notebook::NotebookIndex; + use ruff_source_file::{OneIndexed, SourceFileBuilder}; use ruff_text_size::{Ranged, TextRange, TextSize}; use crate::message::{Emitter, EmitterContext, Message}; @@ -221,6 +222,113 @@ def fibonacci(n): ] } + pub(super) fn create_notebook_messages() -> (Vec, FxHashMap) { + let notebook = r"# cell 1 +import os +# cell 2 +import math + +print('hello world') +# cell 3 +def foo(): + print() + x = 1 +"; + + let unused_import_os = Diagnostic::new( + DiagnosticKind { + name: "UnusedImport".to_string(), + body: "`os` imported but unused".to_string(), + suggestion: Some("Remove unused import: `os`".to_string()), + }, + TextRange::new(TextSize::from(16), TextSize::from(18)), + ) + .with_fix(Fix::safe_edit(Edit::range_deletion(TextRange::new( + TextSize::from(9), + TextSize::from(19), + )))); + + let unused_import_math = Diagnostic::new( + DiagnosticKind { + name: "UnusedImport".to_string(), + body: "`math` imported but unused".to_string(), + suggestion: Some("Remove unused import: `math`".to_string()), + }, + TextRange::new(TextSize::from(35), TextSize::from(39)), + ) + .with_fix(Fix::safe_edit(Edit::range_deletion(TextRange::new( + TextSize::from(28), + TextSize::from(40), + )))); + + let unused_variable = Diagnostic::new( + DiagnosticKind { + name: "UnusedVariable".to_string(), + body: "Local variable `x` is assigned to but never used".to_string(), + suggestion: Some("Remove assignment to unused variable `x`".to_string()), + }, + TextRange::new(TextSize::from(98), TextSize::from(99)), + ) + .with_fix(Fix::unsafe_edit(Edit::deletion( + TextSize::from(94), + TextSize::from(104), + ))); + + let notebook_source = SourceFileBuilder::new("notebook.ipynb", notebook).finish(); + + let mut notebook_indexes = FxHashMap::default(); + notebook_indexes.insert( + "notebook.ipynb".to_string(), + NotebookIndex::new( + vec![ + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(1), + OneIndexed::from_zero_indexed(1), + OneIndexed::from_zero_indexed(1), + OneIndexed::from_zero_indexed(1), + OneIndexed::from_zero_indexed(2), + OneIndexed::from_zero_indexed(2), + OneIndexed::from_zero_indexed(2), + OneIndexed::from_zero_indexed(2), + ], + vec![ + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(1), + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(1), + OneIndexed::from_zero_indexed(2), + OneIndexed::from_zero_indexed(3), + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(1), + OneIndexed::from_zero_indexed(2), + OneIndexed::from_zero_indexed(3), + ], + ), + ); + + let unused_import_os_start = unused_import_os.start(); + let unused_import_math_start = unused_import_math.start(); + let unused_variable_start = unused_variable.start(); + + ( + vec![ + Message::from_diagnostic( + unused_import_os, + notebook_source.clone(), + unused_import_os_start, + ), + Message::from_diagnostic( + unused_import_math, + notebook_source.clone(), + unused_import_math_start, + ), + Message::from_diagnostic(unused_variable, notebook_source, unused_variable_start), + ], + notebook_indexes, + ) + } + pub(super) fn capture_emitter_output( emitter: &mut dyn Emitter, messages: &[Message], @@ -232,4 +340,16 @@ def fibonacci(n): String::from_utf8(output).expect("Output to be valid UTF-8") } + + pub(super) fn capture_emitter_notebook_output( + emitter: &mut dyn Emitter, + messages: &[Message], + notebook_indexes: &FxHashMap, + ) -> String { + let context = EmitterContext::new(notebook_indexes); + let mut output: Vec = Vec::new(); + emitter.emit(&mut output, messages, &context).unwrap(); + + String::from_utf8(output).expect("Output to be valid UTF-8") + } } diff --git a/crates/ruff_linter/src/message/snapshots/ruff_linter__message__text__tests__notebook_output.snap b/crates/ruff_linter/src/message/snapshots/ruff_linter__message__text__tests__notebook_output.snap new file mode 100644 index 0000000000000..5cd2f33939a8f --- /dev/null +++ b/crates/ruff_linter/src/message/snapshots/ruff_linter__message__text__tests__notebook_output.snap @@ -0,0 +1,32 @@ +--- +source: crates/ruff_linter/src/message/text.rs +expression: content +--- +notebook.ipynb:cell 1:2:8: F401 [*] `os` imported but unused + | +1 | # cell 1 +2 | import os + | ^^ F401 + | + = help: Remove unused import: `os` + +notebook.ipynb:cell 2:2:8: F401 [*] `math` imported but unused + | +1 | # cell 2 +2 | import math + | ^^^^ F401 +3 | +4 | print('hello world') + | + = help: Remove unused import: `math` + +notebook.ipynb:cell 3:4:5: F841 [*] Local variable `x` is assigned to but never used + | +2 | def foo(): +3 | print() +4 | x = 1 + | ^ F841 + | + = help: Remove assignment to unused variable `x` + + diff --git a/crates/ruff_linter/src/message/text.rs b/crates/ruff_linter/src/message/text.rs index 867f92bc031de..d43104d97dace 100644 --- a/crates/ruff_linter/src/message/text.rs +++ b/crates/ruff_linter/src/message/text.rs @@ -87,18 +87,15 @@ impl Emitter for TextEmitter { writer, "cell {cell}{sep}", cell = notebook_index - .cell(start_location.row.get()) - .unwrap_or_default(), + .cell(start_location.row) + .unwrap_or(OneIndexed::MIN), sep = ":".cyan(), )?; SourceLocation { - row: OneIndexed::new( - notebook_index - .cell_row(start_location.row.get()) - .unwrap_or(1) as usize, - ) - .unwrap(), + row: notebook_index + .cell_row(start_location.row) + .unwrap_or(OneIndexed::MIN), column: start_location.column, } } else { @@ -203,9 +200,9 @@ impl Display for MessageCodeFrame<'_> { // If we're working with a Jupyter Notebook, skip the lines which are // outside of the cell containing the diagnostic. if let Some(index) = self.notebook_index { - let content_start_cell = index.cell(content_start_index.get()).unwrap_or_default(); + let content_start_cell = index.cell(content_start_index).unwrap_or(OneIndexed::MIN); while start_index < content_start_index { - if index.cell(start_index.get()).unwrap_or_default() == content_start_cell { + if index.cell(start_index).unwrap_or(OneIndexed::MIN) == content_start_cell { break; } start_index = start_index.saturating_add(1); @@ -228,9 +225,9 @@ impl Display for MessageCodeFrame<'_> { // If we're working with a Jupyter Notebook, skip the lines which are // outside of the cell containing the diagnostic. if let Some(index) = self.notebook_index { - let content_end_cell = index.cell(content_end_index.get()).unwrap_or_default(); + let content_end_cell = index.cell(content_end_index).unwrap_or(OneIndexed::MIN); while end_index > content_end_index { - if index.cell(end_index.get()).unwrap_or_default() == content_end_cell { + if index.cell(end_index).unwrap_or(OneIndexed::MIN) == content_end_cell { break; } end_index = end_index.saturating_sub(1); @@ -270,8 +267,9 @@ impl Display for MessageCodeFrame<'_> { || start_index.get(), |notebook_index| { notebook_index - .cell_row(start_index.get()) - .unwrap_or_default() as usize + .cell_row(start_index) + .unwrap_or(OneIndexed::MIN) + .get() }, ), annotations: vec![SourceAnnotation { @@ -353,7 +351,10 @@ struct SourceCode<'a> { mod tests { use insta::assert_snapshot; - use crate::message::tests::{capture_emitter_output, create_messages}; + use crate::message::tests::{ + capture_emitter_notebook_output, capture_emitter_output, create_messages, + create_notebook_messages, + }; use crate::message::TextEmitter; use crate::settings::types::UnsafeFixes; @@ -385,4 +386,16 @@ mod tests { assert_snapshot!(content); } + + #[test] + fn notebook_output() { + let mut emitter = TextEmitter::default() + .with_show_fix_status(true) + .with_show_source(true) + .with_unsafe_fixes(UnsafeFixes::Enabled); + let (messages, notebook_indexes) = create_notebook_messages(); + let content = capture_emitter_notebook_output(&mut emitter, &messages, ¬ebook_indexes); + + assert_snapshot!(content); + } } diff --git a/crates/ruff_notebook/Cargo.toml b/crates/ruff_notebook/Cargo.toml index 7b3dfde8f6a6b..0796757ca36e5 100644 --- a/crates/ruff_notebook/Cargo.toml +++ b/crates/ruff_notebook/Cargo.toml @@ -14,7 +14,7 @@ license = { workspace = true } [dependencies] ruff_diagnostics = { path = "../ruff_diagnostics" } -ruff_source_file = { path = "../ruff_source_file" } +ruff_source_file = { path = "../ruff_source_file", features = ["serde"] } ruff_text_size = { path = "../ruff_text_size" } anyhow = { workspace = true } diff --git a/crates/ruff_notebook/src/index.rs b/crates/ruff_notebook/src/index.rs index 23259468ee56f..a001df375fd5d 100644 --- a/crates/ruff_notebook/src/index.rs +++ b/crates/ruff_notebook/src/index.rs @@ -1,5 +1,7 @@ use serde::{Deserialize, Serialize}; +use ruff_source_file::OneIndexed; + /// Jupyter Notebook indexing table /// /// When we lint a jupyter notebook, we have to translate the row/column based on @@ -7,20 +9,27 @@ use serde::{Deserialize, Serialize}; #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] pub struct NotebookIndex { /// Enter a row (1-based), get back the cell (1-based) - pub(super) row_to_cell: Vec, + pub(super) row_to_cell: Vec, /// Enter a row (1-based), get back the row in cell (1-based) - pub(super) row_to_row_in_cell: Vec, + pub(super) row_to_row_in_cell: Vec, } impl NotebookIndex { + pub fn new(row_to_cell: Vec, row_to_row_in_cell: Vec) -> Self { + Self { + row_to_cell, + row_to_row_in_cell, + } + } + /// Returns the cell number (1-based) for the given row (1-based). - pub fn cell(&self, row: usize) -> Option { - self.row_to_cell.get(row).copied() + pub fn cell(&self, row: OneIndexed) -> Option { + self.row_to_cell.get(row.to_zero_indexed()).copied() } /// Returns the row number (1-based) in the cell (1-based) for the /// given row (1-based). - pub fn cell_row(&self, row: usize) -> Option { - self.row_to_row_in_cell.get(row).copied() + pub fn cell_row(&self, row: OneIndexed) -> Option { + self.row_to_row_in_cell.get(row.to_zero_indexed()).copied() } } diff --git a/crates/ruff_notebook/src/notebook.rs b/crates/ruff_notebook/src/notebook.rs index f1e3e73bffeb9..34562473c4f52 100644 --- a/crates/ruff_notebook/src/notebook.rs +++ b/crates/ruff_notebook/src/notebook.rs @@ -13,7 +13,7 @@ use thiserror::Error; use uuid::Uuid; use ruff_diagnostics::{SourceMap, SourceMarker}; -use ruff_source_file::{NewlineWithTrailingNewline, UniversalNewlineIterator}; +use ruff_source_file::{NewlineWithTrailingNewline, OneIndexed, UniversalNewlineIterator}; use ruff_text_size::TextSize; use crate::index::NotebookIndex; @@ -179,7 +179,7 @@ impl Notebook { .iter() .enumerate() .filter(|(_, cell)| cell.is_valid_code_cell()) - .map(|(idx, _)| u32::try_from(idx).unwrap()) + .map(|(cell_index, _)| u32::try_from(cell_index).unwrap()) .collect::>(); let mut contents = Vec::with_capacity(valid_code_cells.len()); @@ -321,16 +321,16 @@ impl Notebook { /// The index building is expensive as it needs to go through the content of /// every valid code cell. fn build_index(&self) -> NotebookIndex { - let mut row_to_cell = vec![0]; - let mut row_to_row_in_cell = vec![0]; + let mut row_to_cell = Vec::new(); + let mut row_to_row_in_cell = Vec::new(); - for &idx in &self.valid_code_cells { - let line_count = match &self.raw.cells[idx as usize].source() { + for &cell_index in &self.valid_code_cells { + let line_count = match &self.raw.cells[cell_index as usize].source() { SourceValue::String(string) => { if string.is_empty() { 1 } else { - u32::try_from(NewlineWithTrailingNewline::from(string).count()).unwrap() + NewlineWithTrailingNewline::from(string).count() } } SourceValue::StringArray(string_array) => { @@ -339,12 +339,14 @@ impl Notebook { } else { let trailing_newline = usize::from(string_array.last().is_some_and(|s| s.ends_with('\n'))); - u32::try_from(string_array.len() + trailing_newline).unwrap() + string_array.len() + trailing_newline } } }; - row_to_cell.extend(iter::repeat(idx + 1).take(line_count as usize)); - row_to_row_in_cell.extend(1..=line_count); + row_to_cell.extend( + iter::repeat(OneIndexed::from_zero_indexed(cell_index as usize)).take(line_count), + ); + row_to_row_in_cell.extend((0..line_count).map(OneIndexed::from_zero_indexed)); } NotebookIndex { @@ -435,6 +437,8 @@ mod tests { use anyhow::Result; use test_case::test_case; + use ruff_source_file::OneIndexed; + use crate::{Cell, Notebook, NotebookError, NotebookIndex}; /// Construct a path to a Jupyter notebook in the `resources/test/fixtures/jupyter` directory. @@ -514,8 +518,40 @@ print("after empty cells") assert_eq!( notebook.index(), &NotebookIndex { - row_to_cell: vec![0, 1, 1, 1, 1, 1, 1, 3, 3, 3, 3, 3, 5, 7, 7, 8], - row_to_row_in_cell: vec![0, 1, 2, 3, 4, 5, 6, 1, 2, 3, 4, 5, 1, 1, 2, 1], + row_to_cell: vec![ + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(2), + OneIndexed::from_zero_indexed(2), + OneIndexed::from_zero_indexed(2), + OneIndexed::from_zero_indexed(2), + OneIndexed::from_zero_indexed(2), + OneIndexed::from_zero_indexed(4), + OneIndexed::from_zero_indexed(6), + OneIndexed::from_zero_indexed(6), + OneIndexed::from_zero_indexed(7) + ], + row_to_row_in_cell: vec![ + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(1), + OneIndexed::from_zero_indexed(2), + OneIndexed::from_zero_indexed(3), + OneIndexed::from_zero_indexed(4), + OneIndexed::from_zero_indexed(5), + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(1), + OneIndexed::from_zero_indexed(2), + OneIndexed::from_zero_indexed(3), + OneIndexed::from_zero_indexed(4), + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(1), + OneIndexed::from_zero_indexed(0) + ], } ); assert_eq!(