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

Add location information like canonical module name where identifier cannot be found and file location if available. First iteration of this was gh-91
python · warsaw · Feb 22, 2017 · Feb 15, 2017 · Feb 18, 2017 · Feb 20, 2017
commit a1814637d8be7132d33792f0fa2f517f4dbe3cce
diff --git a/Doc/whatsnew/3.7.rst b/Doc/whatsnew/3.7.rst
@@ -83,6 +83,9 @@ Other Language Changes
  whitespace, not only spaces.
  (Contributed by Robert Xiao in :issue:`28927`.)

+* :exc:`ImportError` now displays module name and module ``__file__`` path when
+  ``from ... import ...`` fails. :issue:`29546`.
+

 New Modules
 ===========

diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py
@@ -85,6 +85,15 @@ def test_from_import_missing_attr_has_name_and_path(self):
            from os import i_dont_exist
        self.assertEqual(cm.exception.name, 'os')
        self.assertEqual(cm.exception.path, os.__file__)
+        self.assertRegex(str(cm.exception), "cannot import name 'i_dont_exist' from 'os' \(.*/Lib/os.py\)")
+
+    def test_from_import_missing_attr_has_name_and_so_path(self):
+        import _opcode
+        with self.assertRaises(ImportError) as cm:
+            from _opcode import i_dont_exist
+        self.assertEqual(cm.exception.name, '_opcode')
+        self.assertEqual(cm.exception.path, _opcode.__file__)
+        self.assertRegex(str(cm.exception), "cannot import name 'i_dont_exist' from '_opcode' \(.*\.(so|dll)\)")

    def test_from_import_missing_attr_has_name(self):
        with self.assertRaises(ImportError) as cm:
@@ -365,9 +374,12 @@ def __getattr__(self, _):
        module_name = 'test_from_import_AttributeError'
        self.addCleanup(unload, module_name)
        sys.modules[module_name] = AlwaysAttributeError()
-        with self.assertRaises(ImportError):
+        with self.assertRaises(ImportError) as cm:
            from test_from_import_AttributeError import does_not_exist

+        self.assertEqual(str(cm.exception),
+            "cannot import name 'does_not_exist' from '<unknown module name>' (unknown location)")
+

 @skip_if_dont_write_bytecode
 class FilePermissionTests(unittest.TestCase):

diff --git a/Misc/NEWS b/Misc/NEWS
@@ -21,6 +21,8 @@ Core and Builtins

 - bpo-29546: Set the 'path' and 'name' attribute on ImportError for ``from ... import ...``.

+- bpo-29546: Improve from-import error message with location
+
 - Issue #29319: Prevent RunMainFromImporter overwriting sys.path[0].

 - Issue #29337: Fixed possible BytesWarning when compare the code objects.

diff --git a/Python/ceval.c b/Python/ceval.c
@@ -4995,7 +4995,7 @@ import_from(PyObject *v, PyObject *name)
 {
    PyObject *x;
    _Py_IDENTIFIER(__name__);
-    PyObject *fullmodname, *pkgname, *pkgpath;
+    PyObject *fullmodname, *pkgname, *pkgpath, *pkgname_or_unknown;

    x = PyObject_GetAttr(v, name);
    if (x != NULL || !PyErr_ExceptionMatches(PyExc_AttributeError))
@@ -5022,12 +5022,23 @@ import_from(PyObject *v, PyObject *name)
    return x;
 error:
    pkgpath = PyModule_GetFilenameObject(v);
+    if (pkgname == NULL) {
+        pkgname_or_unknown = PyUnicode_FromString("<unknown module name>");
+    } else {
+        pkgname_or_unknown = pkgname;
+    }

    if (pkgpath == NULL || !PyUnicode_Check(pkgpath)) {
        PyErr_Clear();
-        PyErr_SetImportError(PyUnicode_FromFormat("cannot import name %R", name), pkgname, NULL);
+        PyErr_SetImportError(
+            PyUnicode_FromFormat("cannot import name %R from %R (unknown location)",
+                name, pkgname_or_unknown),
+            pkgname, NULL);
    } else {
-        PyErr_SetImportError(PyUnicode_FromFormat("cannot import name %R", name), pkgname, pkgpath);
+        PyErr_SetImportError(
+            PyUnicode_FromFormat("cannot import name %R from %R (%S)",
+                name, pkgname_or_unknown, pkgpath),
+            pkgname, pkgpath);
    }

    return NULL;
-Original file line number
+Diff line change
@@ -4995,7 +4995,7 @@ import_from(PyObject *v, PyObject *name)
     {
         PyObject *x;
         _Py_IDENTIFIER(__name__);
-        PyObject *fullmodname, *pkgname, *pkgpath;
+        PyObject *fullmodname, *pkgname, *pkgpath, *pkgname_or_unknown;
         x = PyObject_GetAttr(v, name);
         if (x != NULL || !PyErr_ExceptionMatches(PyExc_AttributeError))
@@ -5022,12 +5022,23 @@ import_from(PyObject *v, PyObject *name)
         return x;
      error:
         pkgpath = PyModule_GetFilenameObject(v);
+        if (pkgname == NULL) {
+            pkgname_or_unknown = PyUnicode_FromString("<unknown module name>");
+        } else {
+            pkgname_or_unknown = pkgname;
+        }
             Copy link

  
      
    
  

  
      

  
  Member


      

  

  
    
      

      
            warsaw
  

      

      

      


        Feb 18, 2017


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

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

    
      
      


  


  
    
      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.

    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    



    
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
    Contributor


      

  Author


  

  
    
      

      
            Carreau
  

      

      

      


        Feb 18, 2017


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

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

    
      
      


  


  
    
      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.
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    



    
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
  Member


      

  

  
    
      

      
            warsaw
  

      

      

      


        Feb 18, 2017


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

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

    
      
      


  


  
    
      @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.
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    



    
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
    Contributor


      

  Author


  

  
    
      

      
            Carreau
  

      

      

      


        Feb 20, 2017


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

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

    
      
      


  


  
    
      
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.
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    



    
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
  Member


      

  

  
    
      

      
            warsaw
  

      

      

      


        Feb 20, 2017


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

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

    
      
      


  


  
    
      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;
     }

    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
         if (pkgpath == NULL || !PyUnicode_Check(pkgpath)) {
             PyErr_Clear();
-            PyErr_SetImportError(PyUnicode_FromFormat("cannot import name %R", name), pkgname, NULL);
+            PyErr_SetImportError(
+                PyUnicode_FromFormat("cannot import name %R from %R (unknown location)",
+                    name, pkgname_or_unknown),
+                pkgname, NULL);
         } else {
-            PyErr_SetImportError(PyUnicode_FromFormat("cannot import name %R", name), pkgname, pkgpath);
+            PyErr_SetImportError(
+                PyUnicode_FromFormat("cannot import name %R from %R (%S)",
+                    name, pkgname_or_unknown, pkgpath),
+                pkgname, pkgpath);
         }
         return NULL;
-          Expand Down