[ruff] Abstract method defined in normal class (RUF044)

[InSyncWithFoo]

Summary

Resolves #12861.

Test Plan

cargo nextest run and cargo insta test.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

RasaHQ/rasa (+1 -0 violations, +0 -0 fixes)

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

+ rasa/core/featurizers/tracker_featurizers.py:253:9: RUF044 Abstract method defined in non-abstract class

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

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

+ airflow/auth/managers/models/base_user.py:27:9: RUF044 Abstract method defined in non-abstract class
+ airflow/auth/managers/models/base_user.py:30:9: RUF044 Abstract method defined in non-abstract class
+ airflow/utils/log/logging_mixin.py:138:9: RUF044 Abstract method defined in non-abstract class
+ airflow/utils/log/logging_mixin.py:142:9: RUF044 Abstract method defined in non-abstract class
+ airflow/utils/log/logging_mixin.py:147:9: RUF044 Abstract method defined in non-abstract class
+ dev/breeze/src/airflow_breeze/utils/cdxgen.py:332:9: RUF044 Abstract method defined in non-abstract class
+ dev/breeze/src/airflow_breeze/utils/cdxgen.py:336:9: RUF044 Abstract method defined in non-abstract class
+ providers/tests/amazon/aws/links/test_base_aws.py:243:9: RUF044 Abstract method defined in non-abstract class
+ task_sdk/src/airflow/sdk/definitions/mixins.py:53:9: RUF044 Abstract method defined in non-abstract class
+ task_sdk/src/airflow/sdk/definitions/mixins.py:60:9: RUF044 Abstract method defined in non-abstract class

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

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

+ superset/commands/database/uploaders/base.py:74:9: RUF044 Abstract method defined in non-abstract class
+ superset/commands/database/uploaders/base.py:77:9: RUF044 Abstract method defined in non-abstract class

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

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

+ pymilvus/client/abstract.py:843:9: RUF044 Abstract method defined in non-abstract class
+ pymilvus/client/asynch.py:29:9: RUF044 Abstract method defined in non-abstract class
+ pymilvus/client/asynch.py:40:9: RUF044 Abstract method defined in non-abstract class
+ pymilvus/client/asynch.py:48:9: RUF044 Abstract method defined in non-abstract class
+ pymilvus/client/asynch.py:85:9: RUF044 Abstract method defined in non-abstract class

pandas-dev/pandas (+1 -0 violations, +0 -0 fixes)

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

+ pandas/io/formats/info.py:589:9: RUF044 Abstract method defined in non-abstract class

python-poetry/poetry (+1 -0 violations, +0 -0 fixes)

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

+ src/poetry/plugins/base_plugin.py:17:9: RUF044 Abstract method defined in non-abstract class

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

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

+ rotkehlchen/exchanges/exchange.py:56:9: RUF044 Abstract method defined in non-abstract class
+ rotkehlchen/utils/interfaces.py:107:9: RUF044 Abstract method defined in non-abstract class

zulip/zulip (+1 -0 violations, +0 -0 fixes)

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

+ zproject/backends.py:2560:9: RUF044 Abstract method defined in non-abstract class

astropy/astropy (+3 -0 violations, +0 -0 fixes)

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

+ astropy/cosmology/tests/test_core.py:430:9: RUF044 Abstract method defined in non-abstract class
+ astropy/modeling/fitting.py:365:9: RUF044 Abstract method defined in non-abstract class
+ astropy/timeseries/periodograms/base.py:13:9: RUF044 Abstract method defined in non-abstract class

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
RUF044	26	26	0	0	0

[InSyncWithFoo]

@MichaReiser The function cannot be called is_metaclass_abcmeta because we do not have access to enough information to determine if that is the case. We can only tell if a class "might be" or "is definitely not" abc.ABCMeta. Same for is_abstract_class. I'm sure I have explained this somewhat in the doc comments. See also typing::might_be_generic.

[MichaReiser]

@InSyncWithFoo what's an example where is_metaclass_abcmeta or is_abstract_class returns true where the class isn't a abcmeta or an abstract class?

if there are indeed cases where some of those methods return false when the class isn't abstract, then it's important that we document these as part of the rule.

[InSyncWithFoo]

A few examples:

from abc import ABCMeta
from somewhere import SomeMetaclass  # Could be subclass of ABCMeta, for all we know.

# `B` might be abstract (methods are not checked).
class A(ABCMeta): ...
class B(metaclass=A): ...

# Is `C` abstract or not?
class C(metaclass=SomeMetaclass): ...

# `D` and `E` definitely aren't abstract.
class D: ...
class E(D): ...

[MichaReiser]

Thanks for the example. I feel like we can keep using is_metacalss_abcmeta and add a documentation that clarifies that:

• It only returns true for classes that are guaranteed to be meta classes
• It may return false for classes that are meta classes but Ruff can't detect

[InSyncWithFoo]

Here's a table:

