From d1b73d13d9fb75005bf3c2eb176aff9818f7762b Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Sun, 30 Apr 2023 09:40:03 -0400 Subject: [PATCH 01/13] Complete cache key for inference tip The cache key was lacking the `context` arg Co-authored-by: Sylvain Ackermann --- ChangeLog | 7 +++++++ astroid/inference_tip.py | 7 ++++--- tests/brain/test_brain.py | 10 ++-------- tests/test_regrtest.py | 21 +++++++++++++++++++++ tests/test_scoped_nodes.py | 4 +--- 5 files changed, 35 insertions(+), 14 deletions(-) diff --git a/ChangeLog b/ChangeLog index a760172d10..faccda681d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -17,6 +17,13 @@ Release date: TBA * Reduce file system access in ``ast_from_file()``. +* Fix incorrect cache keys for inference results, thereby correctly inferring types + for calls instantiating types dynamically. + + Closes #1828 + Closes pylint-dev/pylint#7464 + Closes pylint-dev/pylint#8074 + * ``nodes.FunctionDef`` no longer inherits from ``nodes.Lambda``. This is a breaking change but considered a bug fix as the nodes did not share the same API and were not interchangeable. diff --git a/astroid/inference_tip.py b/astroid/inference_tip.py index 041c0a684f..8b0da213ce 100644 --- a/astroid/inference_tip.py +++ b/astroid/inference_tip.py @@ -31,15 +31,16 @@ def _inference_tip_cached( def inner(*args: _P.args, **kwargs: _P.kwargs) -> Iterator[InferenceResult]: node = args[0] + context = args[1] try: - result = _cache[func, node] + result = _cache[func, node, context] # If through recursion we end up trying to infer the same # func + node we raise here. if result is None: raise UseInferenceDefault() except KeyError: - _cache[func, node] = None - result = _cache[func, node] = list(func(*args, **kwargs)) + _cache[func, node, context] = None + result = _cache[func, node, context] = list(func(*args, **kwargs)) assert result return iter(result) diff --git a/tests/brain/test_brain.py b/tests/brain/test_brain.py index 00a023ddb0..4a016868cb 100644 --- a/tests/brain/test_brain.py +++ b/tests/brain/test_brain.py @@ -930,13 +930,7 @@ class A: assert inferred.value == 42 def test_typing_cast_multiple_inference_calls(self) -> None: - """Inference of an outer function should not store the result for cast. - - https://github.com/pylint-dev/pylint/issues/8074 - - Possible solution caused RecursionErrors with Python 3.8 and CPython + PyPy. - https://github.com/pylint-dev/astroid/pull/1982 - """ + """Inference of an outer function should not store the result for cast.""" ast_nodes = builder.extract_node( """ from typing import TypeVar, cast @@ -954,7 +948,7 @@ def ident(var: T) -> T: i1 = next(ast_nodes[1].infer()) assert isinstance(i1, nodes.Const) - assert i1.value == 2 # should be "Hello"! + assert i1.value == "Hello" class ReBrainTest(unittest.TestCase): diff --git a/tests/test_regrtest.py b/tests/test_regrtest.py index 31d9e6b84b..59d344b954 100644 --- a/tests/test_regrtest.py +++ b/tests/test_regrtest.py @@ -336,6 +336,27 @@ def d(self): assert isinstance(inferred, Instance) assert inferred.qname() == ".A" + def test_inference_context_consideration(self) -> None: + """https://github.com/PyCQA/astroid/issues/1828""" + code = """ + class Base: + def return_type(self): + return type(self)() + class A(Base): + def method(self): + return self.return_type() + class B(Base): + def method(self): + return self.return_type() + A().method() #@ + B().method() #@ + """ + node1, node2 = extract_node(code) + inferred1 = next(node1.infer()) + assert inferred1.qname() == ".A" + inferred2 = next(node2.infer()) + assert inferred2.qname() == ".B" + class Whatever: a = property(lambda x: x, lambda x: x) # type: ignore[misc] diff --git a/tests/test_scoped_nodes.py b/tests/test_scoped_nodes.py index b8c55f67d3..86d69624d1 100644 --- a/tests/test_scoped_nodes.py +++ b/tests/test_scoped_nodes.py @@ -1771,9 +1771,7 @@ def __init__(self): "FinalClass", "ClassB", "MixinB", - # We don't recognize what 'cls' is at time of .format() call, only - # what it is at the end. - # "strMixin", + "strMixin", "ClassA", "MixinA", "intMixin", From 171c48bb80b66b0a6da3eb33d38a1ea8d612de52 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Sun, 30 Apr 2023 10:21:49 -0400 Subject: [PATCH 02/13] Adjust type annotation --- astroid/inference_tip.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/astroid/inference_tip.py b/astroid/inference_tip.py index 8b0da213ce..f190835087 100644 --- a/astroid/inference_tip.py +++ b/astroid/inference_tip.py @@ -10,13 +10,16 @@ from typing_extensions import ParamSpec +from astroid.context import InferenceContext from astroid.exceptions import InferenceOverwriteError, UseInferenceDefault from astroid.nodes import NodeNG from astroid.typing import InferenceResult, InferFn _P = ParamSpec("_P") -_cache: dict[tuple[InferFn, NodeNG], list[InferenceResult] | None] = {} +_cache: dict[ + tuple[InferFn, NodeNG, InferenceContext | None], list[InferenceResult] | None +] = {} def clear_inference_tip_cache() -> None: From 07719871e7ff8b113589cc26c464a862e20bb816 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Sun, 30 Apr 2023 11:39:06 -0400 Subject: [PATCH 03/13] Add is_empty() to InferenceContext --- astroid/context.py | 12 ++++++++++++ astroid/inference_tip.py | 3 +++ 2 files changed, 15 insertions(+) diff --git a/astroid/context.py b/astroid/context.py index 070c6f50e2..9562b8f68e 100644 --- a/astroid/context.py +++ b/astroid/context.py @@ -152,6 +152,18 @@ def restore_path(self): yield self.path = path + def is_empty(self) -> bool: + return ( + not self.path + and not self.nodes_inferred + and not self.callcontext + and not self.boundnode + and not self.lookupname + and not self.callcontext + and not self.extra_context + and not self.constraints + ) + def __str__(self) -> str: state = ( f"{field}={pprint.pformat(getattr(self, field), width=80 - len(field))}" diff --git a/astroid/inference_tip.py b/astroid/inference_tip.py index f190835087..d8eb7712d2 100644 --- a/astroid/inference_tip.py +++ b/astroid/inference_tip.py @@ -35,6 +35,9 @@ def _inference_tip_cached( def inner(*args: _P.args, **kwargs: _P.kwargs) -> Iterator[InferenceResult]: node = args[0] context = args[1] + if context.is_empty(): + # Fresh, empty contexts will defeat the cache. + context = None try: result = _cache[func, node, context] # If through recursion we end up trying to infer the same From 370b630ec33fe5906cec6b38b271a4b06c5b9685 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Sun, 30 Apr 2023 11:50:54 -0400 Subject: [PATCH 04/13] fixup! Add is_empty() --- astroid/inference_tip.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/astroid/inference_tip.py b/astroid/inference_tip.py index d8eb7712d2..4b87fe21c9 100644 --- a/astroid/inference_tip.py +++ b/astroid/inference_tip.py @@ -35,7 +35,7 @@ def _inference_tip_cached( def inner(*args: _P.args, **kwargs: _P.kwargs) -> Iterator[InferenceResult]: node = args[0] context = args[1] - if context.is_empty(): + if context is not None and context.is_empty(): # Fresh, empty contexts will defeat the cache. context = None try: From 38955bf992cd0dda6b95b8d48f282a3f0b4a42bb Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Sun, 30 Apr 2023 12:32:00 -0400 Subject: [PATCH 05/13] Add __hash__() and __eq__() to InferenceContext --- astroid/context.py | 31 ++++++++++++++++++++++--------- astroid/inference_tip.py | 3 --- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/astroid/context.py b/astroid/context.py index 9562b8f68e..52bb368ab5 100644 --- a/astroid/context.py +++ b/astroid/context.py @@ -152,16 +152,29 @@ def restore_path(self): yield self.path = path - def is_empty(self) -> bool: + def __hash__(self) -> int: + """Deliberately omit nodes_inferred and constraints. + + TODO: determine the performance upside of omitting more.""" + return hash( + ( + ((id(node), name) for node, name in self.path), + self.callcontext, + self.boundnode, + self.lookupname, + tuple(**self.extra_context), + ) + ) + + def __eq__(self, other) -> bool: return ( - not self.path - and not self.nodes_inferred - and not self.callcontext - and not self.boundnode - and not self.lookupname - and not self.callcontext - and not self.extra_context - and not self.constraints + self.path == other.path + and self.nodes_inferred == other.nodes_inferred + and self.callcontext == other.callcontext + and self.boundnode == other.boundnode + and self.lookupname == other.lookupname + and self.extra_context == other.extra_context + and self.constraints == other.constraints ) def __str__(self) -> str: diff --git a/astroid/inference_tip.py b/astroid/inference_tip.py index 4b87fe21c9..f190835087 100644 --- a/astroid/inference_tip.py +++ b/astroid/inference_tip.py @@ -35,9 +35,6 @@ def _inference_tip_cached( def inner(*args: _P.args, **kwargs: _P.kwargs) -> Iterator[InferenceResult]: node = args[0] context = args[1] - if context is not None and context.is_empty(): - # Fresh, empty contexts will defeat the cache. - context = None try: result = _cache[func, node, context] # If through recursion we end up trying to infer the same From b923101f927bab865cddcdd51c9bb70cc9ce01cb Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 30 Apr 2023 16:32:27 +0000 Subject: [PATCH 06/13] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- astroid/context.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/astroid/context.py b/astroid/context.py index 52bb368ab5..7c48d1ed66 100644 --- a/astroid/context.py +++ b/astroid/context.py @@ -154,7 +154,7 @@ def restore_path(self): def __hash__(self) -> int: """Deliberately omit nodes_inferred and constraints. - + TODO: determine the performance upside of omitting more.""" return hash( ( From 04b07357371b7e8aa1de6d32064e1f411b314114 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Sun, 30 Apr 2023 12:35:08 -0400 Subject: [PATCH 07/13] fixup! Add __hash__ --- astroid/context.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/astroid/context.py b/astroid/context.py index 7c48d1ed66..da9b5c1437 100644 --- a/astroid/context.py +++ b/astroid/context.py @@ -162,7 +162,7 @@ def __hash__(self) -> int: self.callcontext, self.boundnode, self.lookupname, - tuple(**self.extra_context), + tuple(self.extra_context.items()), ) ) From d8a810fd85bb046c374637706f6e6e89f26c65b2 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Sun, 30 Apr 2023 13:07:28 -0400 Subject: [PATCH 08/13] Restore the recursion guard using a partial cache key --- astroid/inference_tip.py | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/astroid/inference_tip.py b/astroid/inference_tip.py index f190835087..84fd60e07d 100644 --- a/astroid/inference_tip.py +++ b/astroid/inference_tip.py @@ -36,15 +36,23 @@ def inner(*args: _P.args, **kwargs: _P.kwargs) -> Iterator[InferenceResult]: node = args[0] context = args[1] try: - result = _cache[func, node, context] + _cache[func, node] + except KeyError: + # Good, no recursion issues. + try: + return _cache[func, node, context] # actual result + except KeyError: + # Recursion guard with a partial cache key. + _cache[func, node] = None + result = _cache[func, node, context] = list(func(*args, **kwargs)) + assert result + # Remove recursion guard. + del _cache[func, node] + else: # If through recursion we end up trying to infer the same # func + node we raise here. - if result is None: - raise UseInferenceDefault() - except KeyError: - _cache[func, node, context] = None - result = _cache[func, node, context] = list(func(*args, **kwargs)) - assert result + raise UseInferenceDefault() + return iter(result) return inner From cb5b291500f34743119fd4369ee458a2ac5aa7ce Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Sun, 30 Apr 2023 13:16:23 -0400 Subject: [PATCH 09/13] Remove `__eq__()` and `__hash__()` --- astroid/context.py | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/astroid/context.py b/astroid/context.py index da9b5c1437..070c6f50e2 100644 --- a/astroid/context.py +++ b/astroid/context.py @@ -152,31 +152,6 @@ def restore_path(self): yield self.path = path - def __hash__(self) -> int: - """Deliberately omit nodes_inferred and constraints. - - TODO: determine the performance upside of omitting more.""" - return hash( - ( - ((id(node), name) for node, name in self.path), - self.callcontext, - self.boundnode, - self.lookupname, - tuple(self.extra_context.items()), - ) - ) - - def __eq__(self, other) -> bool: - return ( - self.path == other.path - and self.nodes_inferred == other.nodes_inferred - and self.callcontext == other.callcontext - and self.boundnode == other.boundnode - and self.lookupname == other.lookupname - and self.extra_context == other.extra_context - and self.constraints == other.constraints - ) - def __str__(self) -> str: state = ( f"{field}={pprint.pformat(getattr(self, field), width=80 - len(field))}" From e054f407304aa3ad59b238c39583d3e960f075f7 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Sun, 30 Apr 2023 13:29:17 -0400 Subject: [PATCH 10/13] fixup! Restore --- astroid/inference_tip.py | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/astroid/inference_tip.py b/astroid/inference_tip.py index 84fd60e07d..53f03bbb41 100644 --- a/astroid/inference_tip.py +++ b/astroid/inference_tip.py @@ -21,6 +21,8 @@ tuple[InferFn, NodeNG, InferenceContext | None], list[InferenceResult] | None ] = {} +_CURRENTLY_INFERRING: set[tuple[InferFn, NodeNG]] = set() + def clear_inference_tip_cache() -> None: """Clear the inference tips cache.""" @@ -35,23 +37,20 @@ def _inference_tip_cached( def inner(*args: _P.args, **kwargs: _P.kwargs) -> Iterator[InferenceResult]: node = args[0] context = args[1] - try: - _cache[func, node] - except KeyError: - # Good, no recursion issues. - try: - return _cache[func, node, context] # actual result - except KeyError: - # Recursion guard with a partial cache key. - _cache[func, node] = None - result = _cache[func, node, context] = list(func(*args, **kwargs)) - assert result - # Remove recursion guard. - del _cache[func, node] - else: + partial_cache_key = (func, node) + if partial_cache_key in _CURRENTLY_INFERRING: # If through recursion we end up trying to infer the same # func + node we raise here. raise UseInferenceDefault() + try: + return _cache[func, node, context] + except KeyError: + # Recursion guard with a partial cache key. + _CURRENTLY_INFERRING.add(partial_cache_key) + result = _cache[func, node, context] = list(func(*args, **kwargs)) + assert result + # Remove recursion guard. + _CURRENTLY_INFERRING.remove(partial_cache_key) return iter(result) From e7354389e1d2cc5ccb7822a5b59314e2bd71d23b Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Sat, 6 May 2023 08:30:35 -0400 Subject: [PATCH 11/13] Add comment. --- astroid/inference_tip.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/astroid/inference_tip.py b/astroid/inference_tip.py index 53f03bbb41..f456a54b3d 100644 --- a/astroid/inference_tip.py +++ b/astroid/inference_tip.py @@ -41,11 +41,15 @@ def inner(*args: _P.args, **kwargs: _P.kwargs) -> Iterator[InferenceResult]: if partial_cache_key in _CURRENTLY_INFERRING: # If through recursion we end up trying to infer the same # func + node we raise here. - raise UseInferenceDefault() + raise UseInferenceDefault try: return _cache[func, node, context] except KeyError: # Recursion guard with a partial cache key. + # Using the full key causes a recursion error on PyPy. + # It's a pragmatic compromise to avoid so much recursive inference + # with slightly different contexts while still passing the simple + # test cases included with this commit. _CURRENTLY_INFERRING.add(partial_cache_key) result = _cache[func, node, context] = list(func(*args, **kwargs)) assert result From 0fa02331f73c080848c28291e338f60d94bf19fb Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Sat, 6 May 2023 08:40:55 -0400 Subject: [PATCH 12/13] Remove assertion --- astroid/inference_tip.py | 1 - 1 file changed, 1 deletion(-) diff --git a/astroid/inference_tip.py b/astroid/inference_tip.py index f456a54b3d..1bd57e7fbf 100644 --- a/astroid/inference_tip.py +++ b/astroid/inference_tip.py @@ -52,7 +52,6 @@ def inner(*args: _P.args, **kwargs: _P.kwargs) -> Iterator[InferenceResult]: # test cases included with this commit. _CURRENTLY_INFERRING.add(partial_cache_key) result = _cache[func, node, context] = list(func(*args, **kwargs)) - assert result # Remove recursion guard. _CURRENTLY_INFERRING.remove(partial_cache_key) From a79a2a74af4428bd5d4841b15f074b522ce4b969 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Sat, 6 May 2023 10:48:52 -0400 Subject: [PATCH 13/13] Remove optionality of list[InferenceResult] --- astroid/inference_tip.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/astroid/inference_tip.py b/astroid/inference_tip.py index 0c08fb345a..92cb6b4fe1 100644 --- a/astroid/inference_tip.py +++ b/astroid/inference_tip.py @@ -22,7 +22,7 @@ _P = ParamSpec("_P") _cache: dict[ - tuple[InferFn, NodeNG, InferenceContext | None], list[InferenceResult] | None + tuple[InferFn, NodeNG, InferenceContext | None], list[InferenceResult] ] = {} _CURRENTLY_INFERRING: set[tuple[InferFn, NodeNG]] = set()