classification
Title: compile() raises SystemError if called from except clause
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.2, Python 3.3
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, benjamin.peterson, brett.cannon, daniel.urban, georg.brandl, july, ncoghlan
Priority: normal Keywords: patch

Created on 2011-03-08 16:24 by july, last changed 2013-01-11 17:06 by brett.cannon. This issue is now closed.

Files
File name Uploaded Description Edit
issue11441.patch daniel.urban, 2011-03-08 21:07 review
issue11441_2.patch daniel.urban, 2011-03-08 22:25 New patch with test in test_compile review
Messages (16)
msg130342 - (view) Author: July Tikhonov (july) * Date: 2011-03-08 16:24
Normal:
>>> compile('1 = 1', '<string>', 'exec')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 1
SyntaxError: can't assign to literal

SystemError is raised instead of SyntaxError:
>>> try: abcde
... except NameError:
...  compile('1 = 1', '<string>', 'exec')
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'abcde' is not defined

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
SystemError: Objects/tupleobject.c:126: bad argument to internal function

Error can be discovered by calling dis.dis('1 = 1').
msg130343 - (view) Author: Daniel Urban (daniel.urban) * (Python triager) Date: 2011-03-08 17:31
Apparently ast_error_finish calls PyTuple_GetItem with a value that is not a tuple, but a SyntaxError instance (in Python/ast.c line 112).  It seems that ast_error_finish expects that PyErr_Fetch will return the exception value as a tuple, and in some cases this seems correct (for example when not in an except clause), but not in this case.  I don't know much about Python exception handling in C, but it seems to me, that it shouldn't expect always a tuple (see also http://docs.python.org/dev/py3k/c-api/exceptions.html#PyErr_NormalizeException).
msg130346 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-03-08 18:04
Right. In most cases, "PyErr_SetObject(PyExc_SyntaxError, tuple);" will store the untouched tuple in tstate->curexc_value, *except* when "Implicit exception chaining" occurs, in which case the exception is normalized.

ast_error_finish() should not expect a tuple in all cases, and should probably normalize the exception as well.
msg130374 - (view) Author: Daniel Urban (daniel.urban) * (Python triager) Date: 2011-03-08 21:07
Here is a patch. I wasn't sure, where to put the test, so I put it in test_ast.
msg130378 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2011-03-08 22:07
You can put the test in test_compile.
msg130380 - (view) Author: Daniel Urban (daniel.urban) * (Python triager) Date: 2011-03-08 22:25
Okay, here is a new patch with the test in the correct place (I hope).
msg130386 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-03-08 23:37
Why is the exception normalized at the end? I suppose it's because when value is an exception instance, it's replaced by a tuple, but the original value has to be recreated at the end. So in some cases, the SyntaxError object is created twice...

If PyErr_NormalizeException() can't be avoided, I suggest to call it at the start, just after PyErr_Fetch, and use the PySyntaxErrorObject* structure directly to get the file name and line numbers.
msg130431 - (view) Author: Daniel Urban (daniel.urban) * (Python triager) Date: 2011-03-09 07:34
> Why is the exception normalized at the end? I suppose it's because
> when value is an exception instance, it's replaced by a tuple, but the
> original value has to be recreated at the end. So in some cases, the
> SyntaxError object is created twice...
>
> If PyErr_NormalizeException() can't be avoided, I suggest to call it
> at the start, just after PyErr_Fetch, and use the PySyntaxErrorObject*
> structure directly to get the file name and line numbers.

Yeah, it is because ast_error_finish creates a new tuple to use as the exception value.  (It creates a new (errstr, (filename, lineno, offset, loc)) tuple from the original (errstr, lineno, offset) tuple).  And yes, in some cases the SyntaxError instance is created twice.  I wasn't sure if it's okay to simply replace the args field of a PyBaseExceptionObject.  I don't know, if PyErr_NormalizeException() can be avoided, you wrote, that it "should probably normalize the exception as well".

Would it be better, if we, when got an exception instance, create the new tuple from the info, and replace the args field of the instance with it?  (But it also seems to me, that the SyntaxError objects have other fields as well, so probably we should modify them also.  That's why I thought that calling PyErr_NormalizeException with the new tuple is the simplest thing to do, becuase I guess that'll take care of all fields automatically.)
msg130434 - (view) Author: July Tikhonov (july) * Date: 2011-03-09 09:22
There is an XXX just before the definition of ast_error. Wouldn't it be
useful?

The idea is to merge ast_error() and ast_error_finish().
This requires redefinition of most functions in ast.c, adding "const char
*filename" to their parameters.
msg130436 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-03-09 09:44
> That's why I thought that calling PyErr_NormalizeException with the new
> tuple is the simplest thing to do, becuase I guess that'll take care of
> all fields automatically.

You could also call PyErr_NormalizeException at the beginning, and update the fields directly in the PySyntaxErrorObject structure.  No need to deal with any tuple.
msg130453 - (view) Author: Daniel Urban (daniel.urban) * (Python triager) Date: 2011-03-09 12:50
> You could also call PyErr_NormalizeException at the beginning, and
> update the fields directly in the PySyntaxErrorObject structure.  No
> need to deal with any tuple.

Sorry, but I don't really understand.  If I call PyErr_NormalizeException at the beginning, the SyntaxError instance will be initialized with the wrong 3-tuple: (errstr, lineno, offset).  If after that I update the msg, filename, ... fields, they will be correct, but I think the args field still will be the wrong 3-tuple.  So I'll have to still create the new tuple, and replace the args field, right?
msg130454 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-03-09 12:54
hmm, you are right, of course. I forgot that e.args is part of the SyntaxError members.
msg130515 - (view) Author: Daniel Urban (daniel.urban) * (Python triager) Date: 2011-03-10 17:13
So, I see four possible solutions:

1. If we get a tuple, create the new tuple, normalize the exception, and store it.  If we get a SyntaxError instance, use its args, create the new tuple, normalize, and store.  (In this case a SyntaxError instance will be created twice.)

2. If we get a tuple, create the new tuple and store it without normalization.  If we get a SyntaxError instance use its args to create the new tuple and store it without normalization.  (I think, that later it's still possible that a new SynaxError will be created, but we don't create it here.)

3. If we get a tuple, create the new tuple, and store it without normalization.  If we get a SyntaxError, take its args, create the new tuple, and call SyntaxError.__init__ with it. I think this will set all fields properly.

4. Like 3., but if we got a tuple, store the new tuple with normalization.

My patch currently does 1. 
My patch, without the PyErr_NormalizeException() call would be 2.
I think maybe 3. would be the best solution, or 4., if normalization is desired in all cases.

I can write a new patch, if the experts tell me what is the best solution from the four (or some other I didn't think of).
msg130517 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-03-10 17:23
I'd choose solution 3, but instead of calling SyntaxError.__init__, call PyErr_NormalizeException().
msg130518 - (view) Author: Daniel Urban (daniel.urban) * (Python triager) Date: 2011-03-10 17:57
Err... sorry, I don't understand again:

If we get a tuple, create a new, store it without normalization. That's okay, I understand.

If we get a SyntaxError instance, then take its args field, create the new tuple. Then call PyErr_NormalizeException(), with:
a) the new tuple? But this will create a new SyntaxError instance...
b) the old SyntaxError instance? But this won't correct the wrong fields of the instance. (I think SyntaxError.__init__ would correct them without creating a new instance.)
c) or replace the args of the SyntaxError instance with the new tuple, then call PyErr_NormalizeException() on the instance? But I think that still won't correct the other fields of the instance...

