Log RecursionErrors out as warnings during node transformation

[jacobtylerwalls]

Type of Changes

	Type
✓	🐛 Bug fix

Description

Closes pylint-dev/pylint#8842

This only addresses recursion errors during node transformation. We should do something similar for the inference system, but suggesting to do that in pylint, not here.

[codecov]

Codecov Report

Attention: Patch coverage is 91.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 92.75%. Comparing base (07419f0) to head (79e046a).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2385      +/-   ##
==========================================
- Coverage   92.75%   92.75%   -0.01%     
==========================================
  Files          94       94              
  Lines       11070    11080      +10     
==========================================
+ Hits        10268    10277       +9     
- Misses        802      803       +1     

Flag	Coverage Δ
linux	92.56% <91.66%> (-0.01%)	⬇️
pypy	90.75% <50.00%> (-0.13%)	⬇️
windows	92.34% <91.66%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
astroid/transforms.py	100.00% <100.00%> (ø)
astroid/nodes/as_string.py	96.80% <83.33%> (-0.23%)	⬇️

[DanielNoord]

Nice idea! One optional comment

[Pierre-Sassoulas]

LGTM, maybe there's a middle ground between 1000 and 10 for the tests ? 😄 I'll let you decide

[jacobtylerwalls]

LGTM, maybe there's a middle ground between 1000 and 10 for the tests ? 😄 I'll let you decide

It has to be pretty high so that it doesn't accidentally cause other RecursionErrors we're not interested in testing. 1000 is the default, but lower than the pytest default of 2000, so that's why I had to do it at all.

[Pierre-Sassoulas]

The warning level make so much sense that I wonder why we didn't do that sooner. 🎉

[cdce8p]

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.

[jacobtylerwalls]

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.

[jacobtylerwalls]

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):

[jacobtylerwalls]

I'll try logging a warning and catching that in the test

[cdce8p]

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

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

Just as POC, try:

f"({node.expr})"

[jacobtylerwalls]

I wondered about that, but pylint lints the file just fine.

[jacobtylerwalls]

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')...

[cdce8p]

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.

[jacobtylerwalls]

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?

[cdce8p]

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).

[cdce8p]

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).

[jacobtylerwalls]

Thanks for the reviews!