bpo-29546: Improve from-import error message with location #103

Carreau · 2017-02-15T01:00:10Z

Add location information like canonical module name where identifier
cannot be found and file location if available.

First iteration of this was gh-91

Testless for now, I can add tests once the exact format has been agreed upon.

brettcannon · 2017-02-15T19:25:52Z

Python/ceval.c

@@ -5022,12 +5022,23 @@ import_from(PyObject *v, PyObject *name)
    return x;
 error:
    pkgpath = PyModule_GetFilenameObject(v);
+    if (name == NULL){


Missing a space between ) and {.

warsaw

Yep, that's exactly the format I was thinking! Great work.

warsaw · 2017-02-15T19:36:25Z

Python 3.7.0a0 (default, Feb 15 2017, 14:30:57) 
[GCC 6.3.0 20161229] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from os import floop
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: cannot import name 'floop' from 'os' (/home/barry/projects/python/cpython/Lib/os.py)
>>> import snarp
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'snarp'
>>> 
>>> from _opcode import fip
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: cannot import name 'fip' from '_opcode' (/home/barry/projects/python/cpython/build/lib.linux-x86_64-3.7/_opcode.cpython-37m-x86_64-linux-gnu.so)
>>>

Carreau · 2017-02-15T20:12:58Z

Yep, that's exactly the format I was thinking! Great work.

Good, and thanks. I'll cleanup and add tests then !

Carreau · 2017-02-16T03:56:10Z

Added tests, Misc/NEWS, What's new.

The only thing I'm unsure is what the module's __file__ extensions is on windows. I'm guessing I can relax the regex in the test. I've assumed .dll.

It's also a bit ugly if a module has neither name not a file:

"cannot import name 'does_not_exist' from '<unknown module name>' (unknown location)"

But that seem to be rare from how it's currently tested:

class AlwaysAttributeError:
            def __getattr__(self, _):
                raise AttributeError

module_name = 'test_from_import_AttributeError'
self.addCleanup(unload, module_name)
sys.modules[module_name] = AlwaysAttributeError()

warsaw · 2017-02-16T14:54:39Z

On Feb 15, 2017, at 07:56 PM, Matthias Bussonnier wrote: It's also a bit ugly if a module has neither name not a file: > "cannot import name 'does_not_exist' from '<unknown module name>' (unknown location)" But that seem to be rare from how it's currently tested:

Agreed it's ugly, but if it won't happen commonly in practice, it's probably not worth dealing with specially. I'm also not sure how you'd print that any better - it is worth knowing what's unknowable. :)

warsaw

Please review the potential reference counting bugs.

warsaw · 2017-02-18T00:53:29Z

Python/ceval.c

+        pkgname_or_unknown = PyUnicode_FromString("<unknown module name>");
+    } else {
+        pkgname_or_unknown = pkgname;
+    }


I think there are some reference counting bugs lurking here.

It's possible the code jumps to error: after pkgname is decref'd. I think that decref should be deferred until just before the good-path return.

pkgname_or_unknown will be a new reference if pkgname is NULL, or it will steal the pkgname reference if it's not. In either case, the error stanza should decref pkgname_or_unknown, which will take care of ensuring the object gets decref for any goto error path.

pkgpath doesn't get decref'd. I think you should Py_XDECREF it before the return NULL just in case pkgpath is itself NULL.

Thanks for the detailed explanation, I still need to get used to do manual ref counting.
I've push changes. Let me know if I got things rights.

@Carreau Yes, I think the refcounting is right now. It can sometimes be difficult to trace all the various exit paths to make sure that everything gets decref'd and incref'd properly, but as best I can tell, you've got it right now. Thanks!

The only other thing is that it's possible for the PyUnicode_FromString("<unknown module name>") to fail and return NULL. Probably the only realistic cause of that would be a memory error (not sure if an encoding error could do it given that this is an ascii C string). If you want to be pedanditc, it would be useful to check for that error condition and do something more reasonable in that case.

If you want to be pedanditc, it would be useful to check for that error condition and do something more reasonable in that case

I'm unsure what "more reasonable" is, I check and failed with the original error message in that case, but I don't see how if allocating PyUnicode_FromString("<unknown module name>") fails we can do something sensible.

Probably the best you can do is to just decref any objects that may still have a reference count, and then immediately return NULL. The PyUnicode_FromString failure would have already set an exception, so that's the one you'll see. Something like:

1 file changed, 4 insertions(+) Python/ceval.c | 4 ++++ modified Python/ceval.c @@ -5024,6 +5024,10 @@ import_from(PyObject *v, PyObject *name) pkgpath = PyModule_GetFilenameObject(v); if (pkgname == NULL) { pkgname_or_unknown = PyUnicode_FromString("<unknown module name>"); + if (pkgname_or_unknown == NULL) { + Py_XDECREF(pkgpath); + return NULL; + } } else { pkgname_or_unknown = pkgname; }

warsaw

Semicolons to end lines.

Please check for PyUnicode_FromString() returning NULL.

warsaw · 2017-02-18T20:05:44Z

Python/ceval.c

    }

+    Py_XDECREF(pkgname_or_unknown)
+    Py_XDECREF(pkgpath)


Oh whoops, these lines need to end with semicolons.

Carreau · 2017-02-20T23:12:43Z

semicolon added

warsaw

Two more small comments, but I think this is really close!

warsaw · 2017-02-20T23:27:13Z

Python/ceval.c

+        pkgname_or_unknown = PyUnicode_FromString("<unknown module name>");
+    } else {
+        pkgname_or_unknown = pkgname;
+    }


Probably the best you can do is to just decref any objects that may still have a reference count, and then immediately return NULL. The PyUnicode_FromString failure would have already set an exception, so that's the one you'll see. Something like:

1 file changed, 4 insertions(+) Python/ceval.c | 4 ++++ modified Python/ceval.c @@ -5024,6 +5024,10 @@ import_from(PyObject *v, PyObject *name) pkgpath = PyModule_GetFilenameObject(v); if (pkgname == NULL) { pkgname_or_unknown = PyUnicode_FromString("<unknown module name>"); + if (pkgname_or_unknown == NULL) { + Py_XDECREF(pkgpath); + return NULL; + } } else { pkgname_or_unknown = pkgname; }

warsaw · 2017-02-20T23:27:24Z

Python/ceval.c


