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

Avoid a global state in utils.builtin_lookup and avoid reinstantiating TransformVisitor #1563

Merged
merged 4 commits into from
May 22, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ Release date: TBA

* Allowed ``AstroidManager.clear_cache`` to reload necessary brain plugins.

* Fixed incorrect inferences after rebuilding the builtins module, e.g. by calling
``AstroidManager.clear_cache``.

Closes #1559

* Rename ``ModuleSpec`` -> ``module_type`` constructor parameter to match attribute
name and improve typing. Use ``type`` instead.

Expand Down
4 changes: 3 additions & 1 deletion astroid/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

from __future__ import annotations

import collections
import os
import types
import zipimport
Expand Down Expand Up @@ -377,7 +378,8 @@ def clear_cache(self) -> None:
clear_inference_tip_cache()

self.astroid_cache.clear()
AstroidManager.brain["_transform"] = TransformVisitor()
# NB: not a new TransformVisitor()
AstroidManager.brain["_transform"].transforms = collections.defaultdict(list)

for lru_cache in (
LookupMixIn.lookup,
Expand Down
15 changes: 7 additions & 8 deletions astroid/nodes/scoped_nodes/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

from __future__ import annotations

import builtins
from collections.abc import Sequence
from typing import TYPE_CHECKING

Expand All @@ -18,18 +17,18 @@
from astroid import nodes


_builtin_astroid: nodes.Module | None = None


def builtin_lookup(name: str) -> tuple[nodes.Module, Sequence[nodes.NodeNG]]:
"""Lookup a name in the builtin module.

Return the list of matching statements and the ast for the builtin module
"""
# pylint: disable-next=global-statement
global _builtin_astroid
if _builtin_astroid is None:
_builtin_astroid = AstroidManager().ast_from_module(builtins)
manager = AstroidManager()
try:
_builtin_astroid = manager.builtins_module
except KeyError:
# User manipulated the astroid cache directly! Rebuild everything.
manager.clear_cache()
_builtin_astroid = manager.builtins_module
if name == "__dict__":
return _builtin_astroid, ()
try:
Expand Down
2 changes: 2 additions & 0 deletions astroid/transforms.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ class TransformVisitor:
:meth:`~visit` with an *astroid* module and the class
will take care of the rest, walking the tree and running the
transforms for each encountered node.

Based on its usage in AstroidManager.brain, it should not be reinstantiated.
"""

def __init__(self):
Expand Down
15 changes: 14 additions & 1 deletion tests/unittest_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ def test_borg(self) -> None:
self.assertIs(built, second_built)


class ClearCacheTest(unittest.TestCase, resources.AstroidCacheSetupMixin):
class ClearCacheTest(unittest.TestCase):
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'd like to get out of the usage of this mixin. I think it masks tests that don't clean up after each other (in this case, the problems were upstream in other tests). We can keep fuzzing to find the problems.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for understanding the problem well enough so that we can stop fuzzing around πŸ˜„

def test_clear_cache_clears_other_lru_caches(self) -> None:
lrus = (
astroid.nodes.node_classes.LookupMixIn.lookup,
Expand Down Expand Up @@ -362,6 +362,19 @@ def test_brain_plugins_reloaded_after_clearing_cache(self) -> None:
inferred = next(format_call.infer())
self.assertIsInstance(inferred, Const)

def test_builtins_inference_after_clearing_cache(self) -> None:
astroid.MANAGER.clear_cache()
isinstance_call = astroid.extract_node("isinstance(1, int)")
inferred = next(isinstance_call.infer())
self.assertIs(inferred.value, True)

def test_builtins_inference_after_clearing_cache_manually(self) -> None:
# Not recommended to manipulate this, so we detect it and call clear_cache() instead
Copy link
Member Author

Choose a reason for hiding this comment

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

In trying to cover the missed lines I just decided to support this, since it's an easy mistake to make.

astroid.MANAGER.brain["astroid_cache"] = {}
isinstance_call = astroid.extract_node("isinstance(1, int)")
inferred = next(isinstance_call.infer())
self.assertIs(inferred.value, True)


if __name__ == "__main__":
unittest.main()