Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.9k
Fix/attrs init overload#19104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Fix/attrs init overload#19104
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This comment has been minimized.
This comment has been minimized.
I'm afraid this will break horribly down the road. Any custom To see this actually causing trouble (maybe I'm reading the code wrong, after all), please try adding a testcase with such overridden |
uko3211 commentedMay 18, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 custominit is detected—something like: (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 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 fromtypingimportoverloadimportattrs@attrs.frozen(init=False)classC:x:"int | str"@overloaddef__init__(self,x:int)->None: ...@overloaddef__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 importattrs@attrs.frozen(init=False)classC: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 |
uko3211 commentedMay 19, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 I'll revise the code accordingly and push the changes soon. |
for more information, seehttps://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
uko3211 commentedMay 19, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
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! |
I'm not sure I love the idea of returning |
Thank you for the review. I will update and upload the test cases within today. Have a great day :) |
…init__ methods in attrs classes
for more information, seehttps://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
…init__ methods in attrs classes
for more information, seehttps://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
@sterliakov Hello, After several attempts, I was able to add the test case. |
uko3211 commentedMay 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 ofmypy 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 |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
return None | ||
return init_method.type | ||
# case 1: normal FuncDef | ||
if isinstance(init_method, FuncDef) and isinstance(init_method.type, CallableType): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd probably suggest checking that thisinit_method
was actuallyplugin_generated
and returningAny
otherwise - a manual implementation can be mismatching even without overloads!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you, I’ve understood everything. First, I will update the test cases as suggested, and then I’ll add the condition related to theplugin-generated
__init__
This comment has been minimized.
This comment has been minimized.
@sterliakov Hello, 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 frommypy_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 |
return init_method.type | ||
# case 1: normal FuncDef | ||
if isinstance(init_method, FuncDef) and isinstance(init_method.type, CallableType): | ||
if getattr(init_method, "plugin_generated", False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Whygetattr
? It should be a regular attribute,init_method.plugin_generated
. If mypy says there's no such attribute, then it should be looked up on a different object, not hidden ingetattr
- that would explain the failing tests
Uh oh!
There was an error while loading.Please reload this page.
Fixes#19003
This PR fixes the issue by handling the case where theinit method can be an OverloadedFuncDef with CallableType items.