Skip to content

Commit

Permalink
Treat display as a builtin in IPython (#8707)
Browse files Browse the repository at this point in the history
## Summary

`display` is a special-cased builtin in IPython. This PR adds it to the
builtin namespace when analyzing IPython notebooks.

Closes #8702.
  • Loading branch information
charliermarsh committed Nov 16, 2023
1 parent 2083352 commit 4ac78d5
Show file tree
Hide file tree
Showing 13 changed files with 160 additions and 22 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
"""Test for IPython-only builtins."""

x = 1
display(x)
74 changes: 74 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pyflakes/F821_22.ipynb
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
{
"cells": [
{
"cell_type": "code",
"execution_count": 1,
"id": "af8fee97-f9aa-47c2-b34c-b109a5f083d6",
"metadata": {},
"outputs": [
{
"data": {
"text/plain": [
"1"
]
},
"execution_count": 1,
"metadata": {},
"output_type": "execute_result"
}
],
"source": [
"1"
]
},
{
"cell_type": "code",
"execution_count": 2,
"id": "acd5eb1e-6991-42b8-806f-20d17d7e571f",
"metadata": {},
"outputs": [
{
"data": {
"text/plain": [
"1"
]
},
"metadata": {},
"output_type": "display_data"
}
],
"source": [
"display(1)"
]
},
{
"cell_type": "code",
"execution_count": null,
"id": "f8f3f599-030e-48b3-bcd3-31d97f725c68",
"metadata": {},
"outputs": [],
"source": []
}
],
"metadata": {
"kernelspec": {
"display_name": "Python 3 (ipykernel)",
"language": "python",
"name": "python3"
},
"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.5"
}
},
"nbformat": 4,
"nbformat_minor": 5
}
11 changes: 9 additions & 2 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ use ruff_python_semantic::{
ModuleKind, NodeId, ScopeId, ScopeKind, SemanticModel, SemanticModelFlags, Snapshot,
StarImport, SubmoduleImport,
};
use ruff_python_stdlib::builtins::{BUILTINS, MAGIC_GLOBALS};
use ruff_python_stdlib::builtins::{IPYTHON_BUILTINS, MAGIC_GLOBALS, PYTHON_BUILTINS};
use ruff_source_file::Locator;

use crate::checkers::ast::deferred::Deferred;
Expand Down Expand Up @@ -1592,9 +1592,16 @@ impl<'a> Checker<'a> {
}

