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

use the ClassDef attribute inference process in Instances #2563

Conversation

temyurchenko
Copy link
Contributor

A part of getting rid of non-Module roots, see #2536

Instances often have «special_attributes». ClassDef does a lot of
transformations of attibutes during inference, so let's reuse them
in Instance.

We are fixing inference of the "special" attributes of
Instances. Before these changes, the FunctionDef attributes
wouldn't get wrapped into a BoundMethod. This was facilitated by
extracting the logic of inferring attributes into
'FunctionDef._infer_attrs' and reusing it in BaseInstance.

This issue isn't visible right now, because the special attributes
are simply not found due not being attached to the parent (the
instance). Which in turn is caused by not providing the actual
parent in the constructor. Since we are on a quest to always
provide a parent, this change is necessary.

@temyurchenko
Copy link
Contributor Author

Note that the biggest source of changes is just moving a bunch of code into a separate function

Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.09%. Comparing base (a99967e) to head (f2c3947).
Report is 20 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2563      +/-   ##
==========================================
+ Coverage   93.05%   93.09%   +0.03%     
==========================================
  Files          93       93              
  Lines       11050    11053       +3     
==========================================
+ Hits        10283    10290       +7     
+ Misses        767      763       -4     
Flag Coverage Δ
linux 92.94% <100.00%> (+<0.01%) ⬆️
pypy 93.09% <100.00%> (+0.03%) ⬆️
windows 93.07% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
astroid/bases.py 88.32% <100.00%> (+0.03%) ⬆️
astroid/nodes/scoped_nodes/scoped_nodes.py 93.22% <100.00%> (+0.08%) ⬆️

... and 3 files with indirect coverage changes

Instances often have «special_attributes». ClassDef does a lot of
   transformations of attibutes during inference, so let's reuse them
   in Instance.

We are fixing inference of the "special" attributes of
   Instances. Before these changes, the FunctionDef attributes
   wouldn't get wrapped into a BoundMethod. This was facilitated by
   extracting the logic of inferring attributes into
   'FunctionDef._infer_attrs' and reusing it in BaseInstance.

  This issue isn't visible right now, because the special attributes
   are simply not found due not being attached to the parent (the
   instance). Which in turn is caused by not providing the actual
   parent in the constructor. Since we are on a quest to always
  provide a parent, this change is necessary.
@@ -1295,7 +1295,7 @@ def func(arg1, arg2):
self.assertIsInstance(inferred[0], BoundMethod)
inferred = list(Instance(cls).igetattr("m4"))
self.assertEqual(len(inferred), 1)
self.assertIsInstance(inferred[0], nodes.FunctionDef)
self.assertIsInstance(inferred[0], BoundMethod)
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 this change is incorrect, and therefore shows something is up with this PR.

To reproduce:

class Clazz(object): ...


def func(arg1, arg2):
    "function that will be used as a method"
    return arg1.value + arg2


Clazz.m3 = func
inst = Clazz()
inst.m4 = func

print(Clazz.m3)
print(inst.m3)
print(inst.m4)
<function func at 0x109582340>
<bound method func of <__main__.Clazz object at 0x1095ab9b0>>
<function func at 0x109582340>

This should remain a FunctionDef and not a BoundMethod.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, you are right. Then, I think the issue is in the semantics of objectmodel. For ordinary Instances, special_attributes are «instance» attributes. They should remain FunctionDef. However, in some cases, for specialized instances, e.g. Generator, the objectmodel is used for «class» methods. So, the FunctionDefs should get wrapped into BoundMethods.
So, the fix should lie on that side, but it also feels quite error-prone to use the same special_attributes for the two different purposes.

I'll do a fix, but a bit later, I'm a bit short on time.

@temyurchenko
Copy link
Contributor Author

Closing in favour of #2584

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

Successfully merging this pull request may close these issues.

2 participants