The unary negative optimization in ast_for_factor() seems to modify the ST in a way changes its meaning.

In Python 3.1.2, the ST is no longer compilable:

$ cat exprbug.py
import parser

st = parser.expr("-3")
print(st.totuple())
compiled = st.compile()
print(eval(compiled))
print(st.totuple())
print(eval(st.compile()))

$ ~/sw/Python-3.1.2/bin/python3 exprbug.py
(258, (326, (301, (305, (306, (307, (308, (310, (311, (312, (313, (314, (315, (316, (317, (15, '-'), (317, (318, (319, (2, '3')))))))))))))))))), (4, ''), (0, ''))
-3
(258, (326, (301, (305, (306, (307, (308, (310, (311, (312, (313, (314, (315, (316, (317, (15, '-'), (317, (318, (319, (2, '-3')))))))))))))))))), (4, ''), (0, ''))
Traceback (most recent call last):
  File "exprbug.py", line 8, in <module>
    print(eval(st.compile()))
ValueError: could not convert string to float: --3


In earlier versions of Python (I have confirmed 2.5 and 2.6), it is compiled to incorrect code and produces wrong results when evaluated:

$ ~/sw/Python-2.6.2/bin/python exprbug.py
(258, (327, (304, (305, (306, (307, (308, (310, (311, (312, (313, (314, (315, (316, (15, '-'), (316, (317, (318, (2, '3'))))))))))))))))), (4, ''), (0, ''))
-3
(258, (327, (304, (305, (306, (307, (308, (310, (311, (312, (313, (314, (315, (316, (15, '-'), (316, (317, (318, (2, '-3'))))))))))))))))), (4, ''), (0, ''))
-1.0


If I remove the big if statement from the front of ast_to_factor(), the code behaves correctly.  I think this is because STR(pnum) is changed in place and never restored to its previous value.

Confirmed in trunk and py3k, both of which raise ValueError on the second compile:

