Skip to content

[red-knot] Handle generic constructors of generic classes#17552

Merged
dcreager merged 8 commits intomainfrom
dcreager/generic-constructor
Apr 23, 2025
Merged

[red-knot] Handle generic constructors of generic classes#17552
dcreager merged 8 commits intomainfrom
dcreager/generic-constructor

Conversation

@dcreager
Copy link
Member

We now handle generic constructor methods on generic classes correctly:

class C[T]:
    def __init__[S](self, t: T, s: S): ...

x = C(1, "str")

Here, constructing C requires us to infer a specialization for the generic contexts of C and __init__ at the same time.

At first I thought I would need to track the full stack of nested generic contexts here (since the [S] context is nested within the [T] context). But I think this is the only way that we might need to specialize more than one generic context at once — in all other cases, a containing generic context must be specialized before we get to a nested one, and so we can just special-case this.

While we're here, we also construct the generic context for a generic function lazily, when its signature is accessed, instead of eagerly when inferring the function body.

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

github-actions bot commented Apr 22, 2025

mypy_primer results

No ecosystem changes detected ✅

* main: (28 commits)
  [red-knot] Make `BoundMethodType` a salsa interned (#17581)
  [red-knot] Emit a diagnostic if a non-protocol is passed to `get_protocol_members` (#17551)
  [red-knot] Add more tests for protocol members (#17550)
  [red-knot] Assignability for subclasses of `Any` and `Unknown` (#17557)
  [red-knot] mypy_primer: add strawberry, print compilation errors to stderr (#17578)
  [red-knot] GenericAlias instances as a base class (#17575)
  Remove redundant `type_to_visitor_function` entries (#17564)
  Fixes how the checker visits `typing.cast`/`typing.NewType` arguments (#17538)
  [red-knot] Class literal `__new__` function callable subtyping (#17533)
  [red-knot] Surround intersections with `()` in potentially ambiguous contexts (#17568)
  [minor] Delete outdated TODO comment (#17565)
  [red-knot] add regression test for fixed cycle panic (#17535)
  [red-knot] fix unions of literals, again (#17534)
  red_knot_python_semantic: remove last vestige of old diagnostics!
  red_knot_python_semantic: migrate `types` to new diagnostics
  red_knot_python_semantic: migrate `types/diagnostic` to new diagnostics
  red_knot_python_semantic: migrate `types/call/bind` to new diagnostics
  red_knot_python_semantic: migrate `types/string_annotation` to new diagnostics
  red_knot_python_semantic: migrate `types/infer` to new diagnostic model
  red_knot_python_semantic: migrate INVALID_ASSIGNMENT for inference
  ...
}

#[salsa::interned(debug)]
pub struct FunctionLiteral<'db> {
Copy link
Member Author

Choose a reason for hiding this comment

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

@sharkdp This is where I pulled function literals out into "an enum"...though I ended up not needing any new variants for this PR, so it's still technically a struct. At the point where we need to introduce synthetic function literals, this is where they would go. (And this struct would become one of the variants.)

I went ahead and did this separation here even though I don't need an additional variant to clearly separate the parts apply to a function literal that appears in the source, and the generic-related stuff that comes from inheriting/specializing (which would apply to synthetic function literals once we have them, too)

Copy link
Member Author

@dcreager dcreager Apr 23, 2025

Choose a reason for hiding this comment

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

This causes a stack overflow in the ecosystem check, and I don't want to hold up this PR on debugging that, since this change is entirely orthogonal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the reference anyway! I guess we can still look at your changes in 43ce8d5 if we want to go back.

…tructor

* origin/main:
  [red-knot] Trust module-level undeclared symbols in stubs (#17577)
  [`airflow`] Apply auto fixes to cases where the names have changed in Airflow 3 (`AIR301`) (#17355)
  [`pycodestyle`] Auto-fix redundant boolean comparison (`E712`) (#17090)
  [red-knot] Detect semantic syntax errors (#17463)
  Fix stale diagnostics in Ruff playground (#17583)
  [red-knot] Early return from `project.is_file_open` for vendored files (#17580)
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Looks great! No notes :) (edit: "narrator: he had one note")

db: &'db dyn Db,
params: DataclassTransformerParams,
) -> Self {
Self::new(
Copy link
Contributor

Choose a reason for hiding this comment

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

Reasonable to debug-assert here that it's None, or is this not an invariant in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's an invariant, since then a user could trigger a panic by applying dataclass_transform twice. That could be a diagnostic we want to enforce, but shouldn't be an assertion. (Inheriting a generic context at most once is a proper invariant, since it's not possible for a user to declare a function "doubly" generic)

@dcreager dcreager merged commit 9db63fc into main Apr 23, 2025
34 checks passed
@dcreager dcreager deleted the dcreager/generic-constructor branch April 23, 2025 19:06
dcreager added a commit that referenced this pull request Apr 23, 2025
* main:
  [red-knot] Handle generic constructors of generic classes (#17552)
  [red-knot] Assignability of class instances to Callable (#17590)
  [red-knot] Trust all symbols in stub files (#17588)
Comment on lines +427 to +429
overload.set_return_type(Type::FunctionLiteral(
function.with_dataclass_transformer_params(db, params),
));
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants