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

Fix duplicate bases error (MROs) with GenericAlias #910

Closed
wants to merge 3 commits into from

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Feb 23, 2021

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

Fix duplicate bases error that occurred for Python <3.9 when using multiple inheritance from _GenericAlias. Starting with 3.9 this is parsed differently as _SpecialGenericAlias and _BaseGenericAlias.

--

As this is a bug fix, it would be great if it could be included in the next release 2.5.1.

Type of Changes

Type
🐛 Bug fix
✨ New feature
🔨 Refactoring
📜 Docs

Related Issue

Closes #905

@cdce8p cdce8p force-pushed the fix-duplicate-bases branch from 9621726 to 23edbea Compare February 23, 2021 01:02
Copy link
Contributor

@hippo91 hippo91 left a comment

Choose a reason for hiding this comment

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

@cdce8p thanks for this review.
However i would prefer we wait a little bit to merge it because i think there is probably a better way to handle this.
I would like to dig in a bit more.

Comment on lines 108 to 113
if (
names
and names[0] is not None
and last_index[names[0]] != 0
and names[0][1] != "typing._GenericAlias"
):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not found of this kind of solution. I think there is maybe something to handle through TypingBrain instead.
However i'm not sure, i'm still investigating this.
By the way on my machine the bug is also present with python 3.9.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand. Thanks for your work!

By the way on my machine the bug is also present with python 3.9.

Hmm, interesting. MacOS with Py 3.9.2 works. Please let me know if I can help test things, once you have found a better solution.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 27, 2021

I took another look at it and even tried to modify brain_typing.py: de223ec
That worked. However I quickly noticed that we would have to add all generic aliases from the typing module to fix all issues which would be a lot of work.

After some more investigation, I found that pylint normally would raise a duplicate-bases (E0241) error in such cases. There is even an existing false-positive issue pylint-dev/pylint#2717. Since this wasn't the case, it meant that the astroid.DuplicateBasesError was raised unexpectedly.

That's when I though about adding a flag to guard the exception during the ast parsing which I've done in the last commit.
This resolves the issue and only leaves the false-positive for E0241 which can be ignored by pylint.

--

A good test example is:

from typing import Sized, Hashable
class Derived(Sized, Hashable):
    def __init__(self):
        self.var = 1

@hippo91
Copy link
Contributor

hippo91 commented Feb 27, 2021

@cdce8p i opened #913 that try to fix the same issue.
IMHO it is more generic than what you proposes here.
I would greatly appreciate your opinion on it, as the one of @AWhetter and @Pierre-Sassoulas.
It still failing on pypy and python3.6. I will investigate it ASAP.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 27, 2021

@cdce8p i opened #913 that try to fix the same issue.
IMHO it is more generic than what you proposes here.
I would greatly appreciate your opinion on it, as the one of @AWhetter and @Pierre-Sassoulas.
It still failing on pypy and python3.6. I will investigate it ASAP.

@hippo91 I just saw it. On first glance, your solution does look quite elegant (and way better than mine). I'll run some tests on my code to see if the issue is completely fixed and will get back to you.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 27, 2021

Replaced by #913

@cdce8p cdce8p closed this Feb 27, 2021
@cdce8p cdce8p deleted the fix-duplicate-bases branch February 27, 2021 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicates found in MROs (OrderedSet)
3 participants