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.

Sign up for GitHub

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

Log RecursionErrors out as warnings during node transformation #2385

Merged
merged 14 commits into from
Feb 23, 2024
Merged

Log RecursionErrors out as warnings during node transformation #2385

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f6b8876
Log RecursionErrors out as warnings during node transformation
jacobtylerwalls Feb 18, 2024
91a5e4a
Trap RecursionError in `visit_attribute`
jacobtylerwalls Feb 18, 2024
72b888a
Adjust test for PyPy?
jacobtylerwalls Feb 18, 2024
491833b
Adjust PyPy recursion limit in test
jacobtylerwalls Feb 18, 2024
4723ed5
Simplify pypy workaround; assert msg
jacobtylerwalls Feb 19, 2024
a27f827
Try lowering pypy recursion limit?
jacobtylerwalls Feb 19, 2024
1f4fc79
Last try
jacobtylerwalls Feb 19, 2024
5cf97a0
Merge branch 'main' into trap-recursion
jacobtylerwalls Feb 19, 2024
b2ff23d
Use pytest.warns
jacobtylerwalls Feb 23, 2024
6920c61
Add test and additional warning
jacobtylerwalls Feb 23, 2024
8e404a9
Adjust for pypy again
jacobtylerwalls Feb 23, 2024
63f54b8
Revert "Adjust for pypy again"
jacobtylerwalls Feb 23, 2024
47c912c
Skip test on pypy
jacobtylerwalls Feb 23, 2024
79e046a
Use pytest.mark.skipif decorator
cdce8p Feb 23, 2024
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: 4 additions & 1 deletion ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,15 @@ 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

* Suppress ``SyntaxWarning`` for invalid escape sequences on Python 3.12 when parsing modules.

Closes pylint-dev/pylint#9322



What's New in astroid 3.0.3?
============================
Release date: 2024-02-04
Expand Down
5 changes: 4 additions & 1 deletion astroid/nodes/as_string.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,10 @@

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)})"

Check warning on line 369 in astroid/nodes/as_string.py

View check run for this annotation

Codecov / codecov/patch

astroid/nodes/as_string.py#L368-L369 

Added lines #L368 - L369 were not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

Seems like these lines aren't covered by the test case. Is it actually necessary to catch RecursionError here? Doing so in _visit_generic seems to be enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries, I'll add the test coverage. It's not related to node transformation, but I wanted this file to be lintable without further patches needed in astroid so we could close the ticket.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I tried, but the test crashes pytest. Any ideas?

diff --git a/tests/test_nodes.py b/tests/test_nodes.py
index 6ea25fd8..713591e4 100644
--- a/tests/test_nodes.py
+++ b/tests/test_nodes.py
@@ -29,7 +29,7 @@ from astroid import (
     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,
@@ -49,6 +49,7 @@ from astroid.nodes.node_classes import (
 from astroid.nodes.scoped_nodes import ClassDef, FunctionDef, GeneratorExp, Module
 
 from . import resources
+from tests.testdata.python3.recursion_error import LONG_CHAINED_METHOD_CALL
 
 abuilder = builder.AstroidBuilder()
 
@@ -279,6 +280,21 @@ everything = f""" " \' \r \t \\ {{ }} {'x' + x!r:a} {["'"]!s:{a}}"""
         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:
+        original_limit = sys.getrecursionlimit()
+        sys.setrecursionlimit(500 if IS_PYPY else 1000)
+        
+        try:
+            with pytest.warns(UserWarning, match="unable to transform"):
+                ast = abuilder.string_build(LONG_CHAINED_METHOD_CALL)
+
+            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")
 class AsStringTypeParamNodes(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'll try logging a warning and catching that in the test

Copy link
Member

Choose a reason for hiding this comment

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

I guess that the except clause itself also triggers a new RecursionError.

f"({node.expr.accept(self)})"

Just as POC, try:

f"({node.expr})"

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 wondered about that, but pylint lints the file just fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

With your change, the output begins:

(Call(func=<Attribute.add l.5 at 0x1075eaf30>,
args=[<Const.str l.10 at 0x1075eb680>],
keywords=[<Keyword l.10 at 0x1075eb6e0>])).add('name', value='value').add('name', value='value')...

instead of

b.builder('name').add('name', value='value').add('name', value='value')...

if left.isdigit():
left = f"({left})"
return f"{left}.{node.attrname}"
Expand Down
14 changes: 13 additions & 1 deletion astroid/transforms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
24 changes: 24 additions & 0 deletions tests/test_transforms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,20 @@
from __future__ import annotations

import contextlib
import sys
import time
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
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
Expand Down Expand Up @@ -258,3 +264,21 @@ 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(500 if IS_PYPY else 1000)

try:
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)
170 changes: 170 additions & 0 deletions tests/testdata/python3/recursion_error.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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()
)"""
Loading