-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[pylint] Implement global-variable-undefined (W0601) #10820
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
# pylint: disable=invalid-name, import-outside-toplevel, too-few-public-methods, unused-import | ||
# pylint: disable=missing-module-docstring, missing-function-docstring, missing-class-docstring | ||
# pylint: disable=global-at-module-level, global-statement, global-variable-not-assigned | ||
# pylint: diable=redefined-outer-name | ||
import dataclasses | ||
from os import getenv | ||
|
||
CONSTANT = 1 | ||
UNDEFINED: int | ||
|
||
|
||
def FUNC(): | ||
pass | ||
|
||
|
||
class CLASS: | ||
pass | ||
|
||
|
||
# BAD | ||
def global_variable_undefined(): | ||
global SOMEVAR # [global-variable-undefined] | ||
SOMEVAR = 2 | ||
|
||
|
||
# OK | ||
def global_constant(): | ||
global CONSTANT | ||
print(CONSTANT) | ||
global UNDEFINED | ||
UNDEFINED = 1 | ||
global CONSTANT_2 | ||
print(CONSTANT_2) | ||
global dataclasses | ||
dataclasses = 3 | ||
global getenv | ||
getenv = 4 | ||
|
||
|
||
def global_with_import(): | ||
global sys | ||
import sys | ||
|
||
|
||
def global_with_import_from(): | ||
global namedtuple | ||
from collections import namedtuple | ||
|
||
|
||
def override_func(): | ||
global FUNC | ||
|
||
def FUNC(): | ||
pass | ||
|
||
FUNC() | ||
|
||
|
||
def override_class(): | ||
global CLASS | ||
|
||
class CLASS(): | ||
pass | ||
|
||
CLASS() | ||
|
||
|
||
CONSTANT_2 = 2 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
# pylint: disable=missing-module-docstring, missing-function-docstring | ||
# pylint: disable=redefined-builtin, unnecessary-lambda-assignment | ||
# pylint: disable=global-statement, unused-wildcard-import, wildcard-import | ||
from os import * | ||
|
||
|
||
# OK | ||
def global_star_import(): | ||
global system | ||
system = lambda _: None |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,152 @@ | ||||||||||||||||||||||||||||||||||||
use ruff_diagnostics::{Diagnostic, Violation}; | ||||||||||||||||||||||||||||||||||||
use ruff_macros::{derive_message_formats, violation}; | ||||||||||||||||||||||||||||||||||||
use ruff_python_ast::{self as ast, Expr, Stmt, StmtGlobal}; | ||||||||||||||||||||||||||||||||||||
use ruff_python_semantic::{BindingId, BindingKind, Scope, ScopeKind}; | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
use crate::checkers::ast::Checker; | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
/// ## What it does | ||||||||||||||||||||||||||||||||||||
/// Checks that all `global` variables are indeed defined on module level | ||||||||||||||||||||||||||||||||||||
/// | ||||||||||||||||||||||||||||||||||||
/// ## Why is this bad? | ||||||||||||||||||||||||||||||||||||
/// If the module level declaration is missing, then either if was | ||||||||||||||||||||||||||||||||||||
/// forgotten or the `global` can be omitted. | ||||||||||||||||||||||||||||||||||||
/// | ||||||||||||||||||||||||||||||||||||
/// ## Example | ||||||||||||||||||||||||||||||||||||
/// ```python | ||||||||||||||||||||||||||||||||||||
/// def foo(): | ||||||||||||||||||||||||||||||||||||
/// global var # [global-variable-undefined] | ||||||||||||||||||||||||||||||||||||
/// var = 10 | ||||||||||||||||||||||||||||||||||||
/// print(var) | ||||||||||||||||||||||||||||||||||||
/// ``` | ||||||||||||||||||||||||||||||||||||
/// | ||||||||||||||||||||||||||||||||||||
/// Use instead: | ||||||||||||||||||||||||||||||||||||
/// ```python | ||||||||||||||||||||||||||||||||||||
/// var = 1 | ||||||||||||||||||||||||||||||||||||
/// | ||||||||||||||||||||||||||||||||||||
/// | ||||||||||||||||||||||||||||||||||||
/// def foo(): | ||||||||||||||||||||||||||||||||||||
/// global var | ||||||||||||||||||||||||||||||||||||
/// var = 10 | ||||||||||||||||||||||||||||||||||||
/// print(var) | ||||||||||||||||||||||||||||||||||||
/// ``` | ||||||||||||||||||||||||||||||||||||
#[violation] | ||||||||||||||||||||||||||||||||||||
pub struct GlobalVariableUndefined { | ||||||||||||||||||||||||||||||||||||
name: String, | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
impl Violation for GlobalVariableUndefined { | ||||||||||||||||||||||||||||||||||||
#[derive_message_formats] | ||||||||||||||||||||||||||||||||||||
fn message(&self) -> String { | ||||||||||||||||||||||||||||||||||||
let GlobalVariableUndefined { name } = self; | ||||||||||||||||||||||||||||||||||||
format!("Global variable `{name}` is undefined at the module") | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
/// PLW0601 | ||||||||||||||||||||||||||||||||||||
pub(crate) fn global_variable_undefined(checker: &mut Checker, stmt: &Stmt) { | ||||||||||||||||||||||||||||||||||||
if checker.semantic().current_scope().kind.is_module() { | ||||||||||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
let Stmt::Global(StmtGlobal { names, range }) = stmt else { | ||||||||||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
if checker | ||||||||||||||||||||||||||||||||||||
.semantic() | ||||||||||||||||||||||||||||||||||||
.current_scopes() | ||||||||||||||||||||||||||||||||||||
.any(Scope::uses_star_imports) | ||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
let module_scope = checker.semantic().global_scope(); | ||||||||||||||||||||||||||||||||||||
let imported_names = get_imports(checker); | ||||||||||||||||||||||||||||||||||||
let mut undefined_names = vec![]; | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
for name in names { | ||||||||||||||||||||||||||||||||||||
// Skip if imported names | ||||||||||||||||||||||||||||||||||||
if imported_names.contains(&name.as_str()) { | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My python knowledge is too limited to understand why it is okay to ignore imports from the enclosing function's scope. Could you help me understand why it is okay to ignore imports? I read https://docs.python.org/3/reference/simple_stmts.html#the-global-statement and couldn't find any special handling of imports in the enclosing scope. |
||||||||||||||||||||||||||||||||||||
continue; | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
// Test binding which has been already defined | ||||||||||||||||||||||||||||||||||||
let Some(binding_id) = module_scope.get(name) else { | ||||||||||||||||||||||||||||||||||||
continue; | ||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||
if is_global_binding(checker, binding_id) { | ||||||||||||||||||||||||||||||||||||
continue; | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
// Test binding which has been only declared | ||||||||||||||||||||||||||||||||||||
if let Some(shadowed_binding_id) = module_scope.shadowed_binding(binding_id) { | ||||||||||||||||||||||||||||||||||||
if is_global_binding(checker, shadowed_binding_id) { | ||||||||||||||||||||||||||||||||||||
continue; | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
undefined_names.push(name); | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
for name in undefined_names { | ||||||||||||||||||||||||||||||||||||
checker.diagnostics.push(Diagnostic::new( | ||||||||||||||||||||||||||||||||||||
GlobalVariableUndefined { | ||||||||||||||||||||||||||||||||||||
name: name.to_string(), | ||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||
*range, | ||||||||||||||||||||||||||||||||||||
)); | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
fn get_imports<'a>(checker: &'a Checker) -> Vec<&'a str> { | ||||||||||||||||||||||||||||||||||||
// Get all names imported in the current scope | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we only look at imports of the current scope? What about imports in the enclosing scope? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This covers the case when the |
||||||||||||||||||||||||||||||||||||
let Some(fs) = checker | ||||||||||||||||||||||||||||||||||||
.semantic() | ||||||||||||||||||||||||||||||||||||
.current_scopes() | ||||||||||||||||||||||||||||||||||||
.find(|scope| scope.kind.is_function()) | ||||||||||||||||||||||||||||||||||||
else { | ||||||||||||||||||||||||||||||||||||
return vec![]; | ||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||
let ScopeKind::Function(ast::StmtFunctionDef { body, .. }) = fs.kind else { | ||||||||||||||||||||||||||||||||||||
return vec![]; | ||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||
Comment on lines
+102
to
+111
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit:
Suggested change
|
||||||||||||||||||||||||||||||||||||
let mut import_names = vec![]; | ||||||||||||||||||||||||||||||||||||
for stmt in body { | ||||||||||||||||||||||||||||||||||||
match stmt { | ||||||||||||||||||||||||||||||||||||
Stmt::Import(ast::StmtImport { names, .. }) | ||||||||||||||||||||||||||||||||||||
| Stmt::ImportFrom(ast::StmtImportFrom { names, .. }) => { | ||||||||||||||||||||||||||||||||||||
for name in names { | ||||||||||||||||||||||||||||||||||||
import_names.push(name.name.as_str()); | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
_ => (), | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
Comment on lines
+113
to
+123
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this also respect imports in conditional branches. E.g. what about: def test(a):
if a:
from b import bar
globals bar |
||||||||||||||||||||||||||||||||||||
import_names | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
fn is_global_binding(checker: &Checker, binding_id: BindingId) -> bool { | ||||||||||||||||||||||||||||||||||||
let binding = checker.semantic().binding(binding_id); | ||||||||||||||||||||||||||||||||||||
// Skip if import | ||||||||||||||||||||||||||||||||||||
if matches!( | ||||||||||||||||||||||||||||||||||||
binding.kind, | ||||||||||||||||||||||||||||||||||||
BindingKind::FromImport(_) | BindingKind::Import(_) | BindingKind::FutureImport | ||||||||||||||||||||||||||||||||||||
) { | ||||||||||||||||||||||||||||||||||||
return true; | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
// Skip if module level class or function definition | ||||||||||||||||||||||||||||||||||||
if matches!( | ||||||||||||||||||||||||||||||||||||
binding.kind, | ||||||||||||||||||||||||||||||||||||
BindingKind::ClassDefinition(_) | BindingKind::FunctionDefinition(_) | ||||||||||||||||||||||||||||||||||||
) { | ||||||||||||||||||||||||||||||||||||
return true; | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
// Skip if module level definition | ||||||||||||||||||||||||||||||||||||
let Some(node_id) = binding.source else { | ||||||||||||||||||||||||||||||||||||
return true; | ||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||
let node = checker.semantic().node(node_id); | ||||||||||||||||||||||||||||||||||||
if let Some(Expr::Name(ast::ExprName { .. })) = node.as_expression() { | ||||||||||||||||||||||||||||||||||||
return true; | ||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||
false | ||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
--- | ||
source: crates/ruff_linter/src/rules/pylint/mod.rs | ||
--- | ||
global_variable_undefined.py:22:5: PLW0601 Global variable `SOMEVAR` is undefined at the module | ||
| | ||
20 | # BAD | ||
21 | def global_variable_undefined(): | ||
22 | global SOMEVAR # [global-variable-undefined] | ||
| ^^^^^^^^^^^^^^ PLW0601 | ||
23 | SOMEVAR = 2 | ||
| |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
--- | ||
source: crates/ruff_linter/src/rules/pylint/mod.rs | ||
--- |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove those comments because our tests run in isolation