Move inference methods to nodes module(s) #2171

jacobtylerwalls · 2023-05-08T15:00:30Z

Type of Changes

	Type
✓	🔨 Refactoring

Description

Move _infer_() methods from inference.py to the nodes themselves (alternative to #2167).

Benefits

Avoid assigning methods to classes in inference.py and protocols.py (yuk!)
Move LookupMixIn to _base_nodes as promised in the long-existing TODO comment
We can now clearly see messages like:

astroid/nodes/node_classes.py:2349:4: W0221: Variadics removed in overriding 'Dict._infer' method (arguments-differ)

Drawbacks

There are some function-level imports created here that are symptoms of circular imports, but many of them are with the helpers module, and many of those are just for safe_infer, which could be extracted into its own helper module (it only needs UninferableBase). I don't think this jeopardizes the design.

Fixes #679

astroid/nodes/__init__.py

jacobtylerwalls · 2023-05-08T15:01:09Z

astroid/nodes/__init__.py

@@ -111,10 +107,7 @@
 )
 from astroid.nodes.utils import Position

-_BaseContainer = BaseContainer  # TODO Remove for astroid 3.0


jacobtylerwalls · 2023-05-08T15:01:32Z

astroid/protocols.py

-nodes.Assign.assigned_stmts = assign_assigned_stmts
-nodes.AnnAssign.assigned_stmts = assign_annassigned_stmts
-nodes.AugAssign.assigned_stmts = assign_assigned_stmts


jacobtylerwalls · 2023-05-08T15:02:36Z

astroid/nodes/node_classes.py


-# TODO: Move into _base_nodes. Blocked by import of _infer_stmts from bases.


jacobtylerwalls · 2023-05-08T15:02:50Z

astroid/nodes/node_classes.py

-    infer_lhs: ClassVar[InferLHS[AssignName]]
-


jacobtylerwalls · 2023-05-08T15:03:14Z

astroid/nodes/node_classes.py

@@ -1470,6 +1727,88 @@ def last_child(self):
        return self.ops[-1][1]
        # return self.left

+    # TODO: move to util?


decision point

jacobtylerwalls · 2023-05-08T15:03:29Z

astroid/nodes/node_classes.py

+    def _to_literal(node: SuccessfulInferenceResult) -> Any:
+        # Can raise SyntaxError or ValueError from ast.literal_eval
+        # Can raise AttributeError from node.as_string() as not all nodes have a visitor
+        # Is this the stupidest idea or the simplest idea?


Pierre-Sassoulas

I really heavily dislike the nodes.Const.infer_binary_op = const_infer_binary_op this is the most unreadable pattern (not to even mention cyclic import creator) I ever saw in the wild. I hope this approach can work !

DanielNoord

Left some early review comments.

Benefits

...

🚀 👍

Drawbacks

There are some function-level imports created here that are symptoms of circular imports, but many of them are with the helpers module, and many of those are just for safe_infer, which could be extracted into its own helper module (it only needs UninferableBase). I don't think this jeopardizes the design.

Agreed!

However, I think we are missing one major drawback (although not necessarily a drawback of this design).
We still have the issue of these classes being behemoths that (imo) try to do too much. With this change, and possibly the inclusion of protocols.py on the nodes themselves as well, there is not that much left besides the astroid.nodes package. I still don't know how I feel about this as it feels we are putting more logic on the nodes than is completely necessary.
There are also still reference to AstroidManager() or MANAGER which makes it impossible to decouple the global state used by astroid without starting the pass an actual manager object in these signatures.

Todos:

Get benchmarks

If we agree on this design I think we shouldn't be held back by performance degradation too much. We can more easily improve performance when we can actually work with the code.

astroid/bases.py

astroid/node_classes.py

astroid/nodes/_base_nodes.py

DanielNoord · 2023-05-08T15:19:50Z

astroid/nodes/_base_nodes.py

+
+
+class AttributeNode(NodeNG):
+    expr: NodeNG


This feels like this class should also define a postinit to couple this annotation to something actually setting it.

It's more of a mixin. The actual concrete classes do have postinit methods. Should I document this or raise some sort of TypeError when attempting to instantiate?

Perhaps make them ABC? However, I don't really like mixins in this way. We actually spend quite some time removing them from pylint because the way this mixins work don't allow us to guarantee that they function at runtime. We can't enforce that expr is being set.. Would it be feasible to make them have a postinit that needs to be called?

Happy to do it, but just for my own understanding, why would a postinit help? When would it ever be called, and how does it help a static type checker? I'm sure there is a reason, I just haven't worked with postinit in astroid yet.

In this case it should probably be an init. Sorry for the confusion!

