From f6b8876170ceb446d095dab39832187f46c44e96 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Sun, 18 Feb 2024 11:16:47 -0500 Subject: [PATCH 01/13] Log RecursionErrors out as warnings during node transformation --- ChangeLog | 3 + astroid/transforms.py | 14 +- tests/test_transforms.py | 19 +++ tests/testdata/python3/recursion_error.py | 170 ++++++++++++++++++++++ 4 files changed, 205 insertions(+), 1 deletion(-) create mode 100644 tests/testdata/python3/recursion_error.py diff --git a/ChangeLog b/ChangeLog index d06026c5e1..c8ae95e6a7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -25,6 +25,9 @@ Release date: TBA * Include modname in AST warnings. Useful for ``invalid escape sequence`` warnings with Python 3.12. +* ``RecursionError`` is now trapped and logged out as ``UserWarning`` during astroid node transformations with instructions about raising the system recursion limit. + + Closes pylint-dev/pylint#8842 What's New in astroid 3.0.3? diff --git a/astroid/transforms.py b/astroid/transforms.py index c6fe51170a..0d9c22e966 100644 --- a/astroid/transforms.py +++ b/astroid/transforms.py @@ -4,6 +4,7 @@ from __future__ import annotations +import warnings from collections import defaultdict from collections.abc import Callable from typing import TYPE_CHECKING, List, Optional, Tuple, TypeVar, Union, cast, overload @@ -110,7 +111,18 @@ def _visit_generic(self, node: _Vistables) -> _VisitReturns: if not node or isinstance(node, str): return node - return self._visit(node) + try: + return self._visit(node) + except RecursionError: + # Returning the node untransformed is better than giving up. + warnings.warn( + f"Astroid was unable to transform {node}.\n" + "Some functionality will be missing unless the system recursion limit is lifted.\n" + "From pylint, try: --init-hook='import sys; sys.setrecursionlimit(2000)' or higher.", + UserWarning, + stacklevel=0, + ) + return node def register_transform( self, diff --git a/tests/test_transforms.py b/tests/test_transforms.py index 59aaf2100d..7e9bcc32b3 100644 --- a/tests/test_transforms.py +++ b/tests/test_transforms.py @@ -5,14 +5,17 @@ from __future__ import annotations import contextlib +import sys import time import unittest from collections.abc import Callable, Iterator from astroid import MANAGER, builder, nodes, parse, transforms +from astroid.brain.brain_dataclasses import _looks_like_dataclass_field_call from astroid.manager import AstroidManager from astroid.nodes.node_classes import Call, Compare, Const, Name from astroid.nodes.scoped_nodes import FunctionDef, Module +from tests.testdata.python3.recursion_error import LONG_CHAINED_METHOD_CALL @contextlib.contextmanager @@ -258,3 +261,19 @@ def transform_class(cls): import UserDict """ ) + + def test_transform_aborted_if_recursion_limited(self): + def transform_call(node: Call) -> Const: + return node + + self.transformer.register_transform( + nodes.Call, transform_call, _looks_like_dataclass_field_call + ) + original_limit = sys.getrecursionlimit() + sys.setrecursionlimit(1000) + + try: + with self.assertWarns(UserWarning): + self.parse_transform(LONG_CHAINED_METHOD_CALL) + finally: + sys.setrecursionlimit(original_limit) diff --git a/tests/testdata/python3/recursion_error.py b/tests/testdata/python3/recursion_error.py new file mode 100644 index 0000000000..770da04038 --- /dev/null +++ b/tests/testdata/python3/recursion_error.py @@ -0,0 +1,170 @@ +LONG_CHAINED_METHOD_CALL = """ +from a import b + +( + b.builder('name') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .add('name', value='value') + .Build() +)""" From 91a5e4a66f2ce4b442bcf4058d28ca894f5ef2f5 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Sun, 18 Feb 2024 11:18:10 -0500 Subject: [PATCH 02/13] Trap RecursionError in `visit_attribute` --- astroid/nodes/as_string.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/astroid/nodes/as_string.py b/astroid/nodes/as_string.py index 3200061a47..2d5fd6bd5b 100644 --- a/astroid/nodes/as_string.py +++ b/astroid/nodes/as_string.py @@ -363,7 +363,10 @@ def visit_generatorexp(self, node) -> str: def visit_attribute(self, node) -> str: """return an astroid.Getattr node as string""" - left = self._precedence_parens(node, node.expr) + try: + left = self._precedence_parens(node, node.expr) + except RecursionError: + left = f"({node.expr.accept(self)})" if left.isdigit(): left = f"({left})" return f"{left}.{node.attrname}" From 72b888a8c5a98d7d8581e2a50b71f0c58f8da7cd Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Sun, 18 Feb 2024 15:00:46 -0500 Subject: [PATCH 03/13] Adjust test for PyPy? --- tests/test_transforms.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/test_transforms.py b/tests/test_transforms.py index 7e9bcc32b3..e96e0563ab 100644 --- a/tests/test_transforms.py +++ b/tests/test_transforms.py @@ -12,6 +12,7 @@ from astroid import MANAGER, builder, nodes, parse, transforms from astroid.brain.brain_dataclasses import _looks_like_dataclass_field_call +from astroid.const import IS_PYPY from astroid.manager import AstroidManager from astroid.nodes.node_classes import Call, Compare, Const, Name from astroid.nodes.scoped_nodes import FunctionDef, Module @@ -269,8 +270,13 @@ def transform_call(node: Call) -> Const: self.transformer.register_transform( nodes.Call, transform_call, _looks_like_dataclass_field_call ) - original_limit = sys.getrecursionlimit() - sys.setrecursionlimit(1000) + + if IS_PYPY: + original_limit = 1000 # pypy doesn't expose this + sys.setrecursionlimit(100) # set something super low + else: + original_limit = sys.getrecursionlimit() + sys.setrecursionlimit(1000) # use the default try: with self.assertWarns(UserWarning): From 491833b3d147c0e70f67e10a42d0195772c6a356 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Sun, 18 Feb 2024 15:05:01 -0500 Subject: [PATCH 04/13] Adjust PyPy recursion limit in test --- tests/test_transforms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_transforms.py b/tests/test_transforms.py index e96e0563ab..9578b4e19e 100644 --- a/tests/test_transforms.py +++ b/tests/test_transforms.py @@ -273,7 +273,7 @@ def transform_call(node: Call) -> Const: if IS_PYPY: original_limit = 1000 # pypy doesn't expose this - sys.setrecursionlimit(100) # set something super low + sys.setrecursionlimit(600) else: original_limit = sys.getrecursionlimit() sys.setrecursionlimit(1000) # use the default From 4723ed5a761d2426294ffc16a4471f1ff55980c9 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Mon, 19 Feb 2024 07:51:35 -0500 Subject: [PATCH 05/13] Simplify pypy workaround; assert msg --- tests/test_transforms.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/tests/test_transforms.py b/tests/test_transforms.py index 9578b4e19e..32c2910578 100644 --- a/tests/test_transforms.py +++ b/tests/test_transforms.py @@ -271,15 +271,13 @@ def transform_call(node: Call) -> Const: nodes.Call, transform_call, _looks_like_dataclass_field_call ) - if IS_PYPY: - original_limit = 1000 # pypy doesn't expose this - sys.setrecursionlimit(600) - else: - original_limit = sys.getrecursionlimit() - sys.setrecursionlimit(1000) # use the default + original_limit = sys.getrecursionlimit() + sys.setrecursionlimit(600 if IS_PYPY else 1000) try: - with self.assertWarns(UserWarning): + with self.assertWarns( + UserWarning, msg="try: --init-hook='import sys; sys.setrecursionlimit" + ): self.parse_transform(LONG_CHAINED_METHOD_CALL) finally: sys.setrecursionlimit(original_limit) From a27f82718b2025146c8ccae444aabf28e53ac868 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Mon, 19 Feb 2024 08:40:11 -0500 Subject: [PATCH 06/13] Try lowering pypy recursion limit? --- tests/test_transforms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_transforms.py b/tests/test_transforms.py index 32c2910578..cfa5ad0377 100644 --- a/tests/test_transforms.py +++ b/tests/test_transforms.py @@ -272,7 +272,7 @@ def transform_call(node: Call) -> Const: ) original_limit = sys.getrecursionlimit() - sys.setrecursionlimit(600 if IS_PYPY else 1000) + sys.setrecursionlimit(400 if IS_PYPY else 1000) try: with self.assertWarns( From 1f4fc795fcb2de03d397c6cba8b751f95c8c4666 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Mon, 19 Feb 2024 08:44:51 -0500 Subject: [PATCH 07/13] Last try --- tests/test_transforms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_transforms.py b/tests/test_transforms.py index cfa5ad0377..ef18dbabca 100644 --- a/tests/test_transforms.py +++ b/tests/test_transforms.py @@ -272,7 +272,7 @@ def transform_call(node: Call) -> Const: ) original_limit = sys.getrecursionlimit() - sys.setrecursionlimit(400 if IS_PYPY else 1000) + sys.setrecursionlimit(500 if IS_PYPY else 1000) try: with self.assertWarns( From b2ff23d15948b4f49b47606e3d9b0f63e8b955a1 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Thu, 22 Feb 2024 20:35:38 -0500 Subject: [PATCH 08/13] Use pytest.warns --- tests/test_transforms.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/test_transforms.py b/tests/test_transforms.py index ef18dbabca..460868e02b 100644 --- a/tests/test_transforms.py +++ b/tests/test_transforms.py @@ -10,6 +10,8 @@ import unittest from collections.abc import Callable, Iterator +import pytest + from astroid import MANAGER, builder, nodes, parse, transforms from astroid.brain.brain_dataclasses import _looks_like_dataclass_field_call from astroid.const import IS_PYPY @@ -275,9 +277,8 @@ def transform_call(node: Call) -> Const: sys.setrecursionlimit(500 if IS_PYPY else 1000) try: - with self.assertWarns( - UserWarning, msg="try: --init-hook='import sys; sys.setrecursionlimit" - ): + with pytest.warns(UserWarning) as records: self.parse_transform(LONG_CHAINED_METHOD_CALL) + assert "sys.setrecursionlimit" in records[0].message.args[0] finally: sys.setrecursionlimit(original_limit) From 6920c61d7debca3876b9a6d8163b0090017da83d Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Fri, 23 Feb 2024 08:00:36 -0500 Subject: [PATCH 09/13] Add test and additional warning --- astroid/nodes/as_string.py | 6 ++++++ tests/test_nodes.py | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/astroid/nodes/as_string.py b/astroid/nodes/as_string.py index 2d5fd6bd5b..f36115ff6e 100644 --- a/astroid/nodes/as_string.py +++ b/astroid/nodes/as_string.py @@ -6,6 +6,7 @@ from __future__ import annotations +import warnings from collections.abc import Iterator from typing import TYPE_CHECKING @@ -366,6 +367,11 @@ def visit_attribute(self, node) -> str: try: left = self._precedence_parens(node, node.expr) except RecursionError: + warnings.warn( + "Recursion limit exhausted; defaulting to adding parentheses.", + UserWarning, + stacklevel=2, + ) left = f"({node.expr.accept(self)})" if left.isdigit(): left = f"({left})" diff --git a/tests/test_nodes.py b/tests/test_nodes.py index 6ea25fd846..a7b4d79ec4 100644 --- a/tests/test_nodes.py +++ b/tests/test_nodes.py @@ -47,6 +47,7 @@ Tuple, ) from astroid.nodes.scoped_nodes import ClassDef, FunctionDef, GeneratorExp, Module +from tests.testdata.python3.recursion_error import LONG_CHAINED_METHOD_CALL from . import resources @@ -279,6 +280,15 @@ def test_as_string_unknown() -> None: assert nodes.Unknown().as_string() == "Unknown.Unknown()" assert nodes.Unknown(lineno=1, col_offset=0).as_string() == "Unknown.Unknown()" + @staticmethod + def test_recursion_error_trapped() -> None: + with pytest.warns(UserWarning, match="unable to transform"): + ast = abuilder.string_build(LONG_CHAINED_METHOD_CALL) + + attribute = ast.body[1].value.func + with pytest.raises(UserWarning): + attribute.as_string() + @pytest.mark.skipif(not PY312_PLUS, reason="Uses 3.12 type param nodes") class AsStringTypeParamNodes(unittest.TestCase): From 8e404a9af041bd9320ccd35dcb3d5260f2440169 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Fri, 23 Feb 2024 08:11:41 -0500 Subject: [PATCH 10/13] Adjust for pypy again --- tests/test_nodes.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/tests/test_nodes.py b/tests/test_nodes.py index a7b4d79ec4..de62b2f7e2 100644 --- a/tests/test_nodes.py +++ b/tests/test_nodes.py @@ -29,7 +29,7 @@ transforms, util, ) -from astroid.const import PY310_PLUS, PY312_PLUS, Context +from astroid.const import IS_PYPY, PY310_PLUS, PY312_PLUS, Context from astroid.context import InferenceContext from astroid.exceptions import ( AstroidBuildingError, @@ -282,12 +282,19 @@ def test_as_string_unknown() -> None: @staticmethod def test_recursion_error_trapped() -> None: - with pytest.warns(UserWarning, match="unable to transform"): - ast = abuilder.string_build(LONG_CHAINED_METHOD_CALL) + original_limit = sys.getrecursionlimit() + if IS_PYPY: + sys.setrecursionlimit(500) - attribute = ast.body[1].value.func - with pytest.raises(UserWarning): - attribute.as_string() + try: + with pytest.warns(UserWarning, match="unable to transform"): + ast = abuilder.string_build(LONG_CHAINED_METHOD_CALL) + + with pytest.raises(UserWarning): + attribute = ast.body[1].value.func + assert attribute.as_string() + finally: + sys.setrecursionlimit(original_limit) @pytest.mark.skipif(not PY312_PLUS, reason="Uses 3.12 type param nodes") From 63f54b88e7e4457d1044183209d92fa95183dc27 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Fri, 23 Feb 2024 08:16:12 -0500 Subject: [PATCH 11/13] Revert "Adjust for pypy again" This reverts commit 8e404a9af041bd9320ccd35dcb3d5260f2440169. --- tests/test_nodes.py | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/tests/test_nodes.py b/tests/test_nodes.py index de62b2f7e2..a7b4d79ec4 100644 --- a/tests/test_nodes.py +++ b/tests/test_nodes.py @@ -29,7 +29,7 @@ transforms, util, ) -from astroid.const import IS_PYPY, PY310_PLUS, PY312_PLUS, Context +from astroid.const import PY310_PLUS, PY312_PLUS, Context from astroid.context import InferenceContext from astroid.exceptions import ( AstroidBuildingError, @@ -282,19 +282,12 @@ def test_as_string_unknown() -> None: @staticmethod def test_recursion_error_trapped() -> None: - original_limit = sys.getrecursionlimit() - if IS_PYPY: - sys.setrecursionlimit(500) + with pytest.warns(UserWarning, match="unable to transform"): + ast = abuilder.string_build(LONG_CHAINED_METHOD_CALL) - try: - with pytest.warns(UserWarning, match="unable to transform"): - ast = abuilder.string_build(LONG_CHAINED_METHOD_CALL) - - with pytest.raises(UserWarning): - attribute = ast.body[1].value.func - assert attribute.as_string() - finally: - sys.setrecursionlimit(original_limit) + attribute = ast.body[1].value.func + with pytest.raises(UserWarning): + attribute.as_string() @pytest.mark.skipif(not PY312_PLUS, reason="Uses 3.12 type param nodes") From 47c912c389f8ec25d25f5b4a691f09e73b22e196 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Fri, 23 Feb 2024 08:18:28 -0500 Subject: [PATCH 12/13] Skip test on pypy --- tests/test_nodes.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/test_nodes.py b/tests/test_nodes.py index a7b4d79ec4..3bdb9f8f5f 100644 --- a/tests/test_nodes.py +++ b/tests/test_nodes.py @@ -29,7 +29,7 @@ transforms, util, ) -from astroid.const import PY310_PLUS, PY312_PLUS, Context +from astroid.const import IS_PYPY, PY310_PLUS, PY312_PLUS, Context from astroid.context import InferenceContext from astroid.exceptions import ( AstroidBuildingError, @@ -282,6 +282,11 @@ def test_as_string_unknown() -> None: @staticmethod def test_recursion_error_trapped() -> None: + if IS_PYPY: + pytest.skip( + "Test requires manipulating the recursion limit, which cannot " + "be undone in a finally block without polluting other tests on PyPy." + ) with pytest.warns(UserWarning, match="unable to transform"): ast = abuilder.string_build(LONG_CHAINED_METHOD_CALL) From 79e046a1f9115e52910054e4d2eba21bb9797162 Mon Sep 17 00:00:00 2001 From: Marc Mueller <30130371+cdce8p@users.noreply.github.com> Date: Fri, 23 Feb 2024 15:57:50 +0100 Subject: [PATCH 13/13] Use pytest.mark.skipif decorator --- tests/test_nodes.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/test_nodes.py b/tests/test_nodes.py index 3bdb9f8f5f..c5605a9328 100644 --- a/tests/test_nodes.py +++ b/tests/test_nodes.py @@ -281,12 +281,12 @@ def test_as_string_unknown() -> None: assert nodes.Unknown(lineno=1, col_offset=0).as_string() == "Unknown.Unknown()" @staticmethod + @pytest.mark.skipif( + IS_PYPY, + reason="Test requires manipulating the recursion limit, which cannot " + "be undone in a finally block without polluting other tests on PyPy.", + ) def test_recursion_error_trapped() -> None: - if IS_PYPY: - pytest.skip( - "Test requires manipulating the recursion limit, which cannot " - "be undone in a finally block without polluting other tests on PyPy." - ) with pytest.warns(UserWarning, match="unable to transform"): ast = abuilder.string_build(LONG_CHAINED_METHOD_CALL)