Skip to content

stubtest: ignore setattr and delattr inherited from object#18325

Merged
hauntsaninja merged 3 commits intopython:masterfrom
tungol:stubtest_object
Mar 2, 2025
Merged

stubtest: ignore setattr and delattr inherited from object#18325
hauntsaninja merged 3 commits intopython:masterfrom
tungol:stubtest_object

Conversation

@tungol
Copy link
Contributor

@tungol tungol commented Dec 21, 2024

__setattr__ and __delattr__ from object are special cased by type checkers, so defining them on an inheriting class, even with the same signature, has a different meaning.

This one is very similar to #18314.

Typeshed has this allowlist entry:

multiprocessing.(dummy|managers).Namespace.__[gs]etattr__ # Any field can be set on Namespace

I was looking into this after I noticed that Namespace.__setattr__ didn't appear in my local stubtest results. The reason is that it's inherited from object, but object.__setattr__ has this note in typeshed:

# N.B. `object.__setattr__` and `object.__delattr__` are heavily special-cased by type checkers.
# Overriding them in subclasses has different semantics, even if the override has an identical signature.

That lead me to python/typeshed#7385, which discusses how type checkers ignore object.__setattr__ and object.__delattr__ as a matter of practicality. In that light, I think that stubtest should also ignore these methods when inherited from object at runtime, and flag their existence in the stubs when runtime doesn't have a custom version.

This change generates these new errors in typeshed:

error: _ctypes.Structure.__setattr__ is not present at runtime
error: _ctypes.Union.__setattr__ is not present at runtime
error: argparse.Namespace.__setattr__ is not present at runtime
error: ctypes.Structure.__setattr__ is not present at runtime
error: ctypes.Union.__setattr__ is not present at runtime
error: logging.LogRecord.__setattr__ is not present at runtime
error: optparse.Values.__setattr__ is not present at runtime
error: types.SimpleNamespace.__delattr__ is not present at runtime
error: types.SimpleNamespace.__setattr__ is not present at runtime

It would also create new errors for multiprocessing.dummy.Namespace.__setattr__ and multiprocessing.managers.Namespace.__setattr__, but these are already on the allowlist.

@tungol
stubtest: ignore setattr and delattr inherited from object
e978e37 
__setattr__ and __delattr__ from object are special cased by
type checkers, so defining them on an inheriting class,
even with the same signature, has a different meaning.
@tungol
Copy link
Contributor Author

tungol commented Dec 21, 2024

I also took the opportunity to make the style of the conditionals introduced by #18314 match.

@tungol
Copy link
Contributor Author

tungol commented Dec 21, 2024 •
edited
Loading

I'll note additionally that I can see arguments both for and against this change. I'm curious to see what other people think.

In favor is that it draws attention to the special nature of overriding object.__setattr__ in the stubs, potentially avoiding problems like what happened in python/typeshed#7385. Against is that runtime is not required to implement a custom version of __setattr__ to get this functionality, and this change kind of implies that they should. On the third hand, if we weren't using stubs but typing inline with the runtime implementation, then the runtime would need a custom version of __setattr__ to get the typing functionality. It's just not needed for the runtime functionality, so this kind of aligns with that, in a funny way?

hauntsaninja
hauntsaninja approved these changes Dec 22, 2024
View reviewed changes
Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good change to me!

Setting runtime_attr = MISSING gets a little dicey.
I think it's fine in this case because of IGNORABLE_CLASS_DUNDERS, but maybe we should make verify_none simply return if runtime is missing (probably should rename it to verify_missing too)

@tungol
Copy link
Contributor Author

tungol commented Dec 22, 2024

Oh, interesting. I thought that would be correct because of runtime_attr = inspect.getattr_static(runtime, mangled_entry, MISSING).

It looks like we hit verify_none when stub_to_verify is MISSING. If I understand correctly, the concern is that we only expect reach stub_to_verify is MISSING when the entry was added to to_check because it was present on the runtime. Setting runtime_attr = MISSING opens up the possibility that both the stub and runtime are MISSING, which we don't currently account for. I think updating verify_none makes sense if that's correct.

hauntsaninja
hauntsaninja approved these changes Dec 22, 2024
View reviewed changes
Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup exactly!

I'll leave open for a few days in case anyone else has opinions

@hauntsaninja hauntsaninja merged commit b1ac028 into python:master Mar 2, 2025
12 checks passed
@tungol tungol deleted the stubtest_object branch March 2, 2025 04:09
@hauntsaninja hauntsaninja mentioned this pull request May 29, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hauntsaninja hauntsaninja hauntsaninja approved these changes

Assignees

No one assigned

Labels

upnext

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tungol @hauntsaninja