If the mixins need to be robust against unintended instantiation, then they're more trouble then they're worth just for the DRY benefit. I'll just extract helper functions that can be shared.

I also greatly dislike mixin for the sake of being dry because having to initialise instance in a particular order and not being able to use typecheck (becauseof course you can't guarantee the instanciation particular order and a type checker will rightfully warn you) is order if magnitude worse than whatever design problem mixin are trying to solve.

astroid/nodes/_base_nodes.py

DanielNoord · 2023-05-08T15:20:40Z

astroid/nodes/_base_nodes.py

+        context.lookupname = self.name
+        context.constraints[self.name] = get_constraints(self, frame)
+
+        return bases._infer_stmts(stmts, context, frame)


I really like these new base classes, but don't really like how long this file is going to be and how many different types of bases nodes are in here. Not sure if this is fixable though.

We could burst some files later on but we have other issues to deal with imo. (Especially cyclic imports that make this harder)

astroid/nodes/node_classes.py

DanielNoord

Left some early review comments.

Benefits

...

🚀 👍

Drawbacks

There are some function-level imports created here that are symptoms of circular imports, but many of them are with the helpers module, and many of those are just for safe_infer, which could be extracted into its own helper module (it only needs UninferableBase). I don't think this jeopardizes the design.

Agreed!

However, I think we are missing one major drawback (although not necessarily a drawback of this design).
We still have the issue of these classes being behemoths that (imo) try to do too much. With this change, and possibly the inclusion of protocols.py on the nodes themselves as well, there is not that much left besides the astroid.nodes package. I still don't know how I feel about this as it feels we are putting more logic on the nodes than is completely necessary.
There are also still reference to AstroidManager() or MANAGER which makes it impossible to decouple the global state used by astroid without starting the pass an actual manager object in these signatures.

Todos:

Get benchmarks

If we agree on this design I think we shouldn't be held back by performance degradation too much. We can more easily improve performance when we can actually work with the code.

jacobtylerwalls · 2023-05-08T15:45:05Z

Thanks for the early feedback. I'll try to make some time over the next week or so to iron out the kinks. In the meantime, let's avoid merging changes to inference.py, because I've already deleted the file and could easily miss all the merge conflicts unless I check every commit on main.

codecov · 2023-05-11T23:53:03Z

Codecov Report

Merging #2171 (80c0391) into main (8d57ce2) will decrease coverage by 0.02%.
The diff coverage is 94.82%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2171      +/-   ##
==========================================
- Coverage   92.93%   92.91%   -0.02%     
==========================================
  Files          95       94       -1     
  Lines       10921    10926       +5     
==========================================
+ Hits        10149    10152       +3     
- Misses        772      774       +2

Flag	Coverage Δ
linux	`92.71% <94.82%> (-0.03%)`	⬇️
pypy	`91.01% <93.66%> (-0.06%)`	⬇️
windows	`92.48% <94.82%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
astroid/nodes/__init__.py	`100.00% <ø> (ø)`
astroid/nodes/node_classes.py	`94.90% <93.22%> (-0.67%)`	⬇️
astroid/nodes/_base_nodes.py	`97.06% <96.60%> (-1.03%)`	⬇️
astroid/__init__.py	`100.00% <100.00%> (ø)`
astroid/bases.py	`87.96% <100.00%> (+0.06%)`	⬆️
astroid/constraint.py	`100.00% <100.00%> (ø)`
astroid/filter_statements.py	`98.91% <100.00%> (ø)`
astroid/helpers.py	`94.54% <100.00%> (+0.24%)`	⬆️
astroid/manager.py	`89.12% <100.00%> (ø)`
astroid/nodes/scoped_nodes/mixin.py	`100.00% <100.00%> (ø)`
... and 2 more

... and 1 file with indirect coverage changes

DanielNoord

Still not sure how I feel about this. I think the use of Mixins and the import issues are not very nice, but you guys seem to agree that this is better so I'm also not sure how productive my opposition to this is 😄

astroid/helpers.py

ChangeLog

astroid/nodes/node_classes.py

DanielNoord · 2023-05-14T09:16:31Z

astroid/nodes/scoped_nodes/scoped_nodes.py

+    def _infer(
+        self, context: InferenceContext | None = None, **kwargs: Any
+    ) -> Generator[objects.Property | FunctionDef, None, InferenceErrorInfo]:
+        from astroid import objects  # pylint: disable=import-outside-toplevel


I think this is a case of pretty bad cyclic imports. With this approach we can't avoid this I think which makes me doubtful about whether this is the right approach.

Challenge accepted!

Here is a branch that removes these cyclic imports by moving PartialFunction and Property to scoped_nodes, which makes sense, since they inherit from FunctionDef anyway (what's the use of a Property being an object, anyway?)

The ease with which I could do this gives me hope that this is a good direction that can enable easier refactors in iterative PRs. What do you think?

If we go with the infer_object approach, we're freezing the inference/objects/protocols ontologies in place, instead of actually questioning if everything is where it should be, which we should feel free to do during breaking changes season ☔

See my comment on the other pull request. I don't necessarily think we would be freezing anything in place.

I do think that we should merge your PartialFunction branch to master irrespective of the approach. Seems like a very sensible change!

Maybe bursting the big node files is actually (one of) the way to solve circular imports?

astroid/protocols.py

DanielNoord

I have one nit, but I don't really mind merging this as is and seeing if there are smaller improvements later on. There is too much going on to make it perfect anyway.

While I still don't prefer this method it seems the discussion doesn't really take off and I don't want to block this any longer. I trust you fixed this and tested that pylint test suite still passes.

If Pierre still approves of this PR (the code, not the idea) then let's ship it 🚀

astroid/helpers.py

Add disables for function-level imports

astroid now better infers self.root() as [Uninferable, Instance of Global] instead of [Uninferable]. But due to control flow, only a Module will be returned, never a Global. astroid has ignore-on-opaque-inference set to No, unlike pylint.

jacobtylerwalls · 2023-06-28T03:55:55Z

Sounds good @DanielNoord I really appreciate the care it took to reach this point, and of course I'm open to infer_object() down the road if we start showing tangible results with it. 👍

I rebased and cleaned up the branch a bit.

Future PRs:

Remove the mixins, they're not essential (see comment) Remove three mixin nodes from #2171 #2231
moving PartialFunction and Property to scoped_nodes (see comment)
move safe_infer to its own module (see comment) Move safe_infer() to util #2232
maybe: remove monkey-patching from protocols.py like infer_binary_op

I trust you fixed this and tested that pylint test suite still passes.

I don't know what I fixed other than the monkey-patching 😉 but yes the pylint suite passes. There's plenty to do in pylint, we should cut an astroid alpha ASAP and add 3.12 support and remove future=True everywhere.

Pierre-Sassoulas

LGTM, amazing refactor there's a lot of work here, and it certainely makes astroid more undrstandable

Pierre-Sassoulas · 2023-07-02T04:17:39Z

astroid/nodes/_base_nodes.py

+        for result in infer_callable(context):
+            if isinstance(result, error):
+                # For the sake of .infer(), we don't care about operation
+                # errors, which is the job of pylint. So return something


Suggested change

# errors, which is the job of pylint. So return something

# errors, which is the job of a linter. So return something

Astroid Can be used without pylint :D

Hah, good point! I'll catch this in the next PR.

Pierre-Sassoulas · 2023-07-02T04:24:26Z

astroid/nodes/_base_nodes.py

+
+
+class AttributeNode(NodeNG):
+    expr: NodeNG


I also greatly dislike mixin for the sake of being dry because having to initialise instance in a particular order and not being able to use typecheck (becauseof course you can't guarantee the instanciation particular order and a type checker will rightfully warn you) is order if magnitude worse than whatever design problem mixin are trying to solve.

Pierre-Sassoulas · 2023-07-02T04:25:49Z

astroid/nodes/_base_nodes.py

+        context.lookupname = self.name
+        context.constraints[self.name] = get_constraints(self, frame)
+
+        return bases._infer_stmts(stmts, context, frame)


We could burst some files later on but we have other issues to deal with imo. (Especially cyclic imports that make this harder)

Pierre-Sassoulas · 2023-07-02T04:28:18Z

astroid/nodes/scoped_nodes/scoped_nodes.py

+    def _infer(
+        self, context: InferenceContext | None = None, **kwargs: Any
+    ) -> Generator[objects.Property | FunctionDef, None, InferenceErrorInfo]:
+        from astroid import objects  # pylint: disable=import-outside-toplevel


Maybe bursting the big node files is actually (one of) the way to solve circular imports?

jacobtylerwalls · 2023-07-02T11:45:02Z

Thanks so much for the reviews. I'll merge this and then get started on the followup PRs for removing the mixins and some of the circular imports.

Follow-up to 082774a.

jacobtylerwalls requested a review from DanielNoord May 8, 2023 15:00

jacobtylerwalls commented May 8, 2023

View reviewed changes

Pierre-Sassoulas reviewed May 8, 2023

View reviewed changes

DanielNoord reviewed May 8, 2023

View reviewed changes

jacobtylerwalls changed the title ~~[discussion] Move inference methods to nodes module(s)~~ Move inference methods to nodes module(s) May 12, 2023

jacobtylerwalls marked this pull request as ready for review May 12, 2023 00:11

jacobtylerwalls added the breaking-change label May 12, 2023

jacobtylerwalls requested a review from DanielNoord May 12, 2023 01:09

jacobtylerwalls added this to the 3.0.0a3 milestone May 12, 2023

This was referenced May 13, 2023

Create infer_object #2167

Closed

Remove all references to _infer #2168

Closed

DanielNoord reviewed May 14, 2023

View reviewed changes

Pierre-Sassoulas modified the milestones: 3.0.0a3, 3.0.0a4 May 14, 2023

jacobtylerwalls mentioned this pull request Jun 6, 2023

Checking incorrect usage of the keyword 'nonlocal' results in fatal errors. pylint-dev/pylint#8735

Closed

Pierre-Sassoulas modified the milestones: 3.0.0a4, 3.0.0a5 Jun 6, 2023

jacobtylerwalls mentioned this pull request Jun 7, 2023

Remove monkey-patching of methods onto classes #679

Closed

Pierre-Sassoulas modified the milestones: 3.0.0a5, 3.0.0a6 Jun 13, 2023

DanielNoord previously approved these changes Jun 27, 2023

View reviewed changes

astroid/helpers.py Show resolved Hide resolved

jacobtylerwalls dismissed DanielNoord’s stale review via b522ee9 June 28, 2023 03:28

jacobtylerwalls added 5 commits June 27, 2023 23:42

[refactor] Remove inference module

ed843e9

Move LookupMixIn

60f02de

Add disables for function-level imports

Review comments

e910116

Update Changelog

1688426

Add **kwargs to Dict._infer

fd17782

jacobtylerwalls added 5 commits June 27, 2023 23:46

Disable pointless-string-statement

3bc4588

Account for bad operator messages in generator yield types

27d150b

Add a no-member disable

c8f0d2e

astroid now better infers self.root() as [Uninferable, Instance of Global] instead of [Uninferable]. But due to control flow, only a Module will be returned, never a Global. astroid has ignore-on-opaque-inference set to No, unlike pylint.

Add attrname annotation

ceff22c

Remove _BaseContainer reference

76c10af

jacobtylerwalls force-pushed the inference-polymorphism branch from b522ee9 to 76c10af Compare June 28, 2023 03:46

Remove mypy disable

80c0391

jacobtylerwalls requested a review from Pierre-Sassoulas June 28, 2023 04:02

Pierre-Sassoulas approved these changes Jul 2, 2023

View reviewed changes

jacobtylerwalls merged commit 7c90b58 into pylint-dev:main Jul 2, 2023

jacobtylerwalls deleted the inference-polymorphism branch July 2, 2023 11:47

jacobtylerwalls added a commit to jacobtylerwalls/astroid that referenced this pull request Jul 2, 2023

Remove three mixin nodes from pylint-dev#2171

5644b04

Follow-up to 082774a.

jacobtylerwalls mentioned this pull request Jul 2, 2023

Move safe_infer() to util #2232

Merged

jacobtylerwalls added a commit that referenced this pull request Jul 2, 2023

Remove three mixin nodes from #2171 (#2231)

65df5e8

Follow-up to 082774a.

This was referenced Jul 4, 2023

Restore AssignAttr.infer_lhs() #2235

Merged

Raise when a name is inferred twice with the same context #2238

Merged


		# TODO: Move into _base_nodes. Blocked by import of _infer_stmts from bases.

	# errors, which is the job of pylint. So return something
	# errors, which is the job of a linter. So return something

Move inference methods to nodes module(s) #2171

Move inference methods to nodes module(s) #2171

Conversation

jacobtylerwalls commented May 8, 2023 • edited Loading

Type of Changes

Description

Benefits

Drawbacks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord left a comment

Choose a reason for hiding this comment

Benefits

Drawbacks

Todos:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielNoord left a comment

Choose a reason for hiding this comment

Benefits

Drawbacks

Todos:

jacobtylerwalls commented May 8, 2023

codecov bot commented May 11, 2023 • edited Loading

Codecov Report

DanielNoord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobtylerwalls May 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielNoord left a comment

Choose a reason for hiding this comment

jacobtylerwalls commented Jun 28, 2023 • edited Loading

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobtylerwalls commented Jul 2, 2023

jacobtylerwalls commented May 8, 2023 •

edited

Loading

codecov bot commented May 11, 2023 •

edited

Loading

jacobtylerwalls May 14, 2023 •

edited

Loading

jacobtylerwalls commented Jun 28, 2023 •

edited

Loading