| msg268849 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) *  |
Date: 2016-06-19 11:53 |
| Seems there is a bug in the implementation of the IMPORT_NAME opcode in ceval.c. If the level argument is -1, it is not passed to __import__ (should never happen, but looks correct). If it is an integer != -1 in C long range, it is passed to __import__ (this is correct). But if it is not integer (e.g. None) or can't be converted to C long, an exception is set and __import__ is called with level and not cleared error (this is wrong). In correct bytecode the level argument can be either integer or None. Default __import__ accepts only integers as the level argument and checks the range. Proposed patch makes the code always passing the level argument to __import__ unless it is None. |
|
|
| msg268931 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2016-06-20 20:57 |
| My suspicion is the -1 aspect is a hold-over from that being the default level value in Python 2.7 that no one removed. The patch LGTM. Do we need a test for this for some reason? |
|
|
| msg268939 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-06-20 21:35 |
| Thak you for the explanation Brett. Since current "import" without "from" is not broken, I suppose that the raised exception is cleared somewhere later. The patch doesn't have visible effect (unless may be using nonstandard loaders or __import__, don't know). It just cleans up the code, avoid raising unneeded exception (small performance boost) and may be fix some hidden non-reproducible bug. |
|
|
| msg268940 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2016-06-20 21:36 |
| Then I say go ahead and commit it. |
|
|
| msg269306 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-06-26 19:51 |
| Sorry, I was wrong. The level argument is always non-negative integer (if a bytecode is not corrupted). The second branch can be just removed. Following patch also fixes the validation of the ImportFrom AST node. |
|
|
| msg269385 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2016-06-27 16:33 |
| LGTM |
|
|
| msg269394 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2016-06-27 18:39 |
| New changeset e3164c9edb0b by Serhiy Storchaka in branch 'default': Issue #27352: Correct the validation of the ImportFrom AST node and simplify https://hg.python.org/cpython/rev/e3164c9edb0b |
|
|
| msg269395 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-06-27 18:40 |
| Thanks Brett. |
|
|
| msg269401 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2016-06-27 20:33 |
| You broke Python! Example: http://buildbot.python.org/all/builders/AMD64%20Snow%20Leop%203.x/builds/4853/steps/test/logs/stdio ====================================================================== FAIL: test_importfrom (test.test_ast.ASTValidatorTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_ast.py", line 757, in test_importfrom self.stmt(imp, "level less than -1") File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_ast.py", line 579, in stmt self.mod(mod, msg) File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_ast.py", line 571, in mod self.assertIn(msg, str(cm.exception)) AssertionError: 'level less than -1' not found in 'Negative ImportFrom level' |
|
|
| msg269402 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2016-06-27 20:41 |
| New changeset e5063a82f490 by Serhiy Storchaka in branch 'default': Issue #27352: Fixed an error message in a test. https://hg.python.org/cpython/rev/e5063a82f490 |
|
|
| msg269403 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-06-27 20:54 |
| Thanks Victor! |
|
|