Skip to content

Fix recursion error for inference of self-referencing class attribute #1392

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

Merged
merged 4 commits into from
Feb 21, 2022

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Feb 11, 2022

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 will close when merged pylint-dev/pylint#5408.

Type of Changes

Type
🐛 Bug fix

Related Issue

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

The actual change looks great, but I don't think we should change the way we currentely import typing and nodes.

@@ -3,16 +3,20 @@

"""Transform utilities (filters and decorator)"""

import typing

from typing import Any, Callable, Dict, Iterator, List, Optional, Tuple, Type, Union
Copy link
Member

Choose a reason for hiding this comment

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

It's imported this way in astroid because List, Dict, and Tuple are also astroid.nodes so it can become confusing. It's probably worth it to stay consistent accross the code base.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this file we only use 2 nodes and a lot of different typing stuff. That's why I opted to do it the other way around in this file.

I'm fine with reverting!

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep the old imports (at least for class name that are also astroid nodes).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reverted to only import typing.


import wrapt

from astroid.exceptions import InferenceOverwriteError
from astroid.nodes import NodeNG
from astroid import bases, nodes, util
Copy link
Member

Choose a reason for hiding this comment

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

There are some circular import in astroid, between files and between packages, I think importing only what's necessary and not whole package will help to understand what is happening in the code if we want to one day handle the issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On the other hand, importing submodules avoid such import errors as we will get notified of those patterns sooner.

But I'll revert if you want me to!

Copy link
Member

Choose a reason for hiding this comment

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

@cdce8p do you have an opinion ? It's been a long time since I tried to fix those circular imports, all I remember is the pain 😄

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Let's worry about circular import when they happen or when we fix them, thank you @DanielNoord

@Pierre-Sassoulas Pierre-Sassoulas merged commit 514c832 into pylint-dev:main Feb 21, 2022
@DanielNoord DanielNoord deleted the recursion branch February 21, 2022 19:57
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