Python 2.7rc1+ (trunk:82042, Jun 17 2010, 09:52:12) 
[GCC 4.0.1 (Apple Inc. build 5490)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import parser
>>> st = parser.expr("-3")
>>> st.compile()
<code object <module> at 0x450448, file "<syntax-tree>", line 1>
>>> st.compile()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: could not convert string to float: --3

This looks nasty; unless there's some rule that says that you're not supposed to hang on to AST objects after compiling them.  (Is there?)

Benjamin, would it be worth just getting rid of this optimization for 2.7, and trying to reinstate a correct version for 2.7.1?

[Removing 2.5 from the versions list since there's nothing we can do about it there (2.5 is only getting security fixes at this point).]

See also issue 1441486.

N.B.  That if block isn't pure optimization:  removing it gives a minor change in behaviour:

Currently (Python 2.7, on a 64-bit machine):

>>> -9223372036854775808
-9223372036854775808

And with the optimization removed:

>>> -9223372036854775808
-9223372036854775808L

I actually consider the second behaviour more correct than the first, since it follows clearly from the language rules (numeric literals have no sign, so the above *should* be interpreted as the unary minus operator applied to a literal, and that literal really is a PyLong).  But obviously the contributors to issue 1441486 either disagree, or didn't want to introduce a regression from 2.4.

I still consider that removing that if block is the right thing to do for 2.7.  The change in behaviour really shouldn't affect any reasonable code---anywhere that an int is acceptable, a long should be too.



Neil, any comments?

Results from Jython:

newton:jython2.5.1 dickinsm$ ./jython
Jython 2.5.1 (Release_2_5_1:6813, Sep 26 2009, 13:47:54) 
[Java HotSpot(TM) 64-Bit Server VM (Apple Inc.)] on java1.6.0_20
Type "help", "copyright", "credits" or "license" for more information.
>>> -2147483648
-2147483648L

On the other hand, IronPython appears to detect this special case and produces an int instead of a long.

Fixed in r82043 (py3k) and r82044 (release31-maint), simply by removing the relevant 'if' block.

For 2.x, Antoine (on IRC) pointed out that there might well be third party code that depends on -0x80000000 being an int rather than a long (32-bit machine), so changing that behaviour could cause breakage.

Can anyone propose a fix that doesn't change behaviour?

How about saving the original value of STR(pnum) and restoring it after 
calling ast_for_atom()?

This is not thread-safe, but I don't understand Python's threading model 
well enough to know whether the GIL is held in this function.

On 6/17/2010 8:40 AM, Mark Dickinson wrote:
>
> Mark Dickinson<dickinsm@gmail.com>  added the comment:
>
> Fixed in r82043 (py3k) and r82044 (release31-maint), simply by removing the relevant 'if' block.
>
> For 2.x, Antoine (on IRC) pointed out that there might well be third party code that depends on -0x80000000 being an int rather than a long (32-bit machine), so changing that behaviour could cause breakage.
>
> Can anyone propose a fix that doesn't change behaviour?
>
> ----------
>
> _______________________________________
> Python tracker<report@bugs.python.org>
> <http://bugs.python.org/issue9011>
> _______________________________________

That sounds like a reasonable quick fix.

Here's a patch.

Summary of brief discussion on #python-dev:

- This is a hacky patch; it would be better to rewrite that portion of ast.c in a way that doesn't modify the original CST at all. With 2.7 so close, I daren't try anything more invasive than this patch, though. (I don't know whether there are any other portions of this file that deliberately change the CST.)

- Given that no-one noticed this problem in 5 years, the fix can probably wait until 2.7.1, when it can be done properly.

It turns out that removing this code from py3k wasn't a no-op.  It changed the semantics for complex literals.  In Python 3.1.2:

>>> -7j
-7j
>>> (-7j).real
0.0

But in current release31-maint branch, and in 3.2a0:

Python 3.2a0 (py3k:82347M, Jun 28 2010, 22:25:11) 
[GCC 4.2.1 (Apple Inc. build 5659)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> -7j
(-0-7j)
>>> (-7j).real
-0.0

So IMO r82044 (the release31-maint branch) should be reverted, to avoid nasty surprises when people upgrade from 3.1.2 to 3.1.3.

But I'd like to keep this change in py3k:  I consider that the 3.1 behaviour is buggy, and that the 3.2 behaviour is more correct (strange though it may seem at first sight), so I'd prefer to leave the current py3k behaviour as is, and add a Misc/NEWS entry describing the change if necessary.

Assigning this to me because I don't want it to get forgotten.  But if anyone else is interested in producing a patch for the cleanup of the CST->AST code, please do!

r82044 reverted in r82389.

Added tests for the current Python < 3.2 treatment of imaginary literals in r82390 (release31-maint).  These tests should be backported to trunk, but I'll wait until after the release of 2.7 for that.

Added tests for Python 3.2's corrected treatment of negated imaginary literals in r82391, along with a Misc/NEWS entry indicating the changed behaviour.

Restored version numbers for this issue, which I seem to have accidentally deleted earlier.  (Including 3.1, since it could also benefit from the same cleanup.)

I'm late to the party but looks like Mark has a good handle on the issues.  I would recommend not changing behavior in a bugfix release.  I'm pretty sure code "in the wild" would be broken.

Mark, are you still planning to do something for 3.1/2.7, or should this be closed?

Yes, sorry;  I'm not likely to find time to do anything with this.  Unassigning, and downgrading priority.

Is it worth leaving this open in case anyone wants to do something about it?

Given the long projected lifetime of 2.7, I suppose it is.

there is still an inconsistency in handling negative imaginary literals:

>>> -1j.real
-0.0
>>> complex('-1j').real
0.0

No, that's expected behaviour.

1j is complex(0.0, 1.0)
-1j is complex(-0.0, -1.0)

so -1j.real is -0.0.  There's not really any other sensible way to handle this.

The complex-from-string constructor, on the other hand, is more careful about interpreting signs.

I would say that the complex-from-string constructor should be fixed to handle this special case "correctly".
I find very confusing that we get a different result whether we use a string literal or not.

For example, in pypy we use the same code for parsing literals and converting strings, so you get -0.0 in both cases.

With the string, the minus sign applies only to the imaginary part;  with the expression '-1j', it applies to the whole complex number (both real and imaginary parts).

I don't see any sensible way to 'fix' the string to complex conversion; indeed, I think any change would make it worse than before.  It's a known issue with complex arithmetic that x + 1j*y doesn't give you complex(x, y);  the conversions from string and the complex(x, y) form are there to make it possible to carefully create a complex number with known real and imaginary parts.


> For example, in pypy we use the same code for parsing literals and
> converting strings, so you get -0.0 in both cases.

But -1j isn't a literal.  It's unary minus applied to a the complex number given by the literal '1j'.  Python's code *does* give the same results both for converting strings and parsing literals.

Indeed, -1j is not a literal:

>>> dis.dis(lambda :-1j.real)
  1           0 LOAD_CONST               0 (1j)
              3 LOAD_ATTR                0 (real)
              6 UNARY_NEGATIVE      
              7 RETURN_VALUE

And -1j.real is -(1j.real), of course, not (-1j).real.  My bad.

New patch.

New changeset ea36de7926f8 by Mark Dickinson in branch '2.7':
Issue #9011: AST creation no longer modifies CST for negated numeric literals.
http://hg.python.org/cpython/rev/ea36de7926f8