Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treat display as a builtin in IPython #8707

Merged
merged 2 commits into from
Nov 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed this to avoid some confusion.

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")
}
Loading