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

Improve handling of metaclasses in various linter rules #12579

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

AlexWaygood
Copy link
Member

Summary

This PR does several interrelated things:

  1. The first thing it does is move an is_metaclass function out of crates/ruff_linter/src/rules/flake8_pyi/rules/non_self_return_type.rs and into the ruff_python_semantic crate. It's a generally useful function, and we were doing similar analysis elsewhere (but we were doing it incorrectly). It's better to centralize it in one place.
  2. The second thing it does is that it changes the classify() function in ruff_python_semantic::analyze::function_type so that it no longer reports that metaclass instance methods are classmethods. For our pep8-naming rules, it makes sense to treat metaclass instance methods like classmethods, since metaclass instance methods usually use cls for the name of the first parameter, like classmethods on non-metaclasses. However, the classify() function is used by many other linter rules, not just our pep8-naming rules. For those rules, it doesn't make sense for metaclass instance methods to be considered classmethods, since, well, they're not. An example is B019, which I've added a test for in this PR: I'm not sure it makes sense to treat instance methods on metaclasses any differently to instance methods on regular classes for that rule.
  3. Since ruff_python_semantic::analyze::function_type::classify() no longer considers instance methods on metaclasses to be classmethods, I adjusted the pep8-naming rules so that they now do their own check to see whether an instance method is a method on a metaclass, in order to determine what the name of the first parameter should be.

FWIW, there were several buggy things in the check for metaclasses that ruff_python_semantic::analyze::function_type::classify() was doing:

  • It was using map_callable() when it should have been using map_subscript()
  • It was missing the enum-module metaclasses that the stdlib provides
  • It was only looking at the direct bases on the class, instead of iterating up through the bases of all superclasses

Test Plan

cargo test -p ruff_linter

@AlexWaygood AlexWaygood added the linter Related to the linter label Jul 30, 2024
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

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

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

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

+ airflow/stats.py:46:24: ARG002 Unused method argument: `args`
- airflow/stats.py:46:24: ARG003 Unused class method argument: `args`
+ airflow/stats.py:46:32: ARG002 Unused method argument: `kwargs`
- airflow/stats.py:46:32: ARG003 Unused class method argument: `kwargs`
+ airflow/traces/tracer.py:259:24: ARG002 Unused method argument: `args`
- airflow/traces/tracer.py:259:24: ARG003 Unused class method argument: `args`
+ airflow/traces/tracer.py:259:32: ARG002 Unused method argument: `kwargs`
- airflow/traces/tracer.py:259:32: ARG003 Unused class method argument: `kwargs`

Changes by rule (2 rules affected)

code total + violation - violation + fix - fix
ARG002 4 4 0 0 0
ARG003 4 0 4 0 0

Linter (preview)

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

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

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

+ airflow/stats.py:46:24: ARG002 Unused method argument: `args`
- airflow/stats.py:46:24: ARG003 Unused class method argument: `args`
+ airflow/stats.py:46:32: ARG002 Unused method argument: `kwargs`
- airflow/stats.py:46:32: ARG003 Unused class method argument: `kwargs`
+ airflow/traces/tracer.py:259:24: ARG002 Unused method argument: `args`
- airflow/traces/tracer.py:259:24: ARG003 Unused class method argument: `args`
+ airflow/traces/tracer.py:259:32: ARG002 Unused method argument: `kwargs`
- airflow/traces/tracer.py:259:32: ARG003 Unused class method argument: `kwargs`

Changes by rule (2 rules affected)

code total + violation - violation + fix - fix
ARG002 4 4 0 0 0
ARG003 4 0 4 0 0

@AlexWaygood
Copy link
Member Author

The ecosystem changes look good, in my opinion. I think those are all improvements in error messages.

@AlexWaygood AlexWaygood merged commit 7a4419a into main Jul 30, 2024
20 checks passed
@AlexWaygood AlexWaygood deleted the alex/is-metaclass-centralize branch July 30, 2024 13:48
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linter Related to the linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants