-
-
Notifications
You must be signed in to change notification settings - Fork 305
Move object dunders from FunctionModel
to ObjectModel
#2847
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Emmanuel Ferdman <[email protected]>
Codecov Reportβ Patch coverage is
β Your patch check has failed because the patch coverage (93.02%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2847 +/- ##
==========================================
- Coverage 93.37% 93.33% -0.04%
==========================================
Files 92 92
Lines 11150 11168 +18
==========================================
+ Hits 10411 10424 +13
- Misses 739 744 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
π New features to boost your workflow:
|
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.
As can be seen from the docs you link, most of these should live on ObjectModel
instead as they come from object
.
We tried to do so before in #1519 but failed. Perhaps you want to have another look at that PR and see if you can fix the issue we faced there?
@DanielNoord Thanks! Iβll take a look at this. From that thread, it seems you were close to a solution, except for one test case that didnβt pass. Do you remember which test it was? |
@emmanuel-ferdman I believe it was an issue in the |
Signed-off-by: Emmanuel Ferdman <[email protected]>
for more information, see https://pre-commit.ci
Signed-off-by: Emmanuel Ferdman <[email protected]>
@DanielNoord I've put together an initial solution for moving object dunders from The solution uses a placeholder pattern:
How it works:
Thanks for any feedback π |
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 is amazing, very well written PR and a nice set of tests.
Well done on getting this to pass all tests. I have left some comments, but would really like to help you push this over the line :)
special_attr = self.special_attributes.lookup(name) | ||
if not isinstance( | ||
special_attr, (util.UninferableBase, node_classes.Unknown) | ||
): | ||
result = [special_attr] | ||
return result |
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.
What is the effect of this? What will we eventually return if the if
is not True
?
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 block is a short-circuit used when there are no concrete locals/ancestor definitions: originally it always returned special_attributes.lookup(name)
(even if that was Unknown
or Uninferable
). The new behavior only returns the special_attr
when it is a concrete value (not node_classes.Unknown
or util.UninferableBase
), preventing a placeholder from being returned prematurely and masking an override in a metaclass/base class. If the if
is not true we continue the normal lookup (metaclass lookup, collect/filter locals/ancestors) and ultimately return any real definitions found - otherwise an AttributeInferenceError
is raised. Placeholders (Unknown
/Uninferable
) therefore mean βkeep looking,β not βreturn this as the final result.β
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.
Unkown
feels like a nice placeholder for "keep looking", Uninferable
generally means "stop inferring, you won't be able to". Can we make this only check for Unknown
? Or does that not work?
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.
Yes, good idea! ObjectModel
only returns Unknown
as placeholders, never Uninferable
. I changed both checks (in ClassDef.getattr()
and BaseInstance.getattr()
) to only check for Unknown
. If Uninferable
somehow appeared from special_attributes
, we should treat it as a final value and return it, not skip it. All tests pass with this change.
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.
Perhaps we should update the docstring of Unknown
to reflect this?
child = object_build_datadescriptor(node, member) | ||
elif isinstance(member, tuple(node_classes.CONST_CLS)): | ||
if alias in node.special_attributes: | ||
# Special case: __hash__ = None overrides ObjectModel for unhashable types. |
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.
Same question as above
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.
Same reasoning as the previous hash question - hash = None
is the only attribute where None
has special semantic meaning in Python (marks unhashable types). Other None
values don't need this override behavior.
) | ||
inferred = next(eq_result.infer()) | ||
assert isinstance(inferred, nodes.Const) | ||
assert inferred.value == "custom equality" |
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.
Nice!
Signed-off-by: Emmanuel Ferdman <[email protected]>
FunctionModel
to ObjectModel
Signed-off-by: Emmanuel Ferdman <[email protected]>
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.
One final comment about a docstring.
Also, can you add a Changelog for this?
Nice work on this! @jacobtylerwalls will be happy to see we finally got this to work :)
special_attr = self.special_attributes.lookup(name) | ||
if not isinstance( | ||
special_attr, (util.UninferableBase, node_classes.Unknown) | ||
): | ||
result = [special_attr] | ||
return result |
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.
Perhaps we should update the docstring of Unknown
to reflect this?
@emmanuel-ferdman Will this also close pylint-dev/pylint#6094? The old PR references it. |
Type of Changes
Description
This PR does:
This PR moves object dunder methods from
FunctionModel
toObjectModel
asUnknown
placeholders, making them available to all object types.Fixes #2742 #2741.