-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Make Options work under mypyc #5518
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
Conversation
This requires reworking a bunch of operations to not directly depend on the __dict__
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.
Just one suggestion.
PROGRAM_TEXT = 2 # type: ClassVar[int] | ||
|
||
|
||
PER_MODULE_OPTIONS = { |
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.
Maybe add a short comment why PER_MODULE_OPTIONS
and OPTIONS_AFFECTING_CACHE
can't be ClassVar
s in Options
(similar to what you did for fastparse.py
IIRC)?
mypy/options.py
Outdated
del d['per_module_cache'] | ||
return d | ||
|
||
def __eq__(self, other: object) -> bool: | ||
return self.__class__ == other.__class__ and self.__dict__ == other.__dict__ | ||
return isinstance(other, Options) and self.snapshot() == other.snapshot() |
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.
This looks like this __eq__
will become much more expensive. Have you tracked where/how it is used?
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 remembered from when I was looking into this while doing the original snapshot stuff that the answer was "not often", but I looked more closely and the answer was: in exactly one test. I just removed __eq__
and made that test use snapshot directly
@@ -209,12 +214,16 @@ def __init__(self) -> None: | |||
|
|||
def snapshot(self) -> object: | |||
"""Produce a comparable snapshot of this Option""" | |||
d = dict(self.__dict__) | |||
# Under mypyc, we don't have a __dict__, so we need to do worse things. | |||
d = dict(getattr(self, '__dict__', ())) |
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.
So IIUC under mypyc d will start out empty?
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.
Yeah
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.
OK. I hope that with some (not-too-far-in-the) future mypyc we will be able to simplify this -- it feels quite gnarly. But I do want to make progress!
This reverts commit b336dc8.
These silence errors about missing type annotations for calls like these: ``` x = getattr(o, 'a', []) y = getattr(o, 'b', {}) ``` This is basically a generalization of #5518 and other overloads we already have. This works around #11572. I encountered the issue in several places when testing recent typeshed against an internal repo. Manually cherry-picked from python/typeshed#6355.
This requires reworking a bunch of operations to not directly depend
on
__dict__