bpo-32888: Improve exception message in ast.literal_eval #340

Rosuav · 2017-02-27T14:17:57Z

When a non-literal is given to literal_eval, attempt to be more
helpful with the message, rather than calling it 'malformed'.

https://bugs.python.org/issue32888

…#340, python#344, python#348, python#349, python#350)

…#340, python#344, python#348, python#349, python#350) (3.5->3.6)

…#340, python#344, python#348, python#349, python#350) (3.6->3.7)

merwok · 2018-02-18T23:45:36Z

This looks good, do you mind opening a bugs.python.org ticket and adding a news entry?

When a non-literal is given to literal_eval, attempt to be more helpful with the message, rather than calling it 'malformed'.

Rosuav · 2018-02-20T17:42:45Z

Rebased onto current master, which includes shifting where the actual change happens. NEWS entry added, issue number added.

serhiy-storchaka

Please add a special test in test_ast.

serhiy-storchaka · 2018-02-20T17:50:37Z

Lib/ast.py

@@ -52,6 +52,8 @@ def _convert_num(node):
                return node.value
        elif isinstance(node, Num):
            return node.n
+        elif isinstance(node, AST):
+            raise ValueError('%s not allowed in literal' % type(node).__name__)
        raise ValueError('malformed node or string: ' + repr(node))


In what cases this exception is raised? Is it covered by tests?

The easiest way to trigger it is to use something that isn't permitted in literal_eval, but is valid syntax. There's already one such test - proving that f-strings aren't literal_evallable - and I guess there could be more, but since this is a whitelist, the error is basically "anything else, raise". The only difference in this patch is that the message is different for "syntactically valid but not acceptable by literal_eval" and "syntactically invalid".

I meant what input triggers the "malformed node or string" error now?

Calling literal_eval with an AST node that has non-AST children in it. I don't think it's possible (post-patch) to trigger this error by calling literal_eval with a string, but I can't say for sure, so I don't want to say just "malformed node".

serhiy-storchaka · 2018-02-20T17:51:26Z

Misc/NEWS.d/next/Library/2018-02-21-04-39-49.bpo-32888.G5i41Q.rst

@@ -0,0 +1,2 @@
+Improve wording of error messages from ast.literal_eval, distinguishing
+valid-but-unacceptable nodes from those that are actually malformed.


Please add "Patch by..."

Should that be done by amending the current commit, or adding an additional commit?

It is better for review to add a new commit. In any case all commits are squashed before merging into the repository.

briancurtin

Overall this looks good, just one suggestion on the test implementation.

briancurtin · 2018-07-03T16:23:54Z

Lib/test/test_ast.py

+            ast.BinOp(ast.Num(1), ast.Add(), 'oops'): "malformed node or string: 'oops'",
+        }
+        for test, message in tests.items():
+            with self.assertRaises(ValueError) as cm:


A helpful thing to use in between lines 594 and 595 would be with self.subTest(test=test, message=message): and then indent the parts below it. That way each of these end up being tested independently rather than potentially having the items loop have a case fail and cause the rest of the cases to fail to execute.

Check out https://docs.python.org/3/library/unittest.html#distinguishing-test-iterations-using-subtests

bedevere-bot · 2018-07-03T16:24:55Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

csabella · 2019-05-13T03:58:58Z

@Rosuav, please address the last change request by @briancurtin and also rebase to fix the conflict. Thanks!

Rosuav · 2019-05-13T04:15:40Z

BPO cites another couple of issues, one of which is still open, as dependencies. And it's not really a high priority - it's nothing more than a nice-to-have. There are a lot of barriers to getting patches accepted and it's often not worth the effort unless you REALLY care about the patch.

merwok · 2019-05-13T15:45:16Z

(Note that you can rebase or do a regular merge, which is often easier! All PRs are squash merged in the end @csabella)

merwok · 2019-05-13T15:49:44Z

@serhiy-storchaka Should this wait for bpo-32893 per your last message on the ticket or could it be merged soon? (when conflicts are resolved + suggestion from Brian)

vstinner · 2019-12-14T15:01:47Z

A different approach using ast.dump(): PR #16620.

vstinner · 2019-12-14T15:03:53Z

What's the status of this PR?

@Rosuav: Are you still working on this?

A different approach using ast.dump(): PR #16620.

Oh, I just closed it since it could produce very long error message: https://bugs.python.org/issue38396#msg354132

Rosuav · 2019-12-14T16:10:22Z

No, I'm not. It was a nice-to-have but between the pushback (no core dev seemed to want it to happen) and the constant need to fix merge conflicts meant that I abandoned this proposal ages ago. If someone else wants to champion this fix, then sure, but I don't have the energy to push everything, and this one wasn't worth it.

vstinner · 2019-12-15T22:11:47Z

@isidentical: Are you interested to take over this change? I'm not the good candidate to review it. Maybe @pablogsal or @serhiy-storchaka.

isidentical · 2019-12-19T14:17:15Z

Sure.

csabella · 2020-01-12T13:12:33Z

Closing this pull request as it is now GH-17662.

Bumps [celery](https://github.com/celery/celery) from 4.4.3 to 4.4.4. - [Release notes](https://github.com/celery/celery/releases) - [Changelog](https://github.com/celery/celery/blob/master/Changelog.rst) - [Commits](celery/celery@4.4.3...v4.4.4) Signed-off-by: dependabot-preview[bot] <[email protected]> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

the-knights-who-say-ni added the CLA signed label Feb 27, 2017

akruis pushed a commit to stackless-dev/stackless that referenced this pull request Nov 1, 2017

Issue python#28556: merge 5 more typing changes from upstream (python…

83ec302

…#340, python#344, python#348, python#349, python#350)

akruis pushed a commit to stackless-dev/stackless that referenced this pull request Nov 1, 2017

Issue python#28556: merge 5 more typing changes from upstream (python…

043a8bc

…#340, python#344, python#348, python#349, python#350) (3.5->3.6)

akruis pushed a commit to stackless-dev/stackless that referenced this pull request Nov 1, 2017

Issue python#28556: merge 5 more typing changes from upstream (python…

52f2959

…#340, python#344, python#348, python#349, python#350) (3.6->3.7)

Rosuav changed the title ~~Improve exception message in ast.literal_eval~~ bpo-32888: Improve exception message in ast.literal_eval Feb 20, 2018

Improve exception message in ast.literal_eval

26ba98e

When a non-literal is given to literal_eval, attempt to be more helpful with the message, rather than calling it 'malformed'.

Rosuav force-pushed the literal_eval-exception branch from b9b6e9d to 26ba98e Compare February 20, 2018 17:41

serhiy-storchaka reviewed Feb 20, 2018

View reviewed changes

zware added the awaiting review label Feb 20, 2018

Rosuav added 3 commits February 21, 2018 05:10

Include patch author name in NEWS

08ffc03

Distinguish different causes of error

fb83598

Add test of different forms of exception message

cce0c6c

briancurtin requested changes Jul 3, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jul 3, 2018

isidentical mentioned this pull request Dec 14, 2019

bpo-38396: dump(node) instead of repr(node) on ast.literal_eval #16620

Closed

isidentical mentioned this pull request Dec 19, 2019

bpo-32888: enhance ast.literal_eval error messagess with context information #17662

Open

csabella closed this Jan 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-32888: Improve exception message in ast.literal_eval #340

bpo-32888: Improve exception message in ast.literal_eval #340

Rosuav commented Feb 27, 2017 •

edited by bedevere-bot

Loading

merwok commented Feb 18, 2018

Rosuav commented Feb 20, 2018

serhiy-storchaka left a comment

serhiy-storchaka Feb 20, 2018

Rosuav Feb 20, 2018

serhiy-storchaka Feb 20, 2018

Rosuav Feb 20, 2018

serhiy-storchaka Feb 20, 2018

Rosuav Feb 20, 2018

serhiy-storchaka Feb 20, 2018

Rosuav Feb 20, 2018

briancurtin left a comment

briancurtin Jul 3, 2018

bedevere-bot commented Jul 3, 2018

csabella commented May 13, 2019

Rosuav commented May 13, 2019

merwok commented May 13, 2019

merwok commented May 13, 2019 •

edited by bedevere-bot

Loading

vstinner commented Dec 14, 2019

vstinner commented Dec 14, 2019

Rosuav commented Dec 14, 2019

vstinner commented Dec 15, 2019

isidentical commented Dec 19, 2019

csabella commented Jan 12, 2020

		@@ -0,0 +1,2 @@
		Improve wording of error messages from ast.literal_eval, distinguishing
		valid-but-unacceptable nodes from those that are actually malformed.

bpo-32888: Improve exception message in ast.literal_eval #340

bpo-32888: Improve exception message in ast.literal_eval #340

Conversation

Rosuav commented Feb 27, 2017 • edited by bedevere-bot Loading

merwok commented Feb 18, 2018

Rosuav commented Feb 20, 2018

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briancurtin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jul 3, 2018

csabella commented May 13, 2019

Rosuav commented May 13, 2019

merwok commented May 13, 2019

merwok commented May 13, 2019 • edited by bedevere-bot Loading

vstinner commented Dec 14, 2019

vstinner commented Dec 14, 2019

Rosuav commented Dec 14, 2019

vstinner commented Dec 15, 2019

isidentical commented Dec 19, 2019

csabella commented Jan 12, 2020

Rosuav commented Feb 27, 2017 •

edited by bedevere-bot

Loading

merwok commented May 13, 2019 •

edited by bedevere-bot

Loading