bpo-30529: Fix assert failure in with pydebug; add tests. #2232
Conversation
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Just few nitpicks.
And needed a Misc/NEWS entry.
Python/ast.c
Outdated
| NULL). */ | ||
| } else if (!state->last_str) { | ||
| /* Note that the literal can be zero length, if the | ||
| Input string is "\\\n" or "\\\r". */ |
There was a problem hiding this comment.
Or "\\\r\n".
The word "Input" shouldn't start from a capital letter.
Lib/test/test_fstring.py
Outdated
| # As part of bpo-30529, this used to raise an assert in pydebug mode. | ||
| for i in range(1, 32): | ||
| with self.subTest(i=i): | ||
| # This will result in a backslash followed by the control |
There was a problem hiding this comment.
A backslash followed by the control character (except '\r' and '\n') produces a DeprecationWarning.
I would test just a case of '\\\n'. Maybe also '\\\r' and '\\\r\n' if you prefer.
There was a problem hiding this comment.
Okay. I think I'll just test '\\\r' and '\\\n'.
Lib/test/test_fstring.py
Outdated
| # string. The same thing happens with non-fstrings. | ||
| expected_len = 0 if i in (10, 13) else 2 | ||
| s = f'f"\\{chr(i)}"' | ||
| self.assertEqual(len(eval(s)), expected_len) |
There was a problem hiding this comment.
You can test not just length, but a value. This could produce more informative error message if the eval() returns unexpected result.
There was a problem hiding this comment.
I'll just test the zero length versions.
|
Since 3.6.0 is affected, bpo-30529 is not related. |
ericvsmith
left a comment
ericvsmith
left a comment
There was a problem hiding this comment.
Agreed on this not being related to bpo-30529. I'll create a new issue.
Lib/test/test_fstring.py
Outdated
| # As part of bpo-30529, this used to raise an assert in pydebug mode. | ||
| for i in range(1, 32): | ||
| with self.subTest(i=i): | ||
| # This will result in a backslash followed by the control |
There was a problem hiding this comment.
Okay. I think I'll just test '\\\r' and '\\\n'.
Lib/test/test_fstring.py
Outdated
| # string. The same thing happens with non-fstrings. | ||
| expected_len = 0 if i in (10, 13) else 2 | ||
| s = f'f"\\{chr(i)}"' | ||
| self.assertEqual(len(eval(s)), expected_len) |
There was a problem hiding this comment.
I'll just test the zero length versions.
Python/ast.c
Outdated
| NULL). */ | ||
| } else if (!state->last_str) { | ||
| /* Note that the literal can be zero length, if the | ||
| Input string is "\\\n" or "\\\r". */ |
|
I've created bpo-30682 and updated my commit to reflect Serhiy's comments. |
…in f-strings. (pythonGH-2232) This caused a segfault on eval("f'\\\n'") and eval("f'\\\r'") in debug build.. (cherry picked from commit 11e97f2)
3 participants
This removes an assert that fails, and adds tests.