[pylint] Implement global-variable-undefined (W0601) #10820

tibor-reiss · 2024-04-07T19:20:10Z

Add pylint rule global-variable-undefined (W0601)

See #970 for rules

Test Plan: cargo test

github-actions · 2024-04-07T19:50:16Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+4 -0 violations, +0 -0 fixes in 2 projects; 52 projects unchanged)

apache/airflow (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ tests/listeners/class_listener.py:39:13: PLW0601 Global variable `stopped_component` is undefined at the module
+ tests/listeners/class_listener.py:71:13: PLW0601 Global variable `stopped_component` is undefined at the module

bokeh/bokeh (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ examples/output/jupyter/push_notebook/Numba Image Example.ipynb:cell 18:2:5: PLW0601 Global variable `_last_kname` is undefined at the module
+ examples/output/jupyter/push_notebook/Numba Image Example.ipynb:cell 18:3:5: PLW0601 Global variable `_last_out` is undefined at the module

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PLW0601	4	4	0	0	0

tusharsadhwani · 2024-04-07T20:21:16Z

Does this break on wildcard imports?

from os import *

def foo():
    global system  # W0601 should not be raised
    system = lambda _: None

tibor-reiss · 2024-04-14T09:16:48Z

Does this break on wildcard imports?

from os import *

def foo():
    global system  # W0601 should not be raised
    system = lambda _: None

Yes, it does. I have not found a function in ruff which resolves star-imports, e.g. there is F403 and F405 which are raised for star-imports and warn about "...unable to detect..." and "...may be undefined...".

MichaReiser · 2024-04-15T07:10:21Z

I think there's a way to detect if there's a star import in which case you could avoid creating any diagnostics (to avoid false-positives).

The main open question to me are

how this fits into ruff long term when Ruff catches more static analyzable errors (undefined variables in general)
What's the motivation that this rule is specific to globals and doesn't apply to all variables.

tibor-reiss · 2024-04-15T18:57:32Z

@MichaReiser
Regarding star imports: when I do from os import *, pylint resolves this and e.g. has system in the namespace. Ruff is not able to do this (yet). For now, I simply left this out in this PR. What is possible though is to add something similar to F403/F405, i.e. if there is a start import, then raise "global variable undefined but maybe imported via star import". What's your preference?

MichaReiser · 2024-04-16T07:31:56Z

@tibor-reiss my preference would be that we don't flag any variables if a file contains a star import. I think there's already precedence for this in Ruff but I would need to search it to. @charliermarsh should know where to find it.

tibor-reiss · 2024-05-16T09:47:27Z

@MichaReiser thanks for feedback, I added the check for star imports together with a new test file.

tibor-reiss · 2024-09-20T17:46:31Z

Hi @MichaReiser, this got lost in translation. I have updated now to the latest master, could you please have a look and let me know if something is still missing?

MichaReiser · 2024-09-23T09:48:37Z

Thanks @tibor-reiss

I took a quick look at the ecosystem result and it's not clear why the rule raises

airflow/settings.py:441:5: PLW0601 Global variable Session is undefined at the module

Is it because there's only a declaration of Session

https://github.com/apache/airflow/blob/ee87fa0cba4d83084b4bc617d63d117101d9e069/airflow/settings.py#L99

crates/ruff_linter/src/codes.rs

MichaReiser · 2024-09-23T09:50:53Z

crates/ruff_linter/src/rules/pylint/rules/global_variable_undefined.rs

+    let Some(module_scope) = checker
+        .semantic()
+        .current_scopes()
+        .find(|scope| scope.kind.is_module())
+    else {
+        return;
+    };


Do you think we could use global_scope here instead?

Suggested change

let Some(module_scope) = checker

.semantic()

.current_scopes()

.find(|scope| scope.kind.is_module())

else {

return;

};

let module_scope = checker

.semantic()

.global_scope();

MichaReiser · 2024-09-23T09:51:53Z

crates/ruff_linter/src/rules/pylint/rules/global_variable_undefined.rs

+}
+
+fn get_imports<'a>(checker: &'a Checker) -> Vec<&'a str> {
+    // Get all names imported in the current scope


Why do we only look at imports of the current scope? What about imports in the enclosing scope?

This covers the case when the global statement is before the import statement - see tests. I additionally added the check for imports in is_global_binding.

Co-authored-by: Micha Reiser <[email protected]>

tibor-reiss · 2024-09-29T15:06:48Z

Thanks @tibor-reiss

I took a quick look at the ecosystem result and it's not clear why the rule raises
* [airflow/settings.py:441:5:](https://github.com/apache/airflow/blob/ee87fa0cba4d83084b4bc617d63d117101d9e069/airflow/settings.py#L441) PLW0601 Global variable `Session` is undefined at the module
Is it because there's only a declaration of Session

https://github.com/apache/airflow/blob/ee87fa0cba4d83084b4bc617d63d117101d9e069/airflow/settings.py#L99

You are right: there was a case missing, i.e. the global variable was just declared (with type annotation). I have added this case now (with test).

@MichaReiser what do you think about this case? I have it now covered. However, the rule reads undefined, so I think there is a strong case for raising the warning if the global was only declared.

MichaReiser · 2024-10-14T10:28:09Z

crates/ruff_linter/src/rules/pylint/rules/global_variable_undefined.rs

+    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![];
+    };


