Repository navigation
ast_for_factor unary minus optimization changes AST #53257
Description
| BPO | 9011 |
|---|---|
| Nosy | @nascheme, @amauryfa, @mdickinson, @benjaminp, @bitdancer, @meadori, @antocuni |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/mdickinson'
closed_at = <Date 2012-11-25.17:12:08.940>
created_at = <Date 2010-06-16.15:38:58.089>
labels = ['interpreter-core', 'type-bug']
title = 'ast_for_factor unary minus optimization changes AST'
updated_at = <Date 2012-11-25.17:12:08.940>
user = 'https://bugs.python.org/alexhsamuel'bugs.python.org fields:
activity = <Date 2012-11-25.17:12:08.940>
actor = 'mark.dickinson'
assignee = 'mark.dickinson'
closed = True
closed_date = <Date 2012-11-25.17:12:08.940>
closer = 'mark.dickinson'
components = ['Interpreter Core']
creation = <Date 2010-06-16.15:38:58.089>
creator = 'alexhsamuel'
dependencies = []
files = ['17696', '28118']
hgrepos = []
issue_num = 9011
keywords = ['patch']
message_count = 24.0
messages = ['107928', '107998', '108001', '108002', '108007', '108011', '108014', '108237', '108969', '108970', '108971', '108973', '119089', '124180', '124206', '124209', '173258', '173259', '173260', '173261', '173262', '173263', '176368', '176375']
nosy_count = 9.0
nosy_names = ['nascheme', 'amaury.forgeotdarc', 'mark.dickinson', 'benjamin.peterson', 'alexhsamuel', 'r.david.murray', 'meador.inge', 'python-dev', 'antocuni']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'needs patch'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue9011'
versions = ['Python 2.7']
Activity
alexhsamuel commentedon Jun 16, 2010
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:
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:
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.
mdickinson commentedon Jun 17, 2010
Confirmed in trunk and py3k, both of which raise ValueError on the second compile:
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 bpo-1441486.
mdickinson commentedon Jun 17, 2010
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):
And with the optimization removed:
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 bpo-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?
mdickinson commentedon Jun 17, 2010
Results from Jython:
On the other hand, IronPython appears to detect this special case and produces an int instead of a long.
mdickinson commentedon Jun 17, 2010
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?
alexhsamuel commentedon Jun 17, 2010
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:
mdickinson commentedon Jun 17, 2010
That sounds like a reasonable quick fix.
Here's a patch.
mdickinson commentedon Jun 20, 2010
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.
mdickinson commentedon Jun 30, 2010
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:
But in current release31-maint branch, and in 3.2a0:
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!
mdickinson commentedon Jun 30, 2010
r82044 reverted in r82389.
mdickinson commentedon Jun 30, 2010
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.
mdickinson commentedon Jun 30, 2010
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.)
1 remaining item
bitdancer commentedon Dec 17, 2010
Mark, are you still planning to do something for 3.1/2.7, or should this be closed?
mdickinson commentedon Dec 17, 2010
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?
bitdancer commentedon Dec 17, 2010
Given the long projected lifetime of 2.7, I suppose it is.
antocuni commentedon Oct 18, 2012
there is still an inconsistency in handling negative imaginary literals:
mdickinson commentedon Oct 18, 2012
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.
antocuni commentedon Oct 18, 2012
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.
mdickinson commentedon Oct 18, 2012
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.
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.
amauryfa commentedon Oct 18, 2012
Indeed, -1j is not a literal:
mdickinson commentedon Oct 18, 2012
And -1j.real is -(1j.real), of course, not (-1j).real. My bad.
mdickinson commentedon Nov 25, 2012
New patch.
python-dev commentedon Nov 25, 2012
New changeset ea36de7926f8 by Mark Dickinson in branch '2.7':
Issue bpo-9011: AST creation no longer modifies CST for negated numeric literals.
http://hg.python.org/cpython/rev/ea36de7926f8