fn bind_builtins(&mut self) {
for builtin in BUILTINS
for builtin in PYTHON_BUILTINS
.iter()
.chain(MAGIC_GLOBALS.iter())
.chain(
self.source_type
.is_ipynb()
.then_some(IPYTHON_BUILTINS)
.into_iter()
.flatten(),
)
.copied()
.chain(self.settings.builtins.iter().map(String::as_str))
{
Expand Down
10 changes: 5 additions & 5 deletions crates/ruff_linter/src/linter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -639,7 +639,7 @@ mod tests {

use crate::registry::Rule;
use crate::source_kind::SourceKind;
use crate::test::{test_contents, test_notebook_path, TestedNotebook};
use crate::test::{assert_notebook_path, test_contents, TestedNotebook};
use crate::{assert_messages, settings};

/// Construct a path to a Jupyter notebook in the `resources/test/fixtures/jupyter` directory.
Expand All @@ -655,7 +655,7 @@ mod tests {
messages,
source_notebook,
..
} = test_notebook_path(
} = assert_notebook_path(
&actual,
expected,
&settings::LinterSettings::for_rule(Rule::UnsortedImports),
Expand All @@ -672,7 +672,7 @@ mod tests {
messages,
source_notebook,
..
} = test_notebook_path(
} = assert_notebook_path(
&actual,
expected,
&settings::LinterSettings::for_rule(Rule::UnusedImport),
Expand All @@ -689,7 +689,7 @@ mod tests {
messages,
source_notebook,
..
} = test_notebook_path(
} = assert_notebook_path(
&actual,
expected,
&settings::LinterSettings::for_rule(Rule::UnusedVariable),
Expand All @@ -706,7 +706,7 @@ mod tests {
let TestedNotebook {
linted_notebook: fixed_notebook,
..
} = test_notebook_path(
} = assert_notebook_path(
actual_path,
&expected_path,
&settings::LinterSettings::for_rule(Rule::UnusedImport),
Expand Down
15 changes: 12 additions & 3 deletions crates/ruff_linter/src/rules/flake8_builtins/helpers.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
use ruff_python_stdlib::builtins::is_builtin;
use ruff_python_ast::PySourceType;
use ruff_python_stdlib::builtins::{is_ipython_builtin, is_python_builtin};

pub(super) fn shadows_builtin(name: &str, ignorelist: &[String]) -> bool {
is_builtin(name) && ignorelist.iter().all(|ignore| ignore != name)
pub(super) fn shadows_builtin(
name: &str,
ignorelist: &[String],
source_type: PySourceType,
) -> bool {
if is_python_builtin(name) || source_type.is_ipynb() && is_ipython_builtin(name) {
ignorelist.iter().all(|ignore| ignore != name)
} else {
false
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ pub(crate) fn builtin_argument_shadowing(checker: &mut Checker, parameter: &Para
if shadows_builtin(
parameter.name.as_str(),
&checker.settings.flake8_builtins.builtins_ignorelist,
checker.source_type,
) {
checker.diagnostics.push(Diagnostic::new(
BuiltinArgumentShadowing {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,11 @@ pub(crate) fn builtin_attribute_shadowing(
name: &str,
range: TextRange,
) {
if shadows_builtin(name, &checker.settings.flake8_builtins.builtins_ignorelist) {
if shadows_builtin(
name,
&checker.settings.flake8_builtins.builtins_ignorelist,
checker.source_type,
) {
// Ignore shadowing within `TypedDict` definitions, since these are only accessible through
// subscripting and not through attribute access.
if class_def
Expand Down Expand Up @@ -102,7 +106,11 @@ pub(crate) fn builtin_method_shadowing(
decorator_list: &[Decorator],
range: TextRange,
) {
if shadows_builtin(name, &checker.settings.flake8_builtins.builtins_ignorelist) {
if shadows_builtin(
name,
&checker.settings.flake8_builtins.builtins_ignorelist,
checker.source_type,
) {
// Ignore some standard-library methods. Ideally, we'd ignore all overridden methods, since
// those should be flagged on the superclass, but that's more difficult.
if is_standard_library_override(name, class_def, checker.semantic()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,11 @@ impl Violation for BuiltinVariableShadowing {

/// A001
pub(crate) fn builtin_variable_shadowing(checker: &mut Checker, name: &str, range: TextRange) {
if shadows_builtin(name, &checker.settings.flake8_builtins.builtins_ignorelist) {
if shadows_builtin(
name,
&checker.settings.flake8_builtins.builtins_ignorelist,
checker.source_type,
) {
checker.diagnostics.push(Diagnostic::new(
BuiltinVariableShadowing {
name: name.to_string(),
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pyflakes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,8 @@ mod tests {
#[test_case(Rule::UndefinedName, Path::new("F821_18.py"))]
#[test_case(Rule::UndefinedName, Path::new("F821_19.py"))]
#[test_case(Rule::UndefinedName, Path::new("F821_20.py"))]
#[test_case(Rule::UndefinedName, Path::new("F821_21.py"))]
#[test_case(Rule::UndefinedName, Path::new("F821_22.ipynb"))]
#[test_case(Rule::UndefinedExport, Path::new("F822_0.py"))]
#[test_case(Rule::UndefinedExport, Path::new("F822_1.py"))]
#[test_case(Rule::UndefinedExport, Path::new("F822_2.py"))]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F821_21.py:4:1: F821 Undefined name `display`
|
3 | x = 1
4 | display(x)
| ^^^^^^^ F821
|


Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---

9 changes: 5 additions & 4 deletions crates/ruff_linter/src/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,13 @@ pub(crate) fn test_resource_path(path: impl AsRef<Path>) -> std::path::PathBuf {
Path::new("./resources/test/").join(path)
}

/// Run [`check_path`] on a file in the `resources/test/fixtures` directory.
/// Run [`check_path`] on a Python file in the `resources/test/fixtures` directory.
#[cfg(not(fuzzing))]
pub(crate) fn test_path(path: impl AsRef<Path>, settings: &LinterSettings) -> Result<Vec<Message>> {
let path = test_resource_path("fixtures").join(path);
let contents = std::fs::read_to_string(&path)?;
Ok(test_contents(&SourceKind::Python(contents), &path, settings).0)
let source_type = PySourceType::from(&path);
let source_kind = SourceKind::from_path(path.as_ref(), source_type)?.expect("valid source");
Ok(test_contents(&source_kind, &path, settings).0)
}

#[cfg(not(fuzzing))]
Expand All @@ -54,7 +55,7 @@ pub(crate) struct TestedNotebook {
}

#[cfg(not(fuzzing))]
pub(crate) fn test_notebook_path(
pub(crate) fn assert_notebook_path(
path: impl AsRef<Path>,
expected: impl AsRef<Path>,
settings: &LinterSettings,
Expand Down
23 changes: 18 additions & 5 deletions crates/ruff_python_stdlib/src/builtins.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/// A list of all Python builtins.
///
/// Intended to be kept in sync with [`is_builtin`].
pub const BUILTINS: &[&str] = &[
/// Intended to be kept in sync with [`is_python_builtin`].
pub const PYTHON_BUILTINS: &[&str] = &[
"ArithmeticError",
"AssertionError",
"AttributeError",
Expand Down Expand Up @@ -161,6 +161,11 @@ pub const BUILTINS: &[&str] = &[
"zip",
];

/// A list of all builtins that are available in IPython.
///
/// Intended to be kept in sync with [`is_ipython_builtin`].
pub const IPYTHON_BUILTINS: &[&str] = &["display"];

/// Globally defined names which are not attributes of the builtins module, or
/// are only present on some platforms.
pub const MAGIC_GLOBALS: &[&str] = &[
Expand All @@ -173,9 +178,9 @@ pub const MAGIC_GLOBALS: &[&str] = &[

/// Returns `true` if the given name is that of a Python builtin.
///
/// Intended to be kept in sync with [`BUILTINS`].
pub fn is_builtin(name: &str) -> bool {
// Constructed by converting the `BUILTINS` slice to a `match` expression.
/// Intended to be kept in sync with [`PYTHON_BUILTINS`].
pub fn is_python_builtin(name: &str) -> bool {
// Constructed by converting the `PYTHON_BUILTINS` slice to a `match` expression.
matches!(
name,
"ArithmeticError"
Expand Down Expand Up @@ -345,3 +350,11 @@ pub fn is_iterator(name: &str) -> bool {
"enumerate" | "filter" | "map" | "reversed" | "zip" | "iter"
)
}

/// Returns `true` if the given name is that of an IPython builtin.
///
/// Intended to be kept in sync with [`IPYTHON_BUILTINS`].
pub fn is_ipython_builtin(name: &str) -> bool {
// Constructed by converting the `IPYTHON_BUILTINS` slice to a `match` expression.
matches!(name, "display")
}

0 comments on commit 4ac78d5

Please sign in to comment.