| msg79548 - (view) |
Author: Armin Ronacher (aronacher) *  |
Date: 2009-01-10 15:45 |
| ast.literal_eval does not properly handle complex numbers: >>> ast.literal_eval("1j") 1j >>> ast.literal_eval("2+1j") Traceback (most recent call last): ... ValueError: malformed string >>> ast.literal_eval("(2+1j)") Traceback (most recent call last): ... ValueError: malformed string Expected result: >>> ast.literal_eval("1j") 1j >>> ast.literal_eval("2+1j") (2+1j) >>> ast.literal_eval("(2+1j)") (2+1j) I attached a patch that fixes this problem. |
|
|
| msg79550 - (view) |
Author: Armin Ronacher (aronacher) * |
Date: 2009-01-10 15:47 |
| fixed patch :) |
|
|
| msg79554 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2009-01-10 16:57 |
| Looks good to me assuming you add a test. |
|
|
| msg79604 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2009-01-11 17:33 |
| I'm not sure that this is desirable behaviour. There's no such thing as a complex literal---only imaginary literals. Why allow evaluation of 2+1j but not of 2 + 1, or 2*1j. In any case, I'm not sure that the patch behaves as intended. For example, >>> ast.literal_eval('2 + (3 + 4j)') (5+4j) |
|
|
| msg79701 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2009-01-12 20:42 |
| BTW, both those "I'm not sure"s should be taken literally: I'm not a user of the ast module, I don't know who the typical users are, and I don't know what the typical uses for the literal_eval function are. The patch just struck me as odd, so I thought I'd comment. IOW, take my comments with a large pinch of salt. |
|
|
| msg79705 - (view) |
Author: Armin Ronacher (aronacher) * |
Date: 2009-01-12 22:15 |
| Here a patch with unittests to correctly handle complex numbers. This does not allow the user of arbitrary add/sub expressions on complex numbers. |
|
|
| msg79723 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2009-01-13 08:14 |
| Nit: the "except" should only catch ValueError. |
|
|
| msg79730 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2009-01-13 10:47 |
| Nice fix! Exactly which complex strings should be accepted here? The patched version of literal-eval still accepts some things that would be rejected as inputs to complex(): >>> ast.literal_eval('-1+-3j') (-1-3j) >>> complex('-1+-3j') Traceback (most recent call last): File "", line 1, in ValueError: complex() arg is a malformed string and it produces different results from the complex constructor in some other cases: >>> complex('-0.0+0.0j').real -0.0 >>> ast.literal_eval('-0.0+0.0j').real 0.0 But since I don't really know what ast_literal is used for, I don't know whether this matters. It still seems odd to me to be doing just one special case of expression evaluation in ast_literal, but maybe that's just me. |
|
|
| msg79733 - (view) |
Author: Armin Ronacher (aronacher) * |
Date: 2009-01-13 11:34 |
| literal_eval has eval() semantics and not complex() constructor semantics. It accepts what eval() accepts just without arithmetic and unsafe features. For exmaple "(2 + 4j)" is perfectly fine even though the complex call only supports "2+4j" (no parentheses and whitespace). I commit the fix with the ValueError except Georg suggested. |
|
|
| msg79735 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2009-01-13 11:40 |
| So why accept (4+2j) but not (2j+4)? (BTW, I'm fairly sure that the complex constructor does accept parentheses; you're right about the whitespace, though.) |
|
|
| msg79736 - (view) |
Author: Armin Ronacher (aronacher) * |
Date: 2009-01-13 11:45 |
| Indeed, it accepts parentheses in 2.6 now, but not in 2.5 or earlier. Why not the other way round? Somewhere there has to be a limit. And if you write down complex numbers you usually have the imaginary part after the real part. But let's try no to make this a bikeshed discussion. If you say that literal_eval can safely evaluate the repr() of builtins (with the notable exception of reprs that eval can't evaluate either [like nan, inf etc.]) and probably a bit more it should be fine :) |
|
|
| msg79737 - (view) |
Author: Armin Ronacher (aronacher) * |
Date: 2009-01-13 11:52 |
| Fixed in rev68571. |
|
|
| msg79738 - (view) |
Author: Guilherme Polo (gpolo) * |
Date: 2009-01-13 12:30 |
| Why didn't you use assertRaises in place of that try/except for a test ? I was somewhat following this issue and just saw it being commited, but the change was being discussed. Aren't you supposed to commit these kind of changes only after entering in agreement with others ? |
|
|
| msg79739 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2009-01-13 12:38 |
| > If you say that > literal_eval can safely evaluate the repr() of builtins Sorry, yes, that makes perfect sense. (And now I see that that's what distinguishes 4+2j from 2j+4---finally the light dawns.) Apologies for being obtuse. |
|
|
| msg79740 - (view) |
Author: Armin Ronacher (aronacher) * |
Date: 2009-01-13 13:03 |
| > Why didn't you use assertRaises in place of that try/except for a test ? Could be changed. > I was somewhat following this issue and just saw it being commited, > but the change was being discussed. Aren't you supposed to commit > these kind of changes only after entering in agreement with others ? The "needs review" keyowrd was removed, I was under the impression I can commit now :) |
|
|
| msg80004 - (view) |
Author: Terry J. Reedy (terry.reedy) * |
Date: 2009-01-17 07:33 |
| I assume from the discussion that the patch was accepted/committed and changed the resolution and stage field to match. FWIW, list displays, for instance, are not literals either but are successfully evaluated, so doing same for complex 'displays' seems sensible to me too, and in line with the purpose of the method. |
|
|
| msg118154 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2010-10-08 00:52 |
| Fixed handling on unary minus in r85314. In so doing, it also liberalized what literal_eval() accepts (3j+4 for example). This simplified the implementation and removed an unnecessary restriction which wasn't needed for "safety". |
|
|