Sorry for all these questions... I'd just like to help.
msg179704 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-01-11 17:06
This no longer seems to be a problem in Python 3.2, 3.3, or 3.4.
History
Date User Action Args
2013-01-11 17:06:19brett.cannonsetstatus: open -> closed
resolution: out of date
messages: + msg179704
2011-03-10 17:57:44daniel.urbansetnosy: brett.cannon, georg.brandl, amaury.forgeotdarc, ncoghlan, benjamin.peterson, july, daniel.urban
messages: + msg130518
2011-03-10 17:23:00amaury.forgeotdarcsetnosy: brett.cannon, georg.brandl, amaury.forgeotdarc, ncoghlan, benjamin.peterson, july, daniel.urban
messages: + msg130517
2011-03-10 17:13:55daniel.urbansetnosy: brett.cannon, georg.brandl, amaury.forgeotdarc, ncoghlan, benjamin.peterson, july, daniel.urban
messages: + msg130515
2011-03-09 12:54:55amaury.forgeotdarcsetnosy: brett.cannon, georg.brandl, amaury.forgeotdarc, ncoghlan, benjamin.peterson, july, daniel.urban
messages: + msg130454
2011-03-09 12:50:05daniel.urbansetnosy: brett.cannon, georg.brandl, amaury.forgeotdarc, ncoghlan, benjamin.peterson, july, daniel.urban
messages: + msg130453
2011-03-09 11:08:26julysetfiles: - unnamed
nosy: brett.cannon, georg.brandl, amaury.forgeotdarc, ncoghlan, benjamin.peterson, july, daniel.urban
2011-03-09 09:44:11amaury.forgeotdarcsetnosy: brett.cannon, georg.brandl, amaury.forgeotdarc, ncoghlan, benjamin.peterson, july, daniel.urban
messages: + msg130436
2011-03-09 09:22:37julysetfiles: + unnamed

messages: + msg130434
nosy: brett.cannon, georg.brandl, amaury.forgeotdarc, ncoghlan, benjamin.peterson, july, daniel.urban
2011-03-09 07:34:56daniel.urbansetnosy: brett.cannon, georg.brandl, amaury.forgeotdarc, ncoghlan, benjamin.peterson, july, daniel.urban
messages: + msg130431
2011-03-08 23:37:54amaury.forgeotdarcsetnosy: brett.cannon, georg.brandl, amaury.forgeotdarc, ncoghlan, benjamin.peterson, july, daniel.urban
messages: + msg130386
2011-03-08 22:25:42daniel.urbansetfiles: + issue11441_2.patch
nosy: brett.cannon, georg.brandl, amaury.forgeotdarc, ncoghlan, benjamin.peterson, july, daniel.urban
messages: + msg130380
2011-03-08 22:07:34benjamin.petersonsetnosy: brett.cannon, georg.brandl, amaury.forgeotdarc, ncoghlan, benjamin.peterson, july, daniel.urban
messages: + msg130378
2011-03-08 21:44:16terry.reedysetnosy: + benjamin.peterson, georg.brandl, brett.cannon, ncoghlan
2011-03-08 21:07:18daniel.urbansetfiles: + issue11441.patch

messages: + msg130374
keywords: + patch
2011-03-08 18:04:17amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg130346
2011-03-08 17:31:41daniel.urbansetnosy: + daniel.urban

messages: + msg130343
versions: + Python 3.2
2011-03-08 16:24:43julycreate