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

Jump to bottom

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 all 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
11 changes: 10 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 @@ -6,6 +6,7 @@

from __future__ import annotations

import warnings
from collections.abc import Iterator
from typing import TYPE_CHECKING

Expand Down Expand Up @@ -363,7 +364,15 @@

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:
warnings.warn(
"Recursion limit exhausted; defaulting to adding parentheses.",
UserWarning,
stacklevel=2,
)
left = f"({node.expr.accept(self)})"

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

View check run for this annotation

Codecov / codecov/patch

astroid/nodes/as_string.py#L375 

Added line #L375 was not covered by tests
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
17 changes: 16 additions & 1 deletion tests/test_nodes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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

Expand Down Expand Up @@ -279,6 +280,20 @@ 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
@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:
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()
Comment on lines +294 to +295
Copy link
Member

Choose a reason for hiding this comment

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

Just know that this is more a workaround. pytest would still hang if the method didn't raise a UserWarning. I believe internally pytest sets the warning to error to be able to catch it here, but that also means the fallback isn't executed.

Copy link
Member Author

Choose a reason for hiding this comment

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

absolutely, but it seems pytest is doing something magical if the using same recursion limit is not a problem in the interactive interpreter but is a problem inside pytest?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, pytest and warnings capture is just magic sometimes 😅
Since the test pass an it works in pylint I would be fine with merging this as is (even with the one uncovered line).



@pytest.mark.skipif(not PY312_PLUS, reason="Uses 3.12 type param nodes")
class AsStringTypeParamNodes(unittest.TestCase):
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