Classes	Is abstract?	might_be_abstract()
class A(ABC)	Yes	true
class B(A), class A(ABC)	Yes	true
class A(metaclass=ABCMeta)	Yes	true
class A(Unknown)	Maybe	true
class A(metaclass=Unknown)	Maybe	true
class A	No	false
class B(A), class A	No	false
class B(metaclass=A), class A	No	false

might_be_abstract() returns false when it is possible to determine that the given class is not abstract. It returns true otherwise (when the class is known to be abstract, or when there are not enough information). Same for metaclass_might_be_abcmeta().

[MichaReiser]

Thanks for the table. I see. I'm then leaning towards simply calling it is_not_abstract.

I also noticed that we have B024 which checks the opposite of this rule. We should make sure that the two rules are consistent in how they detect abstract classes.

ruff/crates/ruff_linter/src/rules/flake8_bugbear/rules/abstract_base_class.rs

Lines 116 to 129 in 14ba469

	fn is_abc_class(bases: &[Expr], keywords: &[Keyword], semantic: &SemanticModel) -> bool {
	keywords.iter().any(|keyword| {
	keyword.arg.as_ref().is_some_and(|arg| arg == "metaclass")
	&& semantic
	.resolve_qualified_name(&keyword.value)
	.is_some_and(|qualified_name| {
	matches!(qualified_name.segments(), ["abc", "ABCMeta"])
	})
	}) || bases.iter().any(|base| {
	semantic
	.resolve_qualified_name(base)
	.is_some_and(|qualified_name| matches!(qualified_name.segments(), ["abc", "ABC"]))
	})
	}

[InSyncWithFoo]

@MichaReiser is_not_abstract has the "double negative" problem:

// This doesn't read well in English: "If *not* `class` is *not* abstract, bail."
if !is_not_abstract(class, checker.semantic()) {
    return;
}

// This reads better: "If `class` might be abstract, bail."
if might_be_abstract(class, checker.semantic()) {
    return;
}

As for B024 (and B027), I can submit another PR, but I don't think that alone should block this PR, especially when the existing logic was taken dirrectly from upstream.

[MichaReiser]

@MichaReiser is_not_abstract has the "double negative" problem:

That's fair. My only concern is that might is a bit hand-wavey. It's unclear if it classifies classes as abstract that aren't and/or classes as not abstract that are. The name suggests to me that it could have false positives as well as false negatives. The is_not_abstract is more explicit about it. The name suggests that it never returns true for a class that might be abstract. We can also try to address this in another way. For example by returning an enum with variant names, although I'm not sure how to name the variants to make it clear.

As for B024 (and B027), I can submit another PR, but I don't think that alone should block this PR, especially when the existing logic was taken dirrectly from upstream.

I'm not sure if we want to update B024, instead I think it would be better to align this rule with what's done in B024 and, if we can, avoid any unnecessary code duplication.

[InSyncWithFoo]

@MichaReiser

• B024 does not check for subclasses of ABCMeta; RUF044 currently does.
• B024 only reports when the class is definitely known to be abstract; RUF044 currently only reports when the class is known to not be abstract.
• B024 wants to avoid classes that do not inherit directly from ABC or has metaclass=ABCMeta, as such a class might not be abstract.
• RUF044 wants to avoid classes that have an ABC base or an ABCMeta metaclass, as such a class might be abstract.

Visualization:

         `is_abc_class() == true`
                    |             `might_be_abstract() == true`
                    /-------------------------------------------------------\
Abstractness:   Abstract        Likely abstract        Unknown      Likely not abstract    Not abstract
                    |------------------|------------------|------------------|------------------|
Requirements:  B024, B027                                                                     RUF044

Are you sure that you want RUF044 to use existing logic from B024 when the requirements are so significantly different?

[MichaReiser]

To me the two rules must be consistent in their behavior. That means they should both respect ABCMeta or not. For now, I suggest to ignore ABCMeta and to create a separate PR that adds support for ABCMeta to both rules. Unless there are specific reasons why B024 doesn't support ABCMeta that I'm overlooking.

Whether both rules use the exact same code is less important but it would be nice if they can share most of the: Does this class implement ABCMeta logic.

[InSyncWithFoo]

That B024 does not recognize subclasses of ABCMeta is a different matter, to be addressed in another PR. I can submit that PR tomorrow if you want (reusing all the code from this PR), but I don't see why I have to worsen RUF044's logic to match that, especially when it will be readded again anyway.

B024 wants only direct subclasses of ABC or classes that have (a subclass of) ABCMeta as the metaclass. RUF044 wants classes with no ABC or ABCMeta anywhere in their and their metaclasses' MROs.

[MichaReiser]

My main concern is that this PR adds more code to identify abstract classes that are hidden away in the rule implementation and isn't shared with the logic that other rules use. I'm okay looking over the ABCMeta difference if we plan on addressing that later, as long as my main concern is addressed