Skip to content

[red-knot] fix inheritance-cycle detection for generic classes#17620

Merged
carljm merged 1 commit intomainfrom
cjm/gencyclemro
Apr 25, 2025
Merged

[red-knot] fix inheritance-cycle detection for generic classes#17620
carljm merged 1 commit intomainfrom
cjm/gencyclemro

Conversation

@carljm
Copy link
Contributor

@carljm carljm commented Apr 25, 2025

Summary

The ClassLiteralType::inheritance_cycle method is intended to detect inheritance cycles that would result in cyclic MROs, emit a diagnostic, and skip actually trying to create the cyclic MRO, falling back to an "error" MRO instead with just Unknown and object.

This method didn't work properly for generic classes. It used fully_static_explicit_bases, which filter-maps explicit_bases over Type::into_class_type, which returns None for an unspecialized generic class literal. So in a case like class C[T](C): ..., because the explicit base is an unspecialized generic, we just skipped it, and failed to detect the class as cyclically defined.

Instead, iterate directly over all explicit_bases, and explicitly handle both the specialized (GenericAlias) and unspecialized (ClassLiteral) cases, so that we check all bases and correctly detect cyclic inheritance.

Test Plan

Added mdtests.

@carljm carljm added the ty Multi-file analysis & type inference label Apr 25, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 25, 2025

mypy_primer results

No ecosystem changes detected ✅

@carljm carljm mentioned this pull request Apr 25, 2025
Merged
@carljm carljm merged commit 4c3f389 into main Apr 25, 2025
34 checks passed
@carljm carljm deleted the cjm/gencyclemro branch April 25, 2025 13:55
carljm added a commit that referenced this pull request Apr 25, 2025
@carljm
[red-knot] fix detecting a metaclass on a not-explicitly-specialized …
fa88989 
…generic base (#17621)

## Summary

After #17620 (which this PR is
based on), I was looking at other call sites of `Type::into_class_type`,
and I began to feel that _all_ of them were currently buggy due to
silently skipping unspecialized generic class literal types (though in
some cases the bug hadn't shown up yet because we don't understand
legacy generic classes from typeshed), and in every case they would be
better off if an unspecialized generic class literal were implicitly
specialized with the default specialization (which is the usual Python
typing semantics for an unspecialized reference to a generic class),
instead of silently skipped.

So I changed the method to implicitly apply the default specialization,
and added a test that previously failed for detecting metaclasses on an
unspecialized generic base.

I also renamed the method to `to_class_type`, because I feel we have a
strong naming convention where `Type::into_foo` is always a trivial
`const fn` that simply returns `Some()` if the type is of variant `Foo`
and `None` otherwise. Even the existing method (with it handling both
`GenericAlias` and `ClassLiteral`, and distinguishing kinds of
`ClassLiteral`) was stretching this convention, and the new version
definitely breaks that envelope.

## Test Plan

Added a test that failed before this PR.
dcreager
dcreager reviewed Apr 25, 2025
View reviewed changes
Copy link
Member

@dcreager dcreager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this! I like how simple the fix turned out to be

dcreager added a commit that referenced this pull request Apr 25, 2025
@dcreager
Merge branch 'main' into dcreager/lazy-generic-class
67e5888 
* main: (24 commits)
  [`airflow`] Extend `AIR301` rule (#17598)
  [`airflow`] update existing `AIR302` rules with better suggestions (#17542)
  red_knot_project: sort diagnostics from checking files
  [red-knot] fix detecting a metaclass on a not-explicitly-specialized generic base (#17621)
  [red-knot] fix inheritance-cycle detection for generic classes (#17620)
  [`pylint`] Detect `global` declarations in module scope (`PLE0118`) (#17411)
  Add Semantic Error Test for LateFutureImport (#17612)
  [red-knot] change TypeVarInstance to be interned, not tracked (#17616)
  [red-knot] Special case `@final`, `@override` (#17608)
  [red-knot] add TODO comment in specialization code (#17615)
  [`semantic-syntax-errors`] test for `LoadBeforeGlobalDeclaration` - ruff linter (#17592)
  [syntax-errors] `nonlocal` declaration at module level (#17559)
  [`airflow`] Apply auto fix to cases where name has been changed in Airflow 3 (`AIR311`) (#17571)
  [syntax-errors] Make `async-comprehension-in-sync-comprehension` more specific (#17460)
  Bump 0.11.7 (#17613)
  [red-knot] Use iterative approach to collect overloads (#17607)
  red_knot_python_semantic: avoid Rust's screaming snake case convention in mdtest
  red_knot_python_semantic: improve diagnostics for unsupported boolean conversions
  red_knot_python_semantic: add "return type span" helper method
  red_knot_python_semantic: move parameter span helper method
  ...
dcreager added a commit that referenced this pull request Apr 26, 2025
@dcreager
Merge branch 'main' into dcreager/legacy-typevar-instance
fb51439 
* main: (27 commits)
  [red-knot] Add new property tests for subtyping with "bottom" callable (#17635)
  [red-knot] Create generic context for generic classes lazily (#17617)
  ruff_db: add tests for annotations with no ranges
  [`airflow`] Extend `AIR301` rule (#17598)
  [`airflow`] update existing `AIR302` rules with better suggestions (#17542)
  red_knot_project: sort diagnostics from checking files
  [red-knot] fix detecting a metaclass on a not-explicitly-specialized generic base (#17621)
  [red-knot] fix inheritance-cycle detection for generic classes (#17620)
  [`pylint`] Detect `global` declarations in module scope (`PLE0118`) (#17411)
  Add Semantic Error Test for LateFutureImport (#17612)
  [red-knot] change TypeVarInstance to be interned, not tracked (#17616)
  [red-knot] Special case `@final`, `@override` (#17608)
  [red-knot] add TODO comment in specialization code (#17615)
  [`semantic-syntax-errors`] test for `LoadBeforeGlobalDeclaration` - ruff linter (#17592)
  [syntax-errors] `nonlocal` declaration at module level (#17559)
  [`airflow`] Apply auto fix to cases where name has been changed in Airflow 3 (`AIR311`) (#17571)
  [syntax-errors] Make `async-comprehension-in-sync-comprehension` more specific (#17460)
  Bump 0.11.7 (#17613)
  [red-knot] Use iterative approach to collect overloads (#17607)
  red_knot_python_semantic: avoid Rust's screaming snake case convention in mdtest
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dcreager dcreager dcreager left review comments

@AlexWaygood AlexWaygood AlexWaygood approved these changes

@sharkdp sharkdp Awaiting requested review from sharkdp

Assignees

No one assigned

Labels

ty Multi-file analysis & type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@carljm @dcreager @AlexWaygood