Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+193 −40
Open

Move object dunders from FunctionModel to ObjectModel #2847

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
aeedd50
Add missing operators in FunctionModel
emmanuel-ferdman Oct 4, 2025
50a56f8
Move object dunders from `FunctionModel` to `ObjectModel`
emmanuel-ferdman Oct 5, 2025
b691e28
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 5, 2025
1bc57c5
Move object dunders from `FunctionModel` to `ObjectModel`
emmanuel-ferdman Oct 5, 2025
2762ac9
Move object dunders from `FunctionModel` to `ObjectModel`
emmanuel-ferdman Oct 7, 2025
6c4d29f
Merge branch 'pylint-dev:main' into main
emmanuel-ferdman Oct 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 29 additions & 9 deletions astroid/bases.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,9 @@ def getattr(
values = self._proxied.instance_attr(name, context)
except AttributeInferenceError as exc:
if self.special_attributes and name in self.special_attributes:
return [self.special_attributes.lookup(name)]
special_attr = self.special_attributes.lookup(name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar question as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same pattern as the ClassDef.getattr() check - when special_attr is Unknown or Uninferable, we skip the early return and continue to the if lookupclass: block which searches the class for the attribute. This ensures we find actual implementations in classes rather than stopping at the Unknown placeholder.

if not isinstance(special_attr, (UninferableBase, nodes.Unknown)):
return [special_attr]

if lookupclass:
# Class attributes not available through the instance
Expand Down Expand Up @@ -571,10 +573,14 @@ def _infer_type_new_call(
raise InferenceError(context=context) from e
if not isinstance(mcs, nodes.ClassDef):
# Not a valid first argument.
return None
raise InferenceError(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels like it could be merged on its own, is that True? In that case I'd probably split this out to ensure we don't do too much in a single PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is independent of the object dunder work. It's about improving type.new() validation error messages - changing silent None returns to descriptive InferenceError exceptions. The existing tests already expect InferenceError, so it's fully compatible. I'm happy to split it into a separate PR to keep this one focused on the object dunder changes. Should I remove it from this PR and create a new one, or would you prefer I keep it here as a small related improvement?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please split it! I'll make sure to review it asap, but generally I and other maintainers need quite a lot of mental energy to review astroid as it all quite complex. Generallty, the smaller the PR the easier I can find that energy.

"type.__new__() requires a class for metaclass", context=context
)
if not mcs.is_subtype_of("builtins.type"):
# Not a valid metaclass.
return None
raise InferenceError(
"type.__new__() metaclass must be a subclass of type", context=context
)

# Verify the name
try:
Expand All @@ -583,10 +589,14 @@ def _infer_type_new_call(
raise InferenceError(context=context) from e
if not isinstance(name, nodes.Const):
# Not a valid name, needs to be a const.
return None
raise InferenceError(
"type.__new__() requires a constant for name", context=context
)
if not isinstance(name.value, str):
# Needs to be a string.
return None
raise InferenceError(
"type.__new__() requires a string for name", context=context
)

# Verify the bases
try:
Expand All @@ -595,14 +605,18 @@ def _infer_type_new_call(
raise InferenceError(context=context) from e
if not isinstance(bases, nodes.Tuple):
# Needs to be a tuple.
return None
raise InferenceError(
"type.__new__() requires a tuple for bases", context=context
)
try:
inferred_bases = [next(elt.infer(context=context)) for elt in bases.elts]
except StopIteration as e:
raise InferenceError(context=context) from e
if any(not isinstance(base, nodes.ClassDef) for base in inferred_bases):
# All the bases needs to be Classes
return None
raise InferenceError(
"type.__new__() requires classes for bases", context=context
)

# Verify the attributes.
try:
Expand All @@ -611,7 +625,9 @@ def _infer_type_new_call(
raise InferenceError(context=context) from e
if not isinstance(attrs, nodes.Dict):
# Needs to be a dictionary.
return None
raise InferenceError(
"type.__new__() requires a dict for attrs", context=context
)
cls_locals: dict[str, list[InferenceResult]] = collections.defaultdict(list)
for key, value in attrs.items:
try:
Expand Down Expand Up @@ -664,9 +680,13 @@ def infer_call_result(
and self.bound.name == "type"
and self.name == "__new__"
and isinstance(caller, nodes.Call)
and len(caller.args) == 4
):
# Check if we have a ``type.__new__(mcs, name, bases, attrs)`` call.
if len(caller.args) != 4:
raise InferenceError(
f"type.__new__() requires 4 arguments, got {len(caller.args)}",
context=context,
)
new_cls = self._infer_type_new_call(caller, context)
if new_cls:
return iter((new_cls,))
Expand Down
54 changes: 33 additions & 21 deletions astroid/interpreter/objectmodel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,33 @@ def attr___init__(self) -> bases.BoundMethod:

return bases.BoundMethod(proxy=node, bound=_get_bound_node(self))

# Base object attributes that return Unknown as fallback placeholders.
@property
def attr___ne__(self):
return node_classes.Unknown(parent=self._instance)

attr___class__ = attr___ne__
attr___delattr__ = attr___ne__
attr___dir__ = attr___ne__
attr___doc__ = attr___ne__
attr___eq__ = attr___ne__
attr___format__ = attr___ne__
attr___ge__ = attr___ne__
attr___getattribute__ = attr___ne__
attr___getstate__ = attr___ne__
attr___gt__ = attr___ne__
attr___hash__ = attr___ne__
attr___init_subclass__ = attr___ne__
attr___le__ = attr___ne__
attr___lt__ = attr___ne__
attr___reduce__ = attr___ne__
attr___reduce_ex__ = attr___ne__
attr___repr__ = attr___ne__
attr___setattr__ = attr___ne__
attr___sizeof__ = attr___ne__
attr___str__ = attr___ne__
attr___subclasshook__ = attr___ne__


class ModuleModel(ObjectModel):
def _builtins(self):
Expand Down Expand Up @@ -459,30 +486,15 @@ def test(self):

return DescriptorBoundMethod(proxy=self._instance, bound=self._instance)

# These are here just for completion.
# Function-specific attributes.
@property
def attr___ne__(self):
def attr___call__(self):
return node_classes.Unknown(parent=self._instance)

attr___subclasshook__ = attr___ne__
attr___str__ = attr___ne__
attr___sizeof__ = attr___ne__
attr___setattr___ = attr___ne__
attr___repr__ = attr___ne__
attr___reduce__ = attr___ne__
attr___reduce_ex__ = attr___ne__
attr___lt__ = attr___ne__
attr___eq__ = attr___ne__
attr___gt__ = attr___ne__
attr___format__ = attr___ne__
attr___delattr___ = attr___ne__
attr___getattribute__ = attr___ne__
attr___hash__ = attr___ne__
attr___dir__ = attr___ne__
attr___call__ = attr___ne__
attr___class__ = attr___ne__
attr___closure__ = attr___ne__
attr___code__ = attr___ne__
attr___builtins__ = attr___call__
attr___closure__ = attr___call__
attr___code__ = attr___call__
attr___type_params__ = attr___call__


class ClassModel(ObjectModel):
Expand Down
19 changes: 17 additions & 2 deletions astroid/nodes/scoped_nodes/scoped_nodes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1618,6 +1618,17 @@ def infer_call_result(
yield node_classes.Const(None)
return

# Builtin dunder methods have empty bodies, return Uninferable.
if (
self.root().qname() == "builtins"
and self.name.startswith("__")
and self.name.endswith("__")
and self.parent
and self.parent.__class__.__name__ == "ClassDef"
):
yield util.Uninferable
return
Comment on lines 1621 to 1629
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I sort of understand this change, but don't understand why we need to special case it like this.

Can we replace the checks with a look up in special_attributes? Or doesn't that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this case because infer_call_result() has to decide what a call returns, and for built-in dunder methods there is no Python body to inspect (since they're implemented in C). If we tried to parse return nodes we would either fail or produce incorrect results.

special_attributes only tells us that an attribute exists (a placeholder), not whether there is a Python body to infer a return value from. Therefore it cannot replace the empty-body/builtins check used here.

But on second thought, I simplified the case by explicitly checking len(self.body) == 0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it be okay if we always did this for len(self.body) == 0? Is there a function where len(self.body) == 0 that is inferable? Or is as ... or pass as body also len(self.body) == 0?


raise InferenceError("The function does not have any return statements")

for returnnode in itertools.chain((first_return,), returns):
Expand Down Expand Up @@ -2346,8 +2357,12 @@ def getattr(
values += classnode.locals.get(name, [])

if name in self.special_attributes and class_context and not values:
result = [self.special_attributes.lookup(name)]
return result
special_attr = self.special_attributes.lookup(name)
if not isinstance(
special_attr, (util.UninferableBase, node_classes.Unknown)
):
result = [special_attr]
return result
Comment on lines +2359 to +2364
Copy link
Collaborator

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?

Copy link
Contributor Author

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.”

Copy link
Collaborator

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?


if class_context:
values += self._metaclass_lookup_attribute(name, context)
Expand Down
10 changes: 8 additions & 2 deletions astroid/raw_building.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,10 @@ def attach_const_node(node, name: str, value) -> None:
"""create a Const node and register it in the locals of the given
node with the specified name
"""
if name not in node.special_attributes:
# Special case: __hash__ = None overrides ObjectModel for unhashable types.
if name == "__hash__" and value is None:
_attach_local_node(node, nodes.const_factory(value), name)
Comment on lines +81 to +83
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be the case for all attributes where value is None? Or only hash?

Copy link
Contributor Author

@emmanuel-ferdman emmanuel-ferdman Oct 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I believe only for __hash__. In Python __hash__ = None has a special semantic meaning - it explicitly marks a type as unhashable and must override the inherited object.__hash__. Other attributes set to None do not carry this override semantics and should not implicitly replace special_attributes entries, because that could hide real implementations or metadata. So I belive we should have this special-case restricted to __hash__ only.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we can link that documentation here? It would prevent me from wondering why this special case is here the next time.

And thanks for the link, TIL about something I knew implicitly but now know where to find the documentation for that feature 😄 Appreciated!

elif name not in node.special_attributes:
_attach_local_node(node, nodes.const_factory(value), name)


Expand Down Expand Up @@ -507,7 +510,10 @@ def object_build(
elif inspect.isdatadescriptor(member):
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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question as above

Copy link
Contributor Author

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.

if alias in node.special_attributes and not (
alias == "__hash__" and member is None
):
continue
child = nodes.const_factory(member)
elif inspect.isroutine(member):
Expand Down
5 changes: 3 additions & 2 deletions tests/test_inference.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5618,8 +5618,9 @@ def test_cannot_infer_call_result_for_builtin_methods() -> None:
)
inferred = next(node.infer())
lenmeth = next(inferred.igetattr("__len__"))
with pytest.raises(InferenceError):
next(lenmeth.infer_call_result(None, None))
# Builtin dunder methods now return Uninferable instead of raising InferenceError
result = next(lenmeth.infer_call_result(None, None))
assert result is util.Uninferable
Comment on lines +5621 to +5623
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I think the changes above now make more sense to me.

pylint works correctly with this change? Thanks for testing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - pylint works correctly (verified by running pylint test suite with astroid changes). pylint handles Uninferable in two ways: (1) explicit isinstance(value, util.UninferableBase) checks (e.g., pylint/checkers/typecheck.py:1365-1369, and (2) Uninferable is falsy like None, so if inferred: checks skip both. The behavior is identical - when builtin dunders can't be inferred, pylint skips type checking whether it gets None from an exception or Uninferable from the return value.



def test_unpack_dicts_in_assignment() -> None:
Expand Down
Loading