Skip to content

Comments

[ty] Respect kw_only from parent class#21820

Merged
sharkdp merged 2 commits intomainfrom
charlie/own
Dec 10, 2025
Merged

[ty] Respect kw_only from parent class#21820
sharkdp merged 2 commits intomainfrom
charlie/own

Conversation

@charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Dec 6, 2025

Summary

Closes astral-sh/ty#1769.

@astral-sh-bot
Copy link

astral-sh-bot bot commented Dec 6, 2025 •
edited
Loading

Diagnostic diff on typing conformance tests

Changes were detected when running ty on typing conformance tests 
--- old-output.txt	2025-12-08 22:08:55.843892827 +0000
+++ new-output.txt	2025-12-08 22:08:59.562927816 +0000
@@ -285,9 +285,6 @@
 dataclasses_kwonly.py:23:11: error[too-many-positional-arguments] Too many positional arguments: expected 1, got 2
 dataclasses_kwonly.py:38:11: error[too-many-positional-arguments] Too many positional arguments: expected 1, got 2
 dataclasses_kwonly.py:53:11: error[too-many-positional-arguments] Too many positional arguments: expected 1, got 2
-dataclasses_kwonly.py:61:1: error[missing-argument] No argument provided for required parameter `c`
-dataclasses_kwonly.py:61:9: error[invalid-argument-type] Argument is incorrect: Expected `int`, found `float`
-dataclasses_kwonly.py:61:14: error[parameter-already-assigned] Multiple values provided for parameter `b`
 dataclasses_match_args.py:42:1: error[unresolved-attribute] Class `DC4` has no attribute `__match_args__`
 dataclasses_match_args.py:49:1: error[type-assertion-failure] Type `tuple[()]` does not match asserted type `Unknown | tuple[()]`
 dataclasses_order.py:50:4: error[unsupported-operator] Operator `<` is not supported between objects of type `DC1` and `DC2`
@@ -1030,4 +1027,4 @@
 typeddicts_usage.py:28:17: error[missing-typed-dict-key] Missing required key 'name' in TypedDict `Movie` constructor
 typeddicts_usage.py:28:18: error[invalid-key] Unknown key "title" for TypedDict `Movie`: Unknown key "title"
 typeddicts_usage.py:40:24: error[invalid-type-form] The special form `typing.TypedDict` is not allowed in type expressions
-Found 1032 diagnostics
+Found 1029 diagnostics

@astral-sh-bot
Copy link

astral-sh-bot bot commented Dec 6, 2025 •
edited
Loading

mypy_primer results

Changes were detected when running on open source projects 
beartype (https://github.com/beartype/beartype)
+ beartype/claw/_package/clawpkgtrie.py:66:29: warning[unsupported-base] Unsupported class base with type `<class 'dict[str, PackagesTrieBlacklist]'> | <class 'dict[str, Divergent]'>`
+ beartype/claw/_package/clawpkgtrie.py:247:29: warning[unsupported-base] Unsupported class base with type `<class 'dict[str, PackagesTrieWhitelist]'> | <class 'dict[str, Divergent]'>`
- Found 492 diagnostics
+ Found 494 diagnostics

scikit-build-core (https://github.com/scikit-build/scikit-build-core)
+ src/scikit_build_core/build/_pathutil.py:25:38: error[invalid-argument-type] Argument to function `__new__` is incorrect: Expected `str | PathLike[str]`, found `DirEntry[Path]`
+ src/scikit_build_core/build/_pathutil.py:27:24: error[invalid-argument-type] Argument to function `__new__` is incorrect: Expected `str | PathLike[str]`, found `DirEntry[Path]`
+ src/scikit_build_core/build/wheel.py:98:20: error[no-matching-overload] No overload of bound method `__init__` matches arguments
- Found 39 diagnostics
+ Found 42 diagnostics

No memory usage changes detected ✅

@charliermarsh
Copy link
Member Author

charliermarsh commented Dec 6, 2025

I believe the conformance tests are right.

@AlexWaygood AlexWaygood added bug Something isn't working ty Multi-file analysis & type inference labels Dec 6, 2025
@AlexWaygood AlexWaygood changed the title Respect kw_only from parent class [ty] Respect kw_only from parent class Dec 6, 2025
@AlexWaygood
Copy link
Member

AlexWaygood commented Dec 7, 2025

I believe the conformance tests are right.

yup!

@AlexWaygood
Copy link
Member

AlexWaygood commented Dec 7, 2025

I'll let @sharkdp review this one, as our dataclasses/dataclass_transform expert

@AlexWaygood AlexWaygood removed their request for review December 7, 2025 13:42
@charliermarsh
Copy link
Member Author

charliermarsh commented Dec 7, 2025

I find the fix here pretty awkward (given that kw_only is optional and we're now setting it to always-Some) so I suspect it doesn't quite fit the intended design.

@carljm
Copy link
Contributor

carljm commented Dec 8, 2025 •
edited
Loading

I think you're right that this fix is awkward; took a look at why and I think it's mostly existing awkwardness / lack of support. I pushed a few TODO comments and one code change, since it was easier than describing in a review.

I think we never properly added support for default_* params on base-class and metaclass-based transformers, but some contributors added partial support when synthesizing __init__. Instead of that partial support, we should either have generalized methods for querying dataclass params (from anywhere) which always fall back to the dataclass-transformer-params, or we should ensure that dataclass_params on the class itself are always already initialized with values from the transformer param defaults. (It looks like this currently happens for function dataclass transforms but not for metaclass or base-class ones.) It also looks like we store dataclass_transformer_params on the class, but this also isn't currently set for metaclass or base-class transformers. So this special-cased support for base-class and metaclass transformers that was added to __init__ synthesis ought to be generalized, and if that happened this PR wouldn't need to explicitly check the transformer params.

Otherwise this fix looks fine to me. The ref mut stuff is a little awkward but that's just a local artifact of how own_fields is currently structured. I'd be fine landing this with the TODO comments I added.

Maybe ideally we'd also make kw_only on a field not optional in this PR, since now we are always setting it to Some. The only reason it was previously optional was so that __init__ synthesis could revisit the question, but that shouldn't really be needed (and isn't after this PR.) I removed one bit of code in the __init__ synthesis which becomes unnecessary with this fix in own_fields.

@sharkdp sharkdp mentioned this pull request Dec 10, 2025
Open
@sharkdp
Copy link
Contributor

sharkdp commented Dec 10, 2025

I am merging this based on Carl's review. I have opened astral-sh/ty#1835 to track the follow up tasks here (thank you for describing them in detail — I very much agree!).

@sharkdp sharkdp merged commit d2aabea into main Dec 10, 2025
41 checks passed
@sharkdp sharkdp deleted the charlie/own branch December 10, 2025 09:12
@thejchap thejchap mentioned this pull request Dec 11, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm approved these changes

@sharkdp sharkdp Awaiting requested review from sharkdp sharkdp is a code owner

@dcreager dcreager Awaiting requested review from dcreager dcreager is a code owner

Assignees

No one assigned

Labels

bug Something isn't working ty Multi-file analysis & type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Incorrect handling of dataclass kw_only with inheritance

4 participants

@charliermarsh @AlexWaygood @carljm @sharkdp