Fix dataclass/BaseModel field named "self" incorrectly flagged as duplicate (issue #3900)#3902
Conversation
A dataclass field named `self` collided with the implicit `self` parameter of the synthesized `__init__`, producing false bad-keyword- argument and bad-argument-type errors on "Cls(self=...)". Instance receiver is bound by prepending it as a positional argument rather than dropping the param so two params named `self` caused the keyword "self=" to bind to the instance param instead of the field. Fix is to mirror CPython dataclasses as follows: when there exists a field named "self", name the instance param "dataclass_self__" instead. Pydantic BaseModel also covered by this change since its init goes through get_dataclass_init Fixes issue facebook#3900 + dillydill123's comment about Pydantic BaseModel
|
Hi @kz357! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
force-with-lease pushed user email change to mine, because I think meta CLA needs it to be my email (?) |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
yangdanny97
left a comment
yangdanny97
left a comment
There was a problem hiding this comment.
LGTM, I'll let @NathanTempest merge
NathanTempest
left a comment
NathanTempest
left a comment
There was a problem hiding this comment.
Thanks for the great fix, @kz357 and welcome aboard!. LGTM. One micro-nit: the new comment is a doc comment but it's inside a function body which is only valid before items, so it'll trigger an unused_doc_comment warning. I will import this and change it myself as this is a pretty small fix.
|
@NathanTempest has imported this pull request. If you are a Meta employee, you can view this in D109467138. |
rchen152
left a comment
rchen152
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
@NathanTempest merged this pull request in d781e76. |
4 participants
Summary
A "@DataClass" (or Pydantic "BaseModel") field named "self" was incorrectly flagged with "bad-keyword-argument" and "bad-argument-type" errors on construction, e.g. "MyDataClass(self="test")", even though this is valid Python that runs fine.
Instance receiver is bound by prepending it as a positional argument rather than dropping the param so two params named
selfcaused the keyword "self=" to bind to the instance param instead of the field.Fix is to mirror CPython dataclasses as follows: when there exists a field named "self", name the instance param "dataclass_self" instead. Pydantic BaseModel also covered by this change since its init goes through get_dataclass_init.
Fixes issue #3900 + dillydill123's comment about Pydantic BaseModel
Test Plan
Added two regression tests, one for dataclass and one for BaseModel, both passing.
Both construct instances via "self=" keyword and assert field type with "assert_type". Both reproduce Pyrefly incorrectly flags valid
dataclassfield namedselfas duplicate and wrong #3900 errors on main and pass with the fix.no diffs!