Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stubtest: ignore setattr and delattr inherited from object #18325

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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.

__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

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?

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.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants