Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
uko3211 wants to merge18 commits intopython:master
base:master
Choose a base branch
Loading
fromuko3211:fix/attrs-init-overload

Conversation

uko3211
Copy link

@uko3211uko3211 commentedMay 18, 2025
edited
Loading

Fixes#19003

This PR fixes the issue by handling the case where theinit method can be an OverloadedFuncDef with CallableType items.

@github-actionsGitHub Actions

This comment has been minimized.

@sterliakov
Copy link
Collaborator

I'm afraid this will break horribly down the road. Any custom__init__ method is a disaster for this plugin, note how we use itsarg_names andarg_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 ("overriddeninit 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/typesdo not correspond to the generated signature and usingattrs.evolve on that class.

uko3211 reacted with eyes emoji

@uko3211
Copy link
Author

uko3211 commentedMay 18, 2025
edited
Loading

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:
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)

@sterliakov
Copy link
Collaborator

Huh, not exactly, raising aTypeError 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 onctx.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:

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
Copy link
Author

uko3211 commentedMay 19, 2025
edited
Loading

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 usingctx.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.

@github-actionsGitHub Actions

This comment has been minimized.

@uko3211
Copy link
Author

uko3211 commentedMay 19, 2025
edited
Loading

  1. The overloaded custom init no longer returns an error.
  2. The ambiguous type in the overloaded custom init is handled by returning any, allowing users to proceed with awareness of this behavior.

@uko3211
Copy link
Author

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

@sterliakov
Copy link
Collaborator

I'm not sure I love the idea of returningAnyType, that seems a bit arbitrary, but should work at least. Please add relevant testcases - seetest-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).

uko3211 reacted with thumbs up emoji

@uko3211
Copy link
Author

Thank you for the review. I will update and upload the test cases within today. Have a great day :)

@github-actionsGitHub Actions

This comment has been minimized.

@github-actionsGitHub Actions

This comment has been minimized.

@uko3211
Copy link
Author

@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!

@uko3211
Copy link
Author

uko3211 commentedMay 22, 2025
edited
Loading

Diff frommypy_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 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

@sterliakov
Copy link
Collaborator

Ignore thexarray diff, we have some recent regression causing nondeterministic results to appear more often, tracked in#19121. Thexarray diff was reported on even more obviously unrelated PRs, don't ping its maintainers - this is a mypy bug.

return None
return init_method.type
# case 1: normal FuncDef
if isinstance(init_method, FuncDef) and isinstance(init_method.type, CallableType):
Copy link
Collaborator

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!

Copy link
Author

@uko3211uko3211May 23, 2025
edited
Loading

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__

@github-actionsGitHub Actions

This comment has been minimized.

@uko3211
Copy link
Author

@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?

@github-actionsGitHub Actions
Copy link
Contributor

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]

@sterliakov
Copy link
Collaborator

Thexarray issue is moot, while the test failures are real - the changed code does not pass some tests. Either the__init__ method is somehownotplugin_generated (which is a bug) or something went wrong in this PR.

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):
Copy link
Collaborator

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

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

@sterliakovsterliakovsterliakov requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Overloaded__init__ prevents type from being recognized as an attrs class
2 participants
@uko3211@sterliakov

[8]ページ先頭

©2009-2025 Movatter.jp