From e1b7b0023a81d12509583999beb9483f747dd41d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 26 Jan 2024 11:26:42 -0500 Subject: [PATCH] Detect automagic-like assignments in notebooks --- crates/ruff_linter/src/linter.rs | 17 ++++++ ...linter__linter__tests__undefined_name.snap | 10 ++++ .../jupyter/cell/automagic_assignment.json | 8 +++ .../fixtures/jupyter/undefined_name.ipynb | 59 +++++++++++++++++++ crates/ruff_notebook/src/cell.rs | 25 +++++--- crates/ruff_notebook/src/notebook.rs | 1 + 6 files changed, 113 insertions(+), 7 deletions(-) create mode 100644 crates/ruff_linter/src/snapshots/ruff_linter__linter__tests__undefined_name.snap create mode 100644 crates/ruff_notebook/resources/test/fixtures/jupyter/cell/automagic_assignment.json create mode 100644 crates/ruff_notebook/resources/test/fixtures/jupyter/undefined_name.ipynb diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index a81f8f89f0831..af7baf4f7d8f9 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -795,6 +795,23 @@ mod tests { Ok(()) } + #[test] + fn test_undefined_name() -> Result<(), NotebookError> { + let actual = notebook_path("undefined_name.ipynb"); + let expected = notebook_path("undefined_name.ipynb"); + let TestedNotebook { + messages, + source_notebook, + .. + } = assert_notebook_path( + &actual, + expected, + &settings::LinterSettings::for_rule(Rule::UndefinedName), + )?; + assert_messages!(messages, actual, source_notebook); + Ok(()) + } + #[test] fn test_json_consistency() -> Result<()> { let actual_path = notebook_path("before_fix.ipynb"); diff --git a/crates/ruff_linter/src/snapshots/ruff_linter__linter__tests__undefined_name.snap b/crates/ruff_linter/src/snapshots/ruff_linter__linter__tests__undefined_name.snap new file mode 100644 index 0000000000000..d38bdfa2e069b --- /dev/null +++ b/crates/ruff_linter/src/snapshots/ruff_linter__linter__tests__undefined_name.snap @@ -0,0 +1,10 @@ +--- +source: crates/ruff_linter/src/linter.rs +--- +undefined_name.ipynb:cell 3:1:7: F821 Undefined name `undefined` + | +1 | print(undefined) + | ^^^^^^^^^ F821 + | + + diff --git a/crates/ruff_notebook/resources/test/fixtures/jupyter/cell/automagic_assignment.json b/crates/ruff_notebook/resources/test/fixtures/jupyter/cell/automagic_assignment.json new file mode 100644 index 0000000000000..c4cb430447bf4 --- /dev/null +++ b/crates/ruff_notebook/resources/test/fixtures/jupyter/cell/automagic_assignment.json @@ -0,0 +1,8 @@ +{ + "execution_count": null, + "cell_type": "code", + "id": "1", + "metadata": {}, + "outputs": [], + "source": ["colors = 6\n", "print(colors)"] +} diff --git a/crates/ruff_notebook/resources/test/fixtures/jupyter/undefined_name.ipynb b/crates/ruff_notebook/resources/test/fixtures/jupyter/undefined_name.ipynb new file mode 100644 index 0000000000000..0256ba88ba81a --- /dev/null +++ b/crates/ruff_notebook/resources/test/fixtures/jupyter/undefined_name.ipynb @@ -0,0 +1,59 @@ +{ + "cells": [ + { + "cell_type": "code", + "execution_count": null, + "id": "a0efffbc-85f1-4513-bf49-5387ec3a2a4e", + "metadata": {}, + "outputs": [], + "source": [ + "colors = 6" + ] + }, + { + "cell_type": "code", + "execution_count": null, + "id": "6e0b2b50-43f2-4f59-951d-9404dd560ae4", + "metadata": { + "is_executing": true + }, + "outputs": [], + "source": [ + "print(colors)" + ] + }, + { + "cell_type": "code", + "execution_count": null, + "outputs": [], + "source": [ + "print(undefined)" + ], + "metadata": { + "collapsed": false + }, + "id": "884e0e4e686fd56" + } + ], + "metadata": { + "kernelspec": { + "display_name": "Python (ruff)", + "language": "python", + "name": "ruff" + }, + "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_notebook/src/cell.rs b/crates/ruff_notebook/src/cell.rs index eefc18918b5cb..b6fad8f1f02df 100644 --- a/crates/ruff_notebook/src/cell.rs +++ b/crates/ruff_notebook/src/cell.rs @@ -80,10 +80,11 @@ impl Cell { // ``` // // See: https://ipython.readthedocs.io/en/stable/interactive/magics.html - if lines - .peek() - .and_then(|line| line.split_whitespace().next()) - .is_some_and(|token| { + if let Some(line) = lines.peek() { + let mut tokens = line.split_whitespace(); + + // The first token must be an automagic, like `load_exit`. + if tokens.next().is_some_and(|token| { matches!( token, "alias" @@ -164,9 +165,19 @@ impl Cell { | "xdel" | "xmode" ) - }) - { - return true; + }) { + // The second token must _not_ be an operator, like `=` (to avoid false positives). + // The assignment operators can never follow a magic. Some binary operators _can_, + // though (e.g., `cd -` is valid), so we omit them. + if !tokens.next().is_some_and(|token| { + matches!( + token, + "=" | "+=" | "-=" | "*=" | "/=" | "//=" | "%=" | "**=" | "&=" | "|=" | "^=" + ) + }) { + return true; + } + } } // Detect cell magics (which operate on multiple lines). diff --git a/crates/ruff_notebook/src/notebook.rs b/crates/ruff_notebook/src/notebook.rs index 94d5a312c6f9f..804e921f13286 100644 --- a/crates/ruff_notebook/src/notebook.rs +++ b/crates/ruff_notebook/src/notebook.rs @@ -454,6 +454,7 @@ mod tests { #[test_case("cell_magic", false)] #[test_case("valid_cell_magic", true)] #[test_case("automagic", false)] + #[test_case("automagic_assignment", true)] #[test_case("automagics", false)] #[test_case("automagic_before_code", false)] #[test_case("automagic_after_code", true)]