Restrict dynamic attribute creation with slots #7388
Conversation
|
Hi @xmunoz! Thanks so much for looking at this. I think there's probably a way to avoid manually managing the slots, instance variables, and parameters to |
|
Great, thanks! Yeah, I think |
|
After some more discussion on #7313 about |
|
Once the existential question about this PR is resolved, I have two follow up questions for the reviewers:
|
xavfernandez
added
the
skip news
|
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
Nope. :) @xmunoz Would it be possible for you to update this PR? |
|
We're green! |
|
I opened this pull request months ago, honestly am not too sure why I did this the way that I did. That said, I rolled back the Base class stuff locally and ran tests locally, which failed. I won't have too much time to dedicate to this in the near term, but given the concern I'll go ahead and push my local changes and leave the failures for someone else to debug. |
|
Scrap my last comment, looks like everything is green :) |
| attr_getters = [operator.attrgetter(attr) | ||
| for attr in self.__slots__] | ||
| return all(getter(self) == getter(other) | ||
| for getter in attr_getters) |
There was a problem hiding this comment.
Equality on list could have ordering issues; we probably need to cast the result; something like
self_attrs = {k: getattr(self, k) for k in self.__slots__}
other_attrs = {k: getattr(other, k) for k in other.__slots__}
return self_attrs == other_attrs
There was a problem hiding this comment.
We're protected by a self.__slots__ == other.__slots__, which is reasonable with self.__class__ IMO.
Your suggestion is more robust tho.
There was a problem hiding this comment.
We're protected by a
self.__slots__ == other.__slots__, which is reasonable withself.__class__IMO.
Subclassing may result in self.__class__ != other.__class__ (therefore not guarenteeing the __slots__ declarations match), which is the main issue I have with the current implementation.
Co-authored-by: Pradyun Gedam <[email protected]>
| return False | ||
|
|
||
| if self.__slots__ != other.__slots__: | ||
| return False |
There was a problem hiding this comment.
Again, this is suspect to false positives when __slots__ is redefined.
Example:
class Foo:
__slots__ = ("a", "b")
def __init__(self, a, b):
self.a = a
self.b = b
def __eq__(self, other):
if self.__slots__ != other.__slots__:
return False
return all(
getattr(self, k) == getattr(other, k)
for k in self.__slots__
)
class Bar(Foo):
__slots__ = ("b", "a")
# False!
print(Foo(1, 2) == Bar(1, 2))
There was a problem hiding this comment.
I wonder if we want to be a little more explicit rather than pulling out a bunch of hacks here 😄 I mean there are only 2 attributes, which can be compared manually. Subclasses extending this may define their own comparison methods.
There was a problem hiding this comment.
I'm inclined to leave this as-is for now.
There was a problem hiding this comment.
So if I understand correctly, the __eq__ should be updated to
def __eq__(self, other):
self_attrs = {k: getattr(self, k) for k in self.__slots__}
other_attrs = {k: getattr(other, k) for k in other.__slots__}
return self_attrs == other_attrsas per this comment and #7388 (comment)
|
@uranusjr are you OK to iterate on the equality function in a follow up PR? I'd like to get this PR merged since it's good enough IMO. |
|
I am fine with merging this to master as long as the tests are able to pass. |
Fixes #7313.