Handle inference of `ComprehensionScope` better #1475

DanielNoord · 2022-03-15T15:58:03Z

Steps

For new features or bug fixes, add a ChangeLog entry describing what your PR does.
Write a good description on what the PR does.

Description

This makes it so that ComprehensionScopes are actually inferred. This requires some changes in pylint as we rely on the fact that these don't get inferred currently.
I have made a related PR to pylint which shows what these changes would be (pylint-dev/pylint#5923). I think it will be useful to review this simultaneously, as they show how this impacts astroid's output. That PR has now been merged.

One problem I couldn't overcome: this will break pre-commit for the project that merges this first. The change in behaviour changes various checks and we need to resolve an order in which these get merged. Pre-commit will be fixed once we have pylint 2.13 available here.

Let me know what you guys think!
We might want to add some additional tests.

Type of Changes

	Type
✓	🐛 Bug fix
✓	✨ New feature

Related Issue

Closes #135, closes #1404

DanielNoord · 2022-03-15T16:01:22Z

astroid/nodes/scoped_nodes/mixin.py

+    generators: List["nodes.Comprehension"]
+    """The generators that are looped through."""
+
+    def qname(self):


This is another instance of #278. We could also change qname() on LocalsDictNodeNG not to use name() if it doesn't exist.

I don't know what is better.

Suggested change

def qname(self):

def qname(self) -> str:

coveralls · 2022-03-25T15:43:07Z

Pull Request Test Coverage Report for Build 2268818330

13 of 21 (61.9%) changed or added relevant lines in 3 files are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.09%) to 91.541%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
astroid/helpers.py	2	4	50.0%
astroid/nodes/scoped_nodes/mixin.py	7	9	77.78%
astroid/nodes/scoped_nodes/scoped_nodes.py	4	8	50.0%

Files with Coverage Reduction	New Missed Lines	%
astroid/brain/brain_builtin_inference.py	3	89.88%

Totals
Change from base Build 2268781407:	-0.09%
Covered Lines:	9144
Relevant Lines:	9989

💛 - Coveralls

Pierre-Sassoulas

I have some questions, but this looks pretty good !

astroid/nodes/scoped_nodes/mixin.py

astroid/nodes/scoped_nodes/scoped_nodes.py

Pierre-Sassoulas · 2022-03-25T18:29:27Z

Forgot to point out that the coverage decreased by more than 0.05% and coverall did not fail. I must have set it up wrong.

Pierre-Sassoulas

The coverage decreased a lot, is this coverall acting up again ?

DanielNoord · 2022-03-26T23:22:25Z

Hmm I'll have a good look.

I think this might actually fix some StopIteration except handlers as we now "can" infer these Scope nodes. See https://coveralls.io/builds/47720462/source?filename=astroid%2Fbrain%2Fbrain_builtin_inference.py#L207.

Part of it is also the merge of main which @jacobtylerwalls mentioned in another PR today. That seems to give coveralls issues.

DanielNoord · 2022-04-17T07:55:08Z

@Pierre-Sassoulas Can we merge this and backport this to the upcoming patch release? Would be good to fix the coverage in pylint before 2.14.

Pierre-Sassoulas · 2022-04-17T18:32:46Z

Can we merge this and backport this to the upcoming patch release?

Sure, but do you mean astroid patch release or pylint's patch release, or both ?

DanielNoord · 2022-04-17T18:33:49Z

Can we merge this and backport this to the upcoming patch release?

Sure, but do you mean astroid patch release or pylint's patch release, or both ?

astroid patch!

jacobtylerwalls · 2022-04-17T19:01:44Z

astroid/nodes/scoped_nodes/mixin.py

+    def qname(self) -> str:
+        """Get the 'qualified' name of the node."""
+        return self.pytype()


@cdce8p do you have thoughts about this? In the past I believe you were a vote against, see #1284 (review).

DanielNoord · 2022-04-19T15:49:00Z

Moving this to 2.12 as we can realise the patch version without this.

cdce8p · 2022-05-09T15:21:54Z

astroid/nodes/scoped_nodes/mixin.py

+    def _infer(
+        self: _T, context: Optional["InferenceContext"] = None, **kwargs: Any
+    ) -> Iterator[_T]:
+        yield self


What do we get by returning self, i.e. the AST node, here?

Do you mean return self instead of yield self?

cdce8p · 2022-05-09T15:27:30Z

tests/unittest_inference.py

        dict_comp = next(module["dict_comp"].infer())
-        self.assertEqual(dict_comp, util.Uninferable)
+        assert isinstance(dict_comp, nodes.DictComp)
        set_comp = next(module["set_comp"].infer())
-        self.assertEqual(set_comp, util.Uninferable)
+        assert isinstance(set_comp, nodes.SetComp)
        list_comp = next(module["list_comp"].infer())
-        self.assertEqual(list_comp, util.Uninferable)
+        assert isinstance(list_comp, nodes.ListComp)


Maybe I'm misunderstanding the concept of inference here. If so, please correct me.

However, the way I see it inference means we look at the AST node and tell the caller what the Python result will be without actually executing the code. To give an example

lst = [x for x in range(3)]

The inference result for lst shouldn't be the list comprehension itself. Instead it should be the AST node for a list with

[0, 1, 2]

Same for generator expressions, dict and set comprehensions.

That is much much harder to do which is likely we it wasn't done already.

Hm, yeah I didn't think of it like that. That would decouple the value of lst from the actual ast tree though.

For example, we would then have a ListComp node in nodes.Module.body but a List node for lst? If we create that List when calling _infer on ListComp what would then be the lineno etc.?

Wouldn't it be better to say that lst is a ListComp and let downstream users decide if they want to handle ListComp and List similarly? Like we often do with FunctionDef and AsyncFunctionDef.

That would decouple the value of lst from the actual ast tree though.

Yeah, it's an inference result after all.

If we create that List when calling _infer on ListComp what would then be the lineno etc.?

None? Both lineno and col_offset attributes are typed as optional.
Or the same one as the original ListComp.

Wouldn't it be better to say that lst is a ListComp and let downstream users decide if they want to handle ListComp and List similarly? Like we often do with FunctionDef and AsyncFunctionDef.

The downstream user still has full control over it. If he once the "raw" data, he can just use the node itself. No need to call infer for that. Regarding FunctionDef, the infer method can actually return a Property if the function is decorated. Similarly, inferring a DictUnpack will return a single dict or a comparison just a bool constant

{1: 'A', **{2: 'B'}} True == True

I guess we should probably handle this comprehension per comprehension then. Instead of one larger PR.

I guess we should probably handle this comprehension per comprehension then. Instead of one larger PR.

Yeah. There are multiple parts to be aware of: evaluating the iterator, matching the loop var(s) with optional sequence unpacking, checking any if statements and doing late assignments via :=, evaluating the loop expression. Likely I'm forgetting something.

[a + c for a, b in func() if (c := b * 2)]

If you, or someone else wants to go down that route, it's probably best to start with a fairly limited set constraints and only one comprehension type. Once that is down extending it should be fairly easy in comparison.

cdce8p · 2022-05-10T14:04:03Z

Going to close this PR since we would need a different approach for inference.
See #1475 (comment)

DanielNoord added 4 commits March 15, 2022 16:24

Define generators on ComprehensionScope

cf1d77b

Define pytype on ComprehensionScope

db2fbaa

Make ComprehensionScope infer as self

ab48be3

Add changelog

0614091

DanielNoord added Enhancement ✨ Improvement to a component pylint-tested PRs that don't cause major regressions with pylint labels Mar 15, 2022

DanielNoord mentioned this pull request Mar 15, 2022

Handle ComprehensionScope and their behaviour better pylint-dev/pylint#5923

Merged

2 tasks

DanielNoord commented Mar 15, 2022

View reviewed changes

This was referenced Mar 15, 2022

Define generators on ComprehensionScope #1476

Merged

dict comprehension is not inferred as dict #1404

Open

Pierre-Sassoulas added this to the 2.12.0 milestone Mar 15, 2022

DanielNoord added 3 commits March 25, 2022 16:30

Merge branch 'main' of https://github.com/PyCQA/astroid into dict

a3c3e06

Fixes

84c1df3

Update astroid/nodes/scoped_nodes/mixin.py

df71eec

DanielNoord requested a review from Pierre-Sassoulas March 25, 2022 15:39

Pierre-Sassoulas reviewed Mar 25, 2022

View reviewed changes

astroid/nodes/scoped_nodes/mixin.py Show resolved Hide resolved

astroid/nodes/scoped_nodes/scoped_nodes.py Outdated Show resolved Hide resolved

astroid/nodes/scoped_nodes/scoped_nodes.py Show resolved Hide resolved

DanielNoord added 2 commits March 26, 2022 23:04

Fix mistake

a30d378

Merge branch 'dict' of https://github.com/DanielNoord/astroid into dict

ee7d066

DanielNoord requested a review from Pierre-Sassoulas March 26, 2022 23:18

Pierre-Sassoulas reviewed Mar 26, 2022

View reviewed changes

Pierre-Sassoulas modified the milestones: 2.12.0, 2.11.3 Apr 17, 2022

Pierre-Sassoulas added the needs-backport label Apr 17, 2022

jacobtylerwalls reviewed Apr 17, 2022

View reviewed changes

DanielNoord modified the milestones: 2.11.3, 2.12.0 Apr 19, 2022

Pierre-Sassoulas removed the needs-backport label Apr 19, 2022

DanielNoord added 2 commits May 4, 2022 11:04

Merge branch 'main' of https://github.com/PyCQA/astroid into dict

c9ad1d8

Use _infer instead

fc971a9

DanielNoord requested a review from cdce8p May 4, 2022 09:11

cdce8p reviewed May 9, 2022

View reviewed changes

cdce8p closed this May 10, 2022

DanielNoord deleted the dict branch May 10, 2022 14:16

jacobtylerwalls mentioned this pull request Jun 11, 2022

[RFC] Yield self when inferring GeneratorExp #1615

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle inference of `ComprehensionScope` better #1475

Handle inference of `ComprehensionScope` better #1475

DanielNoord commented Mar 15, 2022 •

edited

Loading

DanielNoord Mar 15, 2022

DanielNoord Mar 25, 2022

coveralls commented Mar 25, 2022 •

edited

Loading

Pierre-Sassoulas left a comment

Pierre-Sassoulas commented Mar 25, 2022

Pierre-Sassoulas left a comment

DanielNoord commented Mar 26, 2022

DanielNoord commented Apr 17, 2022

Pierre-Sassoulas commented Apr 17, 2022

DanielNoord commented Apr 17, 2022

jacobtylerwalls Apr 17, 2022

DanielNoord commented Apr 19, 2022

cdce8p May 9, 2022

DanielNoord May 9, 2022

cdce8p May 9, 2022

DanielNoord May 9, 2022

cdce8p May 9, 2022

DanielNoord May 9, 2022

cdce8p May 9, 2022

cdce8p commented May 10, 2022

Handle inference of ComprehensionScope better #1475

Handle inference of ComprehensionScope better #1475

Conversation

DanielNoord commented Mar 15, 2022 • edited Loading

Steps

Description

Type of Changes

Related Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 25, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2268818330

💛 - Coveralls

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas commented Mar 25, 2022

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord commented Mar 26, 2022

DanielNoord commented Apr 17, 2022

Pierre-Sassoulas commented Apr 17, 2022

DanielNoord commented Apr 17, 2022

Choose a reason for hiding this comment

DanielNoord commented Apr 19, 2022

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

cdce8p commented May 10, 2022

Handle inference of `ComprehensionScope` better #1475

Handle inference of `ComprehensionScope` better #1475

DanielNoord commented Mar 15, 2022 •

edited

Loading

coveralls commented Mar 25, 2022 •

edited

Loading