Bug 905 #913

hippo91 · 2021-02-27T19:35:52Z

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 PR solves "Duplicates found in MROs" false positives.

The bug was not a regression as i first thought. @cdce8p detected that it was as old as #2717. The last astroid version just make it more present.
The bug is in fact the consequence of types aliasing in the typing module.
As the _alias function of the typing module returns _GenericAlias class, then this last class (and its inheritance tree) ends eventually in the inferred mro of the aliased class.
This way class V(MutableSet[T]) mro is inferred as `V => _GenericAlias => _Final => object.
But the correct mro should be "V => MutableSet => Set => Collection => Sized => Iterable => Container => Generic => object"

To mock the correct behavior, the typing_brain has been enriched with a new function. This function when receiving the call node corresponding to MutableSet = _alias(...) infer the MutableSet node name instead of inferring _alias call.
This way, we obtain the correct class (in this example MutableSet) with its correct mro. We just have to add a __getitem__ class method in the inferred class so that subscripting it is not detected as an error by astroid.

The mro obtained is the same as the one obtained by the python interpreter except the fact that in the inferred one, there is not typing.Generic but i think it is not a problem.

I tested pylint against this PR and all tests except one are ok. The failing one is try_except_raise_crash.py. Due to this PR improvment some false negatives are corrected and thus following messages appear and IMHO are correct:

tests/functional/t/try_except_raise_crash.py:11:0: W0223: Method '__delitem__' is abstract in class 'MutableMapping' but is not overridden (abstract-method)
tests/functional/t/try_except_raise_crash.py:11:0: W0223: Method '__getitem__' is abstract in class 'Mapping' but is not overridden (abstract-method)
tests/functional/t/try_except_raise_crash.py:11:0: W0223: Method '__iter__' is abstract in class 'Iterable' but is not overridden (abstract-method)
tests/functional/t/try_except_raise_crash.py:11:0: W0223: Method '__len__' is abstract in class 'Sized' but is not overridden (abstract-method)
tests/functional/t/try_except_raise_crash.py:11:0: W0223: Method '__setitem__' is abstract in class 'MutableMapping' but is not overridden (abstract-method)
tests/functional/t/try_except_raise_crash.py:15:0: W0223: Method '__delitem__' is abstract in class 'MutableMapping' but is not overridden (abstract-method)
tests/functional/t/try_except_raise_crash.py:15:0: W0223: Method '__getitem__' is abstract in class 'Mapping' but is not overridden (abstract-method)
tests/functional/t/try_except_raise_crash.py:15:0: W0223: Method '__iter__' is abstract in class 'Iterable' but is not overridden (abstract-method)
tests/functional/t/try_except_raise_crash.py:15:0: W0223: Method '__len__' is abstract in class 'Sized' but is not overridden (abstract-method)
tests/functional/t/try_except_raise_crash.py:15:0: W0223: Method '__setitem__' is abstract in class 'MutableMapping' but is not overridden (abstract-method)
tests/functional/t/try_except_raise_crash.py:22:11: E0712: Catching an exception which doesn't inherit from Exception: TestException (catching-non-exception)

Type of Changes

	Type
✓	🐛 Bug fix

Related Issue

Closes #905
Closes pylint-dev/pylint#4093
Closes pylint-dev/pylint#4131
Closes pylint-dev/pylint#4145
Closes pylint-dev/pylint#3247
Closes pylint-dev/pylint#2717

…_alias function

… infer the origin class : i.e MutableSet = _alias(collections.MutableSet ...) origin is the class in collections module. Needs to add __getitem method on its metaclass so that is support indexing (MutableSet[T]).

hippo91 · 2021-02-27T19:57:16Z

The python3.6 CI fails because (and maybe also pypy) the _alias function in the typing module appeared with python3.7.
I will make a fix ASAP.

Pierre-Sassoulas · 2021-02-27T20:04:34Z

ChangeLog

+  Closes PyCQA/pylint#4093 
+  Closes PyCQA/pylint#4131
+  Closes PyCQA/pylint#4145
+  Closes PyCQA/pylint#3247


Suggested change

Closes PyCQA/pylint#4093

Closes PyCQA/pylint#4131

Closes PyCQA/pylint#4145

Closes PyCQA/pylint#3247

Closes PyCQA/pylint#2717

Closes PyCQA/pylint#3247

Closes PyCQA/pylint#4093

Closes PyCQA/pylint#4131

Closes PyCQA/pylint#4145

Copied from the PR description + sorted

Pierre-Sassoulas · 2021-02-27T20:08:38Z

astroid/brain/brain_typing.py

+    if isinstance(node, nodes.Call) and isinstance(node.func, nodes.Name):
+        if node.func.name == "_alias" and isinstance(node.args[0], nodes.Attribute):
+            return True
+    return False


Suggested change

if isinstance(node, nodes.Call) and isinstance(node.func, nodes.Name):

if node.func.name == "_alias" and isinstance(node.args[0], nodes.Attribute):

return True

return False

return (

isinstance(node, nodes.Call) and

isinstance(node.func, nodes.Name) and

node.func.name == "_alias" and

isinstance(node.args[0], nodes.Attribute)

)

It might make sense to take a look at #908 after that. I updated the pre-commit config to use a current version of black.

I merged #908, a rebase is probably necessary. There are a lot of pre-commit check that are in pylint and not in astroid, I might update astroid configuration once we release 2.7.2, upgraded black is enough for now.

Pierre-Sassoulas · 2021-02-27T20:13:42Z

Good job on fixing false negative in pylint, I think maybe we can disable them as they're unrelated to the functional test they broke ? But it could also be kept and prevent regression? Let's first merge this one in astroid.

cdce8p · 2021-02-27T20:17:49Z

I'm noticing some issues with DefaultDict and Deque at the moment ...

hippo91 · 2021-02-27T20:33:18Z

@cdce8p can you give a snippet for the issues you are facing?

cdce8p · 2021-02-27T20:37:55Z

@cdce8p can you give a snippet for the issues you are facing?

@hippo91 Run pylint over

from typing import DefaultDict, OrderedDict
var: DefaultDict[int, str]
var2: OrderedDict[int, str]

--
Edit: Import was missing

cdce8p · 2021-02-27T21:06:35Z

@hippo91 I tried to analyze it a bit more. The following typing symbols seem to cause issue now:

typing.Deque
typing.Defaultdict
typing.OrderedDict
typing.Counter

Additionally I believe that transform might not be working correctly for:

typing.Pattern
typing.Match

cdce8p · 2021-02-27T21:19:27Z

As for Deque, Defaultdict, OrderedDict and Counter. Those seem to be define in C (not pure Python) and thus don't have the ABCMeta class. The result of which being that the __getitem__ method isn't added.

cdce8p

@hippo91 That should work for the first for. That leaves typing.Pattern and typing.Match.

cdce8p · 2021-02-27T21:38:37Z

astroid/brain/brain_typing.py

+def __getitem__(cls, value):
+    return cls
+"""
+


Suggested change

ABC_METACLASS_TEMPLATE = """

from abc import ABCMeta

ABCMeta

"""

cdce8p · 2021-02-27T21:39:22Z

astroid/brain/brain_typing.py

+    if res.metaclass():
+        res.metaclass().locals["__getitem__"] = [func_to_add]


Suggested change

if res.metaclass():

res.metaclass().locals["__getitem__"] = [func_to_add]

if res != astroid.Uninferable:

if not res.metaclass():

res._metaclass = extract_node(ABC_METACLASS_TEMPLATE)

res.metaclass().locals["__getitem__"] = [func_to_add]

else:

return

cdce8p · 2021-02-27T23:31:00Z

@hippo91 Some last commons for today:

Failing pylint tests
I opened a MR to fix them: pylint-dev/pylint#4152

collection - getitem
While working on the test, I noticed that, due to the change, instances of collection.xxx aren't detected as unsubscriptable anymore. Is there a way to decouple them typing and collection? If not, I believe it would be quite easy to add them back in pylint. Especially given the fact that with Python 3.9 they are subscriptable anyway.

import collection
import collection.abc
var1: collections.OrderedDict[str, int]
var2: collections.abc.Iterable[int]

Both should be unsubscriptable in Python 3.7 and 3.8.

typing.Pattern and typing.Match
It seems astroid can't infer the nodes correctly here. That might be because they aren't included in typing.__all__. Do you know a workaround for it?

Additional tests
Could you add additional test cases for typing.OrderedDict and typing.Pattern?

--

All in all, I really like your changes. Thanks for taking the time and working on it. Please let me know if I can help you any further.

…_alias function

… infer the origin class : i.e MutableSet = _alias(collections.MutableSet ...) origin is the class in collections module. Needs to add __getitem method on its metaclass so that is support indexing (MutableSet[T]).

…_905

hippo91 · 2021-02-28T15:16:28Z

@cdce8p thanks a lot for all your investigations. I appreciate a lot.
I will try to fix the bug with DefaultDict, OrderedDict ASAP.

@Pierre-Sassoulas it seems there is still a bit of work before getting this merge.

cdce8p · 2021-02-28T15:17:52Z

@hippo91 Could you rebase this branch onto master and then force push it?
It's really difficult to review otherwise.

I will try to fix the bug with DefaultDict, OrderedDict ASAP.

I think I already did just now. Will publish my changes after your rebase.

cdce8p · 2021-02-28T15:32:29Z

@hippo91 You can find my suggestion here: 1211992
I'm just missing additional tests right now.

cdce8p · 2021-02-28T16:32:46Z

Tests are now also done: cdce8p@2e15d63

Pierre-Sassoulas · 2021-02-28T16:46:41Z

Ideally we would release a fix for the crash today so this is available in 2.7.2 for debian (deadline march 1 ) . Will there be a way to release this fix in astroid and then release pylint today ? If not we might want to release what was fixed in pylint anyway and bump this issue to 2.7.3 ?
Edit: Apology the link was wrong.

cdce8p · 2021-02-28T16:52:21Z

@Pierre-Sassoulas I posted a working solution, so it's just a matter of rebasing, force-pushing and integrating. If it would be of any help, I could open a new MR with both the original idea from @hippo91 and my changes. Maybe that would be easier to review then. Let me know if I should do that.

Pierre-Sassoulas · 2021-02-28T16:55:05Z

Yeah, usually we can squash everything and merge without a rebase but this time it does not seems super wise.

cdce8p · 2021-02-28T16:56:22Z

Yeah, usually we can squash everything and merge without a rebase but this time it does not seems super wise.

Ok. Give me a few minutes. I'll tag you both so you get notified once I open the MR.

hippo91 · 2021-02-28T16:56:58Z

@cdce8p good idea. I am sorry but i will no longer have time to work for pylunt/astroid this week end. So if this patch is needed fast please do it as you feel. Thanks a lot.

PS : as i am not very used to rebase and force push can you give me the commands i would have to type?

cdce8p · 2021-02-28T16:59:52Z

PS : as i am not very used to rebase and force push can you give me the commands i would have to type?

Assuming the PyCQA/astroid remote is upstream

git fetch upstream master
git checkout bug_905
git rebase upstream/master

# Resolve all rebase conflicts
# Best done in an editor (like VS Code)
git add .
git rebase --continue
# Repeat until rebase is complete

git push -f # --force-with-lease to not erase code by mistake if it's a shared branch

Pierre-Sassoulas · 2021-02-28T17:08:29Z

Ok, I never realeased astroid, but I can start. This would be 2.5.1, right ?

cdce8p · 2021-02-28T17:12:18Z

Ok, I never realeased astroid, but I can start. This would be 2.5.1, right ?

👌🏻 What is you opinion on including any of #907, #914, #915 in the release?

Pierre-Sassoulas · 2021-02-28T17:41:21Z

Replaced by #916

hippo91 · 2021-03-05T07:54:39Z

PS : as i am not very used to rebase and force push can you give me the commands i would have to type?

Assuming the PyCQA/astroid remote is upstream
git fetch upstream master
git checkout bug_905
git rebase upstream/master

# Resolve all rebase conflicts
# Best done in an editor (like VS Code)
git add .
git rebase --continue
# Repeat until rebase is complete

git push -f # --force-with-lease to not erase code by mistake if it's a shared branch

Thanks for those guidelines @cdce8p and for all the hard work!

hippo91 added 7 commits February 27, 2021 10:28

Adds inference support for all typing types that are defined through …

a91c3af

…_alias function

Adds doc and type hints

feaddca

Adds a unit test for inferring _alias function of the typing module

a53ee8d

Formatting according to black

63c3d96

Adds an entry

4fbfd10

Removes useless import

b9a649f

hippo91 mentioned this pull request Feb 27, 2021

Fix duplicate bases error (MROs) with GenericAlias #910

Closed

2 tasks

Pierre-Sassoulas reviewed Feb 27, 2021

View reviewed changes

Pierre-Sassoulas approved these changes Feb 27, 2021

View reviewed changes

cdce8p added 5 commits February 27, 2021 21:23

Update pre-commit config

6173010

Update black version - tox

b20cc9c

Fix end of files

04b0481

Fix trailing whitespaces

fe8587b

Fix black issues

4037368

cdce8p reviewed Feb 27, 2021

View reviewed changes

cdce8p mentioned this pull request Feb 27, 2021

Modify tests for astroid update pylint-dev/pylint#4152

Merged

4 tasks

hippo91 added 4 commits February 28, 2021 14:52

Adds inference support for all typing types that are defined through …

b71974c

…_alias function

Adds doc and type hints

bf08bc1

Adds a unit test for inferring _alias function of the typing module

af0c14c

hippo91 added 7 commits February 28, 2021 14:52

Formatting according to black

5fcb911

Adds an entry

a580ce5

Removes useless import

e556215

Takes into account @Pierre-Sassoulas remarks

b9c86d3

Enable _alias mocking and testing only if python version is at least 3.7

20a7b65

Reformat (black)

3dcb5fb

Merge branch 'bug_905' of https://github.com/hippo91/astroid into bug…

9694507

…_905

cdce8p mentioned this pull request Feb 28, 2021

Fix typing alias #916

Merged

2 tasks

Pierre-Sassoulas closed this Feb 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 905 #913

Bug 905 #913

hippo91 commented Feb 27, 2021

hippo91 commented Feb 27, 2021

Pierre-Sassoulas Feb 27, 2021

Pierre-Sassoulas Feb 27, 2021

cdce8p Feb 27, 2021

Pierre-Sassoulas Feb 27, 2021

Pierre-Sassoulas commented Feb 27, 2021

cdce8p commented Feb 27, 2021

hippo91 commented Feb 27, 2021

cdce8p commented Feb 27, 2021 •

edited

Loading

cdce8p commented Feb 27, 2021 •

edited

Loading

cdce8p commented Feb 27, 2021 •

edited

Loading

cdce8p left a comment

cdce8p Feb 27, 2021 •

edited

Loading

cdce8p Feb 27, 2021 •

edited

Loading

cdce8p commented Feb 27, 2021

hippo91 commented Feb 28, 2021

cdce8p commented Feb 28, 2021

cdce8p commented Feb 28, 2021

cdce8p commented Feb 28, 2021

Pierre-Sassoulas commented Feb 28, 2021 •

edited

Loading

cdce8p commented Feb 28, 2021

Pierre-Sassoulas commented Feb 28, 2021

cdce8p commented Feb 28, 2021

hippo91 commented Feb 28, 2021

cdce8p commented Feb 28, 2021 •

edited by Pierre-Sassoulas

Loading

Pierre-Sassoulas commented Feb 28, 2021

cdce8p commented Feb 28, 2021

Pierre-Sassoulas commented Feb 28, 2021

hippo91 commented Mar 5, 2021

+ABC_METACLASS_TEMPLATE = """
+from abc import ABCMeta
+ABCMeta
+"""

		if res.metaclass():
		res.metaclass().locals["__getitem__"] = [func_to_add]

Bug 905 #913

Bug 905 #913

Conversation

hippo91 commented Feb 27, 2021

Steps

Description

Type of Changes

Related Issue

hippo91 commented Feb 27, 2021

Pierre-Sassoulas Feb 27, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas Feb 27, 2021

Choose a reason for hiding this comment

cdce8p Feb 27, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas Feb 27, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas commented Feb 27, 2021

cdce8p commented Feb 27, 2021

hippo91 commented Feb 27, 2021

cdce8p commented Feb 27, 2021 • edited Loading

cdce8p commented Feb 27, 2021 • edited Loading

cdce8p commented Feb 27, 2021 • edited Loading

cdce8p left a comment

Choose a reason for hiding this comment

cdce8p Feb 27, 2021 • edited Loading

Choose a reason for hiding this comment

cdce8p Feb 27, 2021 • edited Loading

Choose a reason for hiding this comment

cdce8p commented Feb 27, 2021

hippo91 commented Feb 28, 2021

cdce8p commented Feb 28, 2021

cdce8p commented Feb 28, 2021

cdce8p commented Feb 28, 2021

Pierre-Sassoulas commented Feb 28, 2021 • edited Loading

cdce8p commented Feb 28, 2021

Pierre-Sassoulas commented Feb 28, 2021

cdce8p commented Feb 28, 2021

hippo91 commented Feb 28, 2021

cdce8p commented Feb 28, 2021 • edited by Pierre-Sassoulas Loading

Pierre-Sassoulas commented Feb 28, 2021

cdce8p commented Feb 28, 2021

Pierre-Sassoulas commented Feb 28, 2021

hippo91 commented Mar 5, 2021

cdce8p commented Feb 27, 2021 •

edited

Loading

cdce8p commented Feb 27, 2021 •

edited

Loading

cdce8p commented Feb 27, 2021 •

edited

Loading

cdce8p Feb 27, 2021 •

edited

Loading

cdce8p Feb 27, 2021 •

edited

Loading

Pierre-Sassoulas commented Feb 28, 2021 •

edited

Loading

cdce8p commented Feb 28, 2021 •

edited by Pierre-Sassoulas

Loading