-    if (pkgpath == NULL || !PyUnicode_Check(pkgpath)) {
+    if (pkgname_or_unknown == NULL){


You need a space between the paren and open brace.

Add location information like canonical module name where identifier cannot be found and file location if available. First iteration of this was pythongh-91

from Barry Warsaw: - It's possible the code jumps to error: after pkgname is decref'd. I think that decref should be deferred until just before the good-path return. - pkgname_or_unknown will be a new reference if pkgname is NULL, or it will steal the pkgname reference if it's not. In either case, the error stanza should decref pkgname_or_unknown, which will take care of ensuring the object gets decref for any goto error path. - pkgpath doesn't get decref'd. I think you should Py_XDECREF it before the return NULL just in case pkgpath is itself NULL.

Carreau · 2017-02-22T03:01:35Z

Comments should be addressed. Thanks !

warsaw · 2017-02-22T15:06:22Z

@Carreau Thanks, this LGTM now. I'm pushing the big green button.

Carreau · 2017-02-22T15:50:12Z

Thanks !

Fix an invalid assertion during interpreter shutdown and add a test case for this problem. https://bitbucket.org/stackless-dev/stackless/issues/103 (grafted from a2f660260c93e911fde4969dac79c9eb6cc03e72)

The reworked C-function slp_kill_tasks_with_stacks() didn't work correctly for deeply nested tasklets. Therefore I had to rework it again. This version first kills a tasklet and only then clears the tstate of its cstacks. I added several new test cases and reorganised them a bit. https://bitbucket.org/stackless-dev/stackless/issues/103 (grafted from 130a60b8d3719e06807acd7e9b63c07bd5435c32)

the-knights-who-say-ni added the CLA signed label Feb 15, 2017

Carreau force-pushed the import-messages branch from 7124949 to ff44351 Compare February 15, 2017 02:28

brettcannon requested a review from warsaw February 15, 2017 19:25

brettcannon reviewed Feb 15, 2017

View reviewed changes

warsaw approved these changes Feb 15, 2017

View reviewed changes

brettcannon added the type-feature A feature request or enhancement label Feb 15, 2017

Carreau force-pushed the import-messages branch from ff44351 to 36c8950 Compare February 16, 2017 03:52

warsaw requested changes Feb 18, 2017

View reviewed changes

warsaw approved these changes Feb 18, 2017

View reviewed changes

warsaw requested changes Feb 18, 2017

View reviewed changes

Python/ceval.c Outdated

}

Py_XDECREF(pkgname_or_unknown)

Py_XDECREF(pkgpath)

Copy link

Member

warsaw Feb 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh whoops, these lines need to end with semicolons.

Carreau force-pushed the import-messages branch from 0e29d44 to e757eec Compare February 20, 2017 23:00

warsaw requested changes Feb 20, 2017

View reviewed changes

Carreau added 3 commits February 20, 2017 15:52

bpo-29546: Improve from-import error message with location

a181463

Add location information like canonical module name where identifier cannot be found and file location if available. First iteration of this was pythongh-91

Handle if PyUnicode_FromString fails.

1b6e462

Carreau force-pushed the import-messages branch from dfa3512 to 1b6e462 Compare February 20, 2017 23:53

warsaw approved these changes Feb 22, 2017

View reviewed changes

warsaw merged commit 1bc1564 into python:master Feb 22, 2017

Carreau deleted the import-messages branch February 22, 2017 15:50

akruis pushed a commit to akruis/cpython that referenced this pull request Sep 9, 2017

merge 3.3-slp (Stackless python#103)

69b82ef

akruis pushed a commit to akruis/cpython that referenced this pull request Sep 9, 2017

merge 3.4-slp (Stackless python#103)

79a894c

akruis pushed a commit to akruis/cpython that referenced this pull request Sep 9, 2017

merge 3.3-slp (Stackless python#103)

126fa99

akruis pushed a commit to akruis/cpython that referenced this pull request Sep 9, 2017

merge 3.4-slp (Stackless python#103)

19b7b19

ghost mentioned this pull request Sep 24, 2017

Stackless 3.4 debug build testsuite failures stackless-dev/stackless#75

Closed

jaraco pushed a commit that referenced this pull request Dec 2, 2022

Use python 3.6.5 in Heroku. (GH-103)

7f2aec0

gvanrossum mentioned this pull request Aug 22, 2023

heap-use-after-free in _PyFunction_LookupByVersion #108253

Closed

ngoldbaum mentioned this pull request Sep 23, 2024

Crash running PyO3 tests with --test-threads=1000 #124375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-29546: Improve from-import error message with location #103

bpo-29546: Improve from-import error message with location #103

Carreau commented Feb 15, 2017

brettcannon Feb 15, 2017

warsaw left a comment

warsaw commented Feb 15, 2017

Carreau commented Feb 15, 2017

Carreau commented Feb 16, 2017 •

edited

Loading

warsaw commented Feb 16, 2017 via email

warsaw left a comment

warsaw Feb 18, 2017

Carreau Feb 18, 2017

warsaw Feb 18, 2017

Carreau Feb 20, 2017

warsaw Feb 20, 2017

warsaw left a comment

warsaw Feb 18, 2017

Carreau commented Feb 20, 2017

warsaw left a comment

warsaw Feb 20, 2017

warsaw Feb 20, 2017

Carreau commented Feb 22, 2017

warsaw commented Feb 22, 2017

Carreau commented Feb 22, 2017


		if (pkgpath == NULL \|\| !PyUnicode_Check(pkgpath)) {
		if (pkgname_or_unknown == NULL){

bpo-29546: Improve from-import error message with location #103

bpo-29546: Improve from-import error message with location #103

Conversation

Carreau commented Feb 15, 2017

Choose a reason for hiding this comment

warsaw left a comment

Choose a reason for hiding this comment

warsaw commented Feb 15, 2017

Carreau commented Feb 15, 2017

Carreau commented Feb 16, 2017 • edited Loading

warsaw commented Feb 16, 2017 via email

warsaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warsaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carreau commented Feb 20, 2017

warsaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carreau commented Feb 22, 2017

warsaw commented Feb 22, 2017

Carreau commented Feb 22, 2017

Carreau commented Feb 16, 2017 •

edited

Loading