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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tibor-reiss
Copy link
Contributor

Add pylint rule global-variable-undefined (W0601)

See #970 for rules

Test Plan: cargo test

@tibor-reiss tibor-reiss force-pushed the add-W0601 branch 4 times, most recently from fa06883 to 181ccaa Compare April 7, 2024 19:34
Copy link

github-actions bot commented Apr 7, 2024

ruff-ecosystem results

Linter (stable)

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

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

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

+ airflow/serialization/serialized_objects.py:1675:5: PLW0601 Global variable `HAS_KUBERNETES` is undefined at the module
+ airflow/settings.py:243:5: PLW0601 Global variable `Session` is undefined at the module
+ airflow/settings.py:244:5: PLW0601 Global variable `engine` is undefined at the module
+ airflow/settings.py:364:5: PLW0601 Global variable `engine` is undefined at the module
+ airflow/settings.py:365:5: PLW0601 Global variable `Session` is undefined at the module
+ airflow/settings.py:407:5: PLW0601 Global variable `engine` is undefined at the module
+ tests/executors/test_executor_loader.py:54:9: PLW0601 Global variable `ExecutorLoader` is undefined at the module
+ tests/listeners/class_listener.py:37:9: PLW0601 Global variable `stopped_component` is undefined at the module

milvus-io/pymilvus (+1 -0 violations, +0 -0 fixes)

+ _version_helper.py:92:9: PLW0601 Global variable `version` is undefined at the module

Changes by rule (1 rules affected)

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

Linter (preview)

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

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

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

+ airflow/serialization/serialized_objects.py:1675:5: PLW0601 Global variable `HAS_KUBERNETES` is undefined at the module
+ airflow/settings.py:243:5: PLW0601 Global variable `Session` is undefined at the module
+ airflow/settings.py:244:5: PLW0601 Global variable `engine` is undefined at the module
+ airflow/settings.py:364:5: PLW0601 Global variable `engine` is undefined at the module
+ airflow/settings.py:365:5: PLW0601 Global variable `Session` is undefined at the module
+ airflow/settings.py:407:5: PLW0601 Global variable `engine` is undefined at the module
+ tests/executors/test_executor_loader.py:54:9: PLW0601 Global variable `ExecutorLoader` is undefined at the module
+ tests/listeners/class_listener.py:37:9: PLW0601 Global variable `stopped_component` is undefined at the module

milvus-io/pymilvus (+1 -0 violations, +0 -0 fixes)

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

+ _version_helper.py:92:9: PLW0601 Global variable `version` is undefined at the module

Changes by rule (1 rules affected)

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

@tusharsadhwani
Copy link
Contributor

Does this break on wildcard imports?

from os import *

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

@tibor-reiss
Copy link
Contributor Author

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
Copy link
Member

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.

@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Apr 15, 2024
@tibor-reiss
Copy link
Contributor Author

@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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants