Treat display as a builtin in IPython (#8707)

## Summary

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

Closes https://github.com/astral-sh/ruff/issues/8702.
This commit is contained in:
Charlie Marsh 2023-11-15 17:58:44 -08:00 committed by GitHub
parent 2083352ae3
commit 4ac78d5725
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
13 changed files with 160 additions and 22 deletions

View file

@ -0,0 +1,4 @@
"""Test for IPython-only builtins."""
x = 1
display(x)

View file

@ -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
}

View file

@ -54,7 +54,7 @@ use ruff_python_semantic::{
ModuleKind, NodeId, ScopeId, ScopeKind, SemanticModel, SemanticModelFlags, Snapshot, ModuleKind, NodeId, ScopeId, ScopeKind, SemanticModel, SemanticModelFlags, Snapshot,
StarImport, SubmoduleImport, 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 ruff_source_file::Locator;
use crate::checkers::ast::deferred::Deferred; use crate::checkers::ast::deferred::Deferred;
@ -1592,9 +1592,16 @@ impl<'a> Checker<'a> {
} }
fn bind_builtins(&mut self) { fn bind_builtins(&mut self) {
for builtin in BUILTINS for builtin in PYTHON_BUILTINS
.iter() .iter()
.chain(MAGIC_GLOBALS.iter()) .chain(MAGIC_GLOBALS.iter())
.chain(
self.source_type
.is_ipynb()
.then_some(IPYTHON_BUILTINS)
.into_iter()
.flatten(),
)
.copied() .copied()
.chain(self.settings.builtins.iter().map(String::as_str)) .chain(self.settings.builtins.iter().map(String::as_str))
{ {

View file

@ -639,7 +639,7 @@ mod tests {
use crate::registry::Rule; use crate::registry::Rule;
use crate::source_kind::SourceKind; 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}; use crate::{assert_messages, settings};
/// Construct a path to a Jupyter notebook in the `resources/test/fixtures/jupyter` directory. /// Construct a path to a Jupyter notebook in the `resources/test/fixtures/jupyter` directory.
@ -655,7 +655,7 @@ mod tests {
messages, messages,
source_notebook, source_notebook,
.. ..
} = test_notebook_path( } = assert_notebook_path(
&actual, &actual,
expected, expected,
&settings::LinterSettings::for_rule(Rule::UnsortedImports), &settings::LinterSettings::for_rule(Rule::UnsortedImports),
@ -672,7 +672,7 @@ mod tests {
messages, messages,
source_notebook, source_notebook,
.. ..
} = test_notebook_path( } = assert_notebook_path(
&actual, &actual,
expected, expected,
&settings::LinterSettings::for_rule(Rule::UnusedImport), &settings::LinterSettings::for_rule(Rule::UnusedImport),
@ -689,7 +689,7 @@ mod tests {
messages, messages,
source_notebook, source_notebook,
.. ..
} = test_notebook_path( } = assert_notebook_path(
&actual, &actual,
expected, expected,
&settings::LinterSettings::for_rule(Rule::UnusedVariable), &settings::LinterSettings::for_rule(Rule::UnusedVariable),
@ -706,7 +706,7 @@ mod tests {
let TestedNotebook { let TestedNotebook {
linted_notebook: fixed_notebook, linted_notebook: fixed_notebook,
.. ..
} = test_notebook_path( } = assert_notebook_path(
actual_path, actual_path,
&expected_path, &expected_path,
&settings::LinterSettings::for_rule(Rule::UnusedImport), &settings::LinterSettings::for_rule(Rule::UnusedImport),

View file

@ -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 { pub(super) fn shadows_builtin(
is_builtin(name) && ignorelist.iter().all(|ignore| ignore != name) 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
}
} }

View file

@ -67,6 +67,7 @@ pub(crate) fn builtin_argument_shadowing(checker: &mut Checker, parameter: &Para
if shadows_builtin( if shadows_builtin(
parameter.name.as_str(), parameter.name.as_str(),
&checker.settings.flake8_builtins.builtins_ignorelist, &checker.settings.flake8_builtins.builtins_ignorelist,
checker.source_type,
) { ) {
checker.diagnostics.push(Diagnostic::new( checker.diagnostics.push(Diagnostic::new(
BuiltinArgumentShadowing { BuiltinArgumentShadowing {

View file

@ -74,7 +74,11 @@ pub(crate) fn builtin_attribute_shadowing(
name: &str, name: &str,
range: TextRange, 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 // Ignore shadowing within `TypedDict` definitions, since these are only accessible through
// subscripting and not through attribute access. // subscripting and not through attribute access.
if class_def if class_def
@ -102,7 +106,11 @@ pub(crate) fn builtin_method_shadowing(
decorator_list: &[Decorator], decorator_list: &[Decorator],
range: TextRange, 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 // 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. // those should be flagged on the superclass, but that's more difficult.
if is_standard_library_override(name, class_def, checker.semantic()) { if is_standard_library_override(name, class_def, checker.semantic()) {

View file

@ -60,7 +60,11 @@ impl Violation for BuiltinVariableShadowing {
/// A001 /// A001
pub(crate) fn builtin_variable_shadowing(checker: &mut Checker, name: &str, range: TextRange) { 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( checker.diagnostics.push(Diagnostic::new(
BuiltinVariableShadowing { BuiltinVariableShadowing {
name: name.to_string(), name: name.to_string(),

View file

@ -138,6 +138,8 @@ mod tests {
#[test_case(Rule::UndefinedName, Path::new("F821_18.py"))] #[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_19.py"))]
#[test_case(Rule::UndefinedName, Path::new("F821_20.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_0.py"))]
#[test_case(Rule::UndefinedExport, Path::new("F822_1.py"))] #[test_case(Rule::UndefinedExport, Path::new("F822_1.py"))]
#[test_case(Rule::UndefinedExport, Path::new("F822_2.py"))] #[test_case(Rule::UndefinedExport, Path::new("F822_2.py"))]

View file

@ -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
|

View file

@ -0,0 +1,4 @@
---
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---

View file

@ -38,12 +38,13 @@ pub(crate) fn test_resource_path(path: impl AsRef<Path>) -> std::path::PathBuf {
Path::new("./resources/test/").join(path) 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))] #[cfg(not(fuzzing))]
pub(crate) fn test_path(path: impl AsRef<Path>, settings: &LinterSettings) -> Result<Vec<Message>> { pub(crate) fn test_path(path: impl AsRef<Path>, settings: &LinterSettings) -> Result<Vec<Message>> {
let path = test_resource_path("fixtures").join(path); let path = test_resource_path("fixtures").join(path);
let contents = std::fs::read_to_string(&path)?; let source_type = PySourceType::from(&path);
Ok(test_contents(&SourceKind::Python(contents), &path, settings).0) 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))] #[cfg(not(fuzzing))]
@ -54,7 +55,7 @@ pub(crate) struct TestedNotebook {
} }
#[cfg(not(fuzzing))] #[cfg(not(fuzzing))]
pub(crate) fn test_notebook_path( pub(crate) fn assert_notebook_path(
path: impl AsRef<Path>, path: impl AsRef<Path>,
expected: impl AsRef<Path>, expected: impl AsRef<Path>,
settings: &LinterSettings, settings: &LinterSettings,

View file

@ -1,7 +1,7 @@
/// A list of all Python builtins. /// A list of all Python builtins.
/// ///
/// Intended to be kept in sync with [`is_builtin`]. /// Intended to be kept in sync with [`is_python_builtin`].
pub const BUILTINS: &[&str] = &[ pub const PYTHON_BUILTINS: &[&str] = &[
"ArithmeticError", "ArithmeticError",
"AssertionError", "AssertionError",
"AttributeError", "AttributeError",
@ -161,6 +161,11 @@ pub const BUILTINS: &[&str] = &[
"zip", "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 /// Globally defined names which are not attributes of the builtins module, or
/// are only present on some platforms. /// are only present on some platforms.
pub const MAGIC_GLOBALS: &[&str] = &[ pub const MAGIC_GLOBALS: &[&str] = &[
@ -173,9 +178,9 @@ pub const MAGIC_GLOBALS: &[&str] = &[
/// Returns `true` if the given name is that of a Python builtin. /// Returns `true` if the given name is that of a Python builtin.
/// ///
/// Intended to be kept in sync with [`BUILTINS`]. /// Intended to be kept in sync with [`PYTHON_BUILTINS`].
pub fn is_builtin(name: &str) -> bool { pub fn is_python_builtin(name: &str) -> bool {
// Constructed by converting the `BUILTINS` slice to a `match` expression. // Constructed by converting the `PYTHON_BUILTINS` slice to a `match` expression.
matches!( matches!(
name, name,
"ArithmeticError" "ArithmeticError"
@ -345,3 +350,11 @@ pub fn is_iterator(name: &str) -> bool {
"enumerate" | "filter" | "map" | "reversed" | "zip" | "iter" "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")
}