Nit:

Suggested change

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![];

};

let Some(ast::StmtFunctionDef { body, .. }) = checker

.semantic()

.current_scopes()

.find_map(|scope| scope.kind.as_function())

else {

return vec![];

};

MichaReiser · 2024-10-14T10:35:08Z

crates/ruff_linter/src/rules/pylint/rules/global_variable_undefined.rs

+    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());
+                }
+            }
+            _ => (),
+        }
+    }


Should this also respect imports in conditional branches. E.g. what about:

def test(a): if a: from b import bar globals bar

MichaReiser · 2024-10-14T12:28:37Z

crates/ruff_linter/resources/test/fixtures/pylint/global_variable_undefined.py

+# 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


We can remove those comments because our tests run in isolation

MichaReiser · 2024-10-14T12:30:26Z

crates/ruff_linter/src/rules/pylint/rules/global_variable_undefined.rs

+
+    for name in names {
+        // Skip if imported names
+        if imported_names.contains(&name.as_str()) {


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.

tibor-reiss force-pushed the add-W0601 branch 5 times, most recently from 181ccaa to 4ea1948 Compare April 7, 2024 19:36

MichaReiser added the rule Implementing or modifying a lint rule label Apr 15, 2024

tibor-reiss force-pushed the add-W0601 branch 2 times, most recently from 6d1d4ca to 68bb046 Compare May 16, 2024 09:37

tibor-reiss added 2 commits September 20, 2024 19:38

Add W0601

6181b39

Do not check star imports

5a61a5b

tibor-reiss force-pushed the add-W0601 branch from 68bb046 to 5a61a5b Compare September 20, 2024 17:38

MichaReiser reviewed Sep 23, 2024

View reviewed changes

Use suggestion from CR

91d15a7

Co-authored-by: Micha Reiser <[email protected]>

tibor-reiss force-pushed the add-W0601 branch from 1eed643 to f1dc57c Compare September 29, 2024 15:04

tibor-reiss force-pushed the add-W0601 branch 5 times, most recently from 56b56c6 to 7b9de3e Compare September 29, 2024 16:30

Fix for undefined global

3d4dfd2

tibor-reiss force-pushed the add-W0601 branch from 7b9de3e to 3d4dfd2 Compare September 29, 2024 17:28

Take care of all imports

adb7237

tibor-reiss requested a review from MichaReiser September 29, 2024 19:20

dhruvmanila added the preview Related to preview mode features label Oct 1, 2024

MichaReiser reviewed Oct 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pylint] Implement global-variable-undefined (W0601) #10820

[pylint] Implement global-variable-undefined (W0601) #10820

tibor-reiss commented Apr 7, 2024

github-actions bot commented Apr 7, 2024 •

edited

Loading

tusharsadhwani commented Apr 7, 2024

tibor-reiss commented Apr 14, 2024

MichaReiser commented Apr 15, 2024

tibor-reiss commented Apr 15, 2024

MichaReiser commented Apr 16, 2024

tibor-reiss commented May 16, 2024

tibor-reiss commented Sep 20, 2024

MichaReiser commented Sep 23, 2024

MichaReiser Sep 23, 2024

MichaReiser Sep 23, 2024

tibor-reiss Sep 29, 2024

tibor-reiss commented Sep 29, 2024 •

edited

Loading

MichaReiser Oct 14, 2024

MichaReiser Oct 14, 2024

MichaReiser Oct 14, 2024

MichaReiser Oct 14, 2024

[pylint] Implement global-variable-undefined (W0601) #10820

Are you sure you want to change the base?

[pylint] Implement global-variable-undefined (W0601) #10820

Conversation

tibor-reiss commented Apr 7, 2024

github-actions bot commented Apr 7, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

tusharsadhwani commented Apr 7, 2024

tibor-reiss commented Apr 14, 2024

MichaReiser commented Apr 15, 2024

tibor-reiss commented Apr 15, 2024

MichaReiser commented Apr 16, 2024

tibor-reiss commented May 16, 2024

tibor-reiss commented Sep 20, 2024

MichaReiser commented Sep 23, 2024

MichaReiser Sep 23, 2024

Choose a reason for hiding this comment

MichaReiser Sep 23, 2024

Choose a reason for hiding this comment

tibor-reiss Sep 29, 2024

Choose a reason for hiding this comment

tibor-reiss commented Sep 29, 2024 • edited Loading

MichaReiser Oct 14, 2024

Choose a reason for hiding this comment

MichaReiser Oct 14, 2024

Choose a reason for hiding this comment

MichaReiser Oct 14, 2024

Choose a reason for hiding this comment

MichaReiser Oct 14, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 7, 2024 •

edited

Loading

`ruff-ecosystem` results

tibor-reiss commented Sep 29, 2024 •

edited

Loading