python
Fix/attrs init overload
Fixes https://github.com/python/mypy/issues/19003
This PR fixes the issue by handling the case where the init method can be an OverloadedFuncDef with CallableType items.
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
I'm afraid this will break horribly down the road. Any custom __init__ method is a disaster for this plugin, note how we use its arg_names and arg_types to get the actual field types. If __init__ method is customized, this is no longer true, and _get_expanded_attr_types will no longer be correct. I'd suggest just failing with a different error ("overridden init not supported" instead of "not an attrs class"), but also feel free to explore alternative ways to gather fields!
To see this actually causing trouble (maybe I'm reading the code wrong, after all), please try adding a testcase with such overridden __init__ where arg names/types do not correspond to the generated signature and using attrs.evolve on that class.
Hello! First of all, thank you for your detailed comment. From what I understand, I don’t believe there’s anything wrong with the code I wrote to address the issue. However, based on your feedback, it seems you’re suggesting that instead of returning None, it would be better to explicitly fail when a custom init is detected—something like:
raise TypeError("Custom __init__ methods are not supported with this plugin")
Is that correct?
(This is my first PR, and English is not my first language, so I really appreciate your understanding in case I misunderstood anything)
Huh, not exactly, raising a TypeError is a bit too harsh: mypy reports errors differently, we do not make it crash when a problem is detected in user's code. Look at how _fail_not_attrs_class function reports the problem - I suspect you need to do something similar based on ctx.api.fail.
However, on second thought we usually avoid producing error messages when some construct is not supported or cannot be expressed statically. It might be wiser to fall back to Any in that case and accept the call as-is, even if the arguments do not match. I'm not sure which of the options is better.
To explain my point about "potential problems" caused by overloaded or even just manually defined __init__, here are two snippets:
from typing import overload
import attrs
@attrs.frozen(init=False)
class C:
x: "int | str"
@overload
def __init__(self, x: int) -> None: ...
@overload
def __init__(self, x: str) -> None: ...
def __init__(self, x: "int | str") -> None:
self.__attrs_init__(x)
obj = C(1)
attrs.evolve(obj, x=2)
attrs.evolve(obj, x="2") # E: ... - False positive
import attrs
@attrs.frozen(init=False)
class C:
x: "int | str"
def __init__(self, x: "int | str", y: bool) -> None:
self.__attrs_init__(x)
obj = C(1, False)
attrs.evolve(obj, x=2) # False negative, error at runtime
Thank you so much for the detailed explanation. It was really helpful and I believe it will greatly assist me in resolving the issue.
Among the approaches you suggested, I feel that using ctx.api.fail might actually introduce unnecessary limitations for mypy users. So I think falling back to Any, as you mentioned, would be a better solution in this case.
I'll revise the code accordingly and push the changes soon.
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅
- The overloaded custom init no longer returns an error.
- The ambiguous type in the overloaded custom init is handled by returning any, allowing users to proceed with awareness of this behavior.
I understand you may be busy, but I would really appreciate it if you could take a moment to review my latest commit. Thank you! @sterliakov
I'm not sure I love the idea of returning AnyType, that seems a bit arbitrary, but should work at least. Please add relevant testcases - see test-data/unit/check-plugin-attrs.test for inspiring examples, you'll just need to add new testcases with snippets that fail before this patch and work now (and try to think of as many corner cases as you can) to that file. Note the [builtins fixtures/plugin_attrs.pyi] line at the bottom of every testcase, you'll need to add it to new tests as well (or you may see weird errors about undefined builtin names).
Thank you for the review. I will update and upload the test cases within today. Have a great day :)
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅
Diff from mypy_primer, showing the effect of this PR on open source code:
xarray (https://github.com/pydata/xarray)
+ xarray/conventions.py:413: error: Argument "decode_timedelta" to "decode_cf_variable" has incompatible type "object"; expected "bool | CFTimedeltaCoder | None" [arg-type]
@sterliakov Hello, After several attempts, I was able to add the test case. I would appreciate it if you could review it carefully. Have a great day!
Diff from mypy_primer, showing the effect of this PR on open source code:
xarray (https://github.com/pydata/xarray) + xarray/conventions.py:413: error: Argument "decode_timedelta" to "decode_cf_variable" has incompatible type "object"; expected "bool | CFTimedeltaCoder | None" [arg-type]
After I committed the changes to attrs.py, there were no issues reported by mypy_primer. However, the error appeared only after I committed the test cases. Based on this, I believe it might be a false positive.
When comparing the importance of mypy issue #19003 with the reported error in xarray, I think resolving issue #19003 should take priority and is more critical at this point.
If it turns out that the error reported in xarray is actually a valid detection — due to an argument not matching the expected type — then after this PR is merged, I will either modify the xarray source code to ignore the case where the function receives an object type, or open an issue in the xarray repository to address it.
All in all, it seems that the error is an expected outcome of the changes made in this PR, and I believe it's something that should be fixed within the xarray project
Ignore the xarray diff, we have some recent regression causing nondeterministic results to appear more often, tracked in #19121. The xarray diff was reported on even more obviously unrelated PRs, don't ping its maintainers - this is a mypy bug.
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅
@sterliakov Hello, I've completed the updates to both the test-case and attrs.py files. All the changes are intentional and the tests have been updated accordingly.
However, I'm now seeing an error from the GitHub bot that wasn't occurring before. From my investigation, this issue doesn’t appear to be caused by my changes. Could you please help verify whether this is a problem with the bot or something unrelated to my patch?
Diff from mypy_primer, showing the effect of this PR on open source code:
xarray (https://github.com/pydata/xarray)
+ xarray/conventions.py:415: error: Argument "decode_timedelta" to "decode_cf_variable" has incompatible type "object"; expected "bool | CFTimedeltaCoder | None" [arg-type]
The xarray issue is moot, while the test failures are real - the changed code does not pass some tests. Either the __init__ method is somehow not plugin_generated (which is a bug) or something went wrong in this PR.
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅
@sterliakov Hi, the updated changes are now working correctly. Could you please review my PR to see if it’s ready to be merged?
Hello! I haven't received a response regarding my recent commits for a few days, so I just wanted to send a quick ping. Apologies for bothering you during your busy schedule! @sterliakov @JukkaL
@sterliakov Hi, all issues have now been resolved.
mypy no longer raises errors for overloaded __init__ methods.
Just pinging to let you know the problems have been fully addressed.
I'd appreciate it if you could take a look or proceed when you have a chance. Thank you!
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅
@sterliakov @JukkaL Hello, I’m following up as I haven’t received a response for a week. I understand you must be busy, but I would really appreciate it if you could kindly check and get back to me. Thank you
Personally I'm totally swamped right now and won't have any chance to review code outside of my primary work with enough attention for at least a few days. I'm not even the maintainer here, just a regular contributor with triager access, I don't merge PRs nor do I have a final say of what should/shouldn't be done this or other way. My reviews are essentially my personal suggestions based on what I know about mypy and my overall engineering experience.
I invest my free time into mypy because I love how it works, because I enjoy playing with static analysis and dev tooling, because I use mypy myself a lot and prefer it to its competitors. I try to help to the extent possible without harming my life and work, but I can't promise to review/commit/triage stuff on a daily basis (or with some other predictable schedule other than "perhaps you'll hear back from me this month"). Code merges and reviews aren't that fast here - some of my PRs from Jan 2024 still awaiting merge/review, just because mypy is a huge OSS product mostly maintained by enthusiasts. It's OK to tag or ping someone once in a while, but please do that wisely.
I can personally thank you for your contribution, every such PR makes mypy better. I can assure you that this PR will get merged eventually, it just takes some time. mypy 1.16 was just released a few days ago, there's no rush to merge this - there's enough time to get this thoroughly reviewed and merged before the next release.
And I need to clean my n key, there were too many typos above with this letter missing...
I lacked understanding of how things are usually handled. I apologize if any of my actions came across as rude. Thank you so much for taking the time to review my PR, I really appreciate it!
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