classification
Title: ImportError needs attributes for module and file name
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: Trundle, belopolsky, brett.cannon, brian.curtin, cool-RR, eric.araujo, eric.snow, giampaolo.rodola, gruszczy, haypo, loewis, ncoghlan, nedbat, pitrou, python-dev, r.david.murray, techtonik
Priority: normal Keywords: patch

Created on 2006-09-15 19:55 by nedbat, last changed 2012-04-13 00:27 by brett.cannon. This issue is now closed.

Files
File name Uploaded Description Edit
1559549_3.patch gruszczy, 2011-03-18 22:56 review
issue1559549.diff brian.curtin, 2011-10-26 00:03 review
issue1559549_v2.diff brian.curtin, 2012-02-09 18:59 review
importerror.diff brett.cannon, 2012-02-10 14:55 review
Messages (40)
msg54901 - (view) Author: Ned Batchelder (nedbat) * Date: 2006-09-15 19:55
Exceptions would be more useful if they had some
structured information attached to them.  For example,
an ImportError could have the name of the module that
could not be imported.  This would make it possible to
deal with exceptions in more powerful ways.

For more discussion:
http://www.nedbatchelder.com/blog/200609.html#e20060906T055924
msg54902 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2006-09-16 17:52
Logged In: YES 
user_id=21627

Exceptions do have structured informations, for example

py> try:
...   open("/tmp/xxx")
... except IOError, e:
...   print e.__dict__
...
{'errno': 2, 'args': (2, 'No such file or directory'),
'strerror': 'No such file or directory', 'filename': '/tmp/xxx'}

It's just that ImportError doesn't, so I'm retitling this
request to restrict attention to ImportError.

If you have other proposals for specific information that
should be on specific exceptions, please submit a separate
issue.

Would you like to work on this specific problem? I think
ImportError should get a module attribute (always set), and
a filename attribute (set only if a file was selected, yet
failed to import; otherwise set to None). It probably will
require some refactoring of C code to simplify raising
ImportError in the importing code.

Explicit raises of ImportError should also be considered;
those in the standard library should be fixed to include
atleast the module; raising ImportError without giving a
module should set the module to None.
msg125660 - (view) Author: anatoly techtonik (techtonik) Date: 2011-01-07 17:07
I think we should investigate deeper why this enhancement request didn't get into Python 3.

Another user story is: "from xxx import yyy", if yyy not found, the args message is ('cannot import name yyy',)

Python4?

label:api
msg125661 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-01-07 17:31
There's no need for any deeper investigation.  The answer is "nobody wrote the patch".  If someone writes a good patch, it will go in.
msg125662 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2011-01-07 17:34
> I think we should investigate deeper why this enhancement
> request didn't get into Python 3.

There is nothing to investigate here.  This is a request for a marginal improvement and OP did not follow up even though a core developer responded on the next day after his post.

The only lesson to be learned from this is that Python improvements should be discussed on the tracker or appropriate python mailing lists and not on private blogs.
msg130190 - (view) Author: Filip Gruszczyński (gruszczy) Date: 2011-03-06 20:58
This is a draft of a patch. I have only used this new ImportError api in once place, so it would work with following code:

>>> try:
...  import nosuchmodule  
... except ImportError as e:
...  print(e.module)
... 
nosuchmodule

I have literally no experience with Python core, so I would be very grateful for comments and advice, so I could make this patch meet all requirements.
msg130218 - (view) Author: Andreas Stührk (Trundle) Date: 2011-03-07 02:01
There are some issues with the patch:

- The check for size of `args` in `ImportError_init()` is wrong: You can't create an `ImportError` with 3 arguments now ("TypeError: ImportError expected 2 arguments, got 3")
- `ImportError_clear()` doesn't clear ``self->msg``.
- `ImportError_str()` doesn't increase the reference count for ``self->msg`` before returning it.
- The code that raises the exception (Python/import.c) doesn't check for error return values: All the calls can return NULL which will cause a segfault due to the uncoditional Py_DECREFs.
- Using `PyUnicode_DecodeASCII()` for the module name is wrong (IMHO), it should rather be something like `PyUnicode_DecodeUTF8()` as module names can contain non-ASCII characters in Python 3 and are AFAIK (at least right now) always encoded using utf-8.
msg130243 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-03-07 09:18
The module name is a UTF-8 encoded string yes. It should be documented in PyModuleDef structure. I already documented the encoding in PyModule_New().
msg130250 - (view) Author: Filip Gruszczyński (gruszczy) Date: 2011-03-07 11:49
I am sorry again for those mistakes, it's all completely new to me. I have fixed those issues and created new patch. Using hg export, that now spans over two commits. Is it the way those patches should be provided, or should I gather all changes into a one commit?
msg130950 - (view) Author: Andreas Stührk (Trundle) Date: 2011-03-15 03:58
> I am sorry again for those mistakes, it's all completely new to me.

No worries!

>I have fixed those issues and created new patch. Using hg export, that now spans over two commits. Is it the way those patches should be provided, or should I gather all changes into a one commit?

A single patch (where all changesets are flattened into one) is the
preferred way. For general advice, you can read the Developer's Guide
at http://docs.python.org/devguide/ (e.g.
http://docs.python.org/devguide/patch.html).
msg131027 - (view) Author: Filip Gruszczyński (gruszczy) Date: 2011-03-15 19:38
I didn't know about this mq extension. I will do a proper patch with a test after friday.
msg131147 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2011-03-16 17:41
At the PyCon 2011 sprint we discussed this issue and Nick, myself, and some other people agreed that using a keyword-only argument for passing in the module name is probably a better solution. While it won't be backwards-compatible (BaseException does not accept keyword arguments), it does provide a very clean API with an unambiguous way of specifying the module. Going another route (e.g., a constructor method) has the same backwards-compatibility issue. But a reason to use a solution other than the magical handling of the second argument is that it prevents doing the wrong thing simply because someone passes two or more arguments to ImportError.

Another nicety of a new API for ImportError is that it can be made such that if the keyword-only argument ('module_name'?) is the only thing supplied (e.g., no positional arguments) then the message can be auto-generated, which would be a nice way to solve issue #8754.
msg131153 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-03-16 18:15
+1 for providing a way to autogenerate the message.
msg131368 - (view) Author: Filip Gruszczyński (gruszczy) Date: 2011-03-18 22:56
Ok, here is a patch created using mq. I have a problem, however. I managed to solve following situation:

try:
    raise ImportError('failed import' module_name='somemodule')
except ImportError as e:
    print(e.module_name)

that would print somemodule.

However, I can't pass kwargs to ImportError_init from load_next. If someone could instruct me, how this can be achieved, I'll be happy to do that.
msg140266 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-07-13 15:17
How about this case:

  from validmodule import name_with_typo

Do we need one keyword argument module_name and one attribute_name?
msg140307 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2011-07-13 21:05
No, we don't need attribute_name as that is getting too specific. Your example is simply importing validmodule.name_with_typo which happens to possibly be an attribute on the module instead of another module or subpackage.
msg142251 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2011-08-17 03:59
I have a use for this in my bootstrapping for importlib, so I am assigning this to myself in order to make sure that at least ImportError grows the needed keyboard argument and attribute. I will probably not bother with tweaking import.c, though.
msg146404 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2011-10-26 00:03
Here's an updated patch, plus support for a second attribute that I need for #10854. I previously wrote a patch that does this same thing for that issue, but this one handles things a lot more nicely :)

I renamed "module_name" to just be "name" since I was adding "path" and didn't want to have to name it "module_path" and have two "module_" attributes since I think it's clear what they are for. We can go back to "module_" names if you want - no big deal.

It's really just the same patch as before but updated for a few minor changes (the ComplexExtendsException sig changed), and the moving of keyword handling in ImportError_init into a macro since we have to do the same initialization twice now. I'm not married to that implementation, it just minimized duplication and seems to work alright.

Also, this removes the import.c change (see Brett's last message).
msg149517 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2011-12-15 08:20
Just following up on this ticket.  Anyone have any objections to Brian's patch?

Also, would 'fullname' be more appropriate than 'name', to be more in sync with that identifier in importlib?
msg149521 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-12-15 09:57
The patch doesn't appear to have the necessary mechanics to pass the new arguments from the import machinery when an ImportError is raised, is this deliberate?

Also, I'm not sure why the new arguments are keyword-only.
msg149539 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-12-15 12:13
The keyword-only idea is a backwards compatibility hack we discussed at the PyCon US sprints because ImportError currently accepts an arbitrary number of arguments:

>>> raise ImportError(1, 2, 3)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: (1, 2, 3)

We don't really want to be claiming that the module name is some strange value due to existing code that passes multiple arguments, hence the suggestion to make this a keyword-only argument.

Regarding import.c, I think Brian misinterpreted Brett's last message. import.c absolutely *should* be modified by this patch, since it's still the default import implementation for the moment. Brett's comment was just to say that *he* wasn't going to do that part, since his aim with the importlib bootstrapping exercise is to nuke most of import.c from orbit :)
msg149592 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2011-12-16 05:15
I'm guessing that more than just Python/import.c should be updated, and more than one spot in import.c.
msg149593 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2011-12-16 05:20
If I add back in the import.c change, would this then be alright?

Eric - fullname seems fine, I'll update that.
msg149603 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-12-16 09:11
"fullname" is technically wrong:

>>> import logging.bar
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: No module named 'bar'

"module_name" sounds fine and explicit enough to me. Also, as Eric points out, there are many places where an ImportError is raised. You might want to create a private helper API.
msg149628 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2011-12-16 15:56
I think I'm going to stick with name unless anyone is super opposed. If we can eventually import something else (sausages?), then setting module_name with a sausage name will seem weird.

I'll work up a more complete patch. The private helper is a good idea.
msg152921 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-02-09 00:43
Brian, what is left for updating your patch? I'm going to need this to fix test_pydoc to not fail in my importlib bootstrap work so I can (finally) help with this. Is it just actually using the new features of the exception?
msg152930 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2012-02-09 05:03
Yep, I just need to actually make use of the feature. I'll generate a new patch shortly.
msg152956 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-02-09 16:18
On Thu, Feb 9, 2012 at 00:03, Brian Curtin <report@bugs.python.org> wrote:

>
> Brian Curtin <brian@python.org> added the comment:
>
> Yep, I just need to actually make use of the feature. I'll generate a new
> patch shortly.
>

If you can generate it before 17:30 EST today I can review it or at least
help use it in the codebase.
msg152974 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2012-02-09 18:59
Here's an updated patch which creates two convenience functions, PyErr_SetFromImportErrorWithName and PyErr_SetFromImportErrorWithNameAndPath. I figure the two common cases are that you'll want to set just a name or you'll want a name and a path.

*WithName is all that I've applied for now, and I've only done it in import.c to allow Brett to continue with his work. I'll come back and make changes elsewhere in the code, and I'll apply the *WithNameAndPath function where I need it for #10854.

All tests pass and some IRL testing works nicely.

Note: I'm a little rushed at the moment so I have not included docs but I will surely update them. I just want to get Brett what he needs by this afternoon.
msg152977 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-02-09 19:37
Thanks, Brian! I'll do a review tonight on the drive home (and maybe even write the docs up).
msg152978 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-02-09 20:02
On the drive home...are you borrowing one of Google's self driving cars? :)
msg153047 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-02-10 14:40
The patch looks fine for its current semantics except for the fact that the macro doesn't work under clang; ##macro_var is supposed to be used only when concatenating parts of a string to create a complete identifier, not concatenating two identifiers next to each other (e.g. gooblede##gook is fine, but self->##gook isn't because '->' is its own identifier).

But one issue is that for my purposes I need 'name' to be the full name of the module, not just the tail of the full name. The reason is that pydoc needs a way to tell when a module import failed because the module isn't there vs. when a module had an actual error itself. Using the tail end of the name doesn't help me because it means I can't tell if an import of a module named 'bunk' containing the line 'import test.bunk' fails because the module 'bunk' doesn't exist or because the import line failed since in both cases name == 'bunk'. But if name == 'test.bunk' there is no ambiguity. Pydoc used to do this by inspecting the stack and seeing where the exception was thrown which is just nasty and brittle (i.e. doesn't work with importlib since the stack goes much farther until ImportError is thrown).

IOW I don't think we should be tossing out valuable data just because historically modules that didn't exist only returned the tail end of the full module name. Anyone object to switching to using the full name of the module triggering the exception? The message wouldn't change to use the full name (unfortunately) for backwards-compatibility reasons of course.
msg153048 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-02-10 14:41
And to answer David's joke, I carpool from Toronto to Waterloo four days a week so I have an hour each direction to work on stuff.
msg153051 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-02-10 14:46
Oh, and there are no forward declarations for the new functions added to errors.c
msg153054 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-02-10 14:55
Attached is Brian's patch with the macro fix and forward declarations.
msg153060 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-02-10 16:00
So I just tried to pass fullname in import.c and it didn't work (of course). Looks like even when I try to use fullname in find_module_path_list() and load_next() it is still leaving out the package name. Ugh. That means either refactoring import.c to get the full name in all places, not get the full name (and thus I'm still screwed), have importlib do something different than import.c in hopes that importlib replaces import.c before this goes public, or add a full_name attribute for importlib to fill in with the full details.
msg153070 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-02-10 17:56
However you do it, I'm very much in favor of having the full name available.  I either wrote or fixed (I can't remember which) that stack walk in pydoc, and you are right, it is very very ugly.  This would also be a big benefit for unittest, which currently *doesn't* do the stack walk and therefore generates incorrect error messages when sub-imports fail.
msg158002 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-04-11 01:17
This is now officially blocking my importlib bootstrap work (issue #2377), so I have assigned this to myself to get done and I hope to get this committed within a week *without* updating Python/import.c and the requisite tests or pydoc (which I will make part of my merge instead). If Brian beats me to getting this checked in that's great, but otherwise I will get to it myself.
msg158188 - (view) Author: Roundup Robot (python-dev) Date: 2012-04-13 00:25
New changeset 6825fd9b00ed by Brett Cannon in branch 'default':
Issue #1559549: Add 'name' and 'path' attributes to ImportError.
http://hg.python.org/cpython/rev/6825fd9b00ed
msg158189 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-04-13 00:27
Patch for the attributes is in (no use by import.c)! I'm going to do a separate commit for use by importlib and then fix pydoc in my bootstrap branch.
History
Date User Action Args
2012-04-13 00:27:13brett.cannonsetstatus: open -> closed
resolution: fixed
messages: + msg158189

stage: patch review -> resolved
2012-04-13 00:25:14python-devsetnosy: + python-dev
messages: + msg158188
2012-04-12 23:39:50brett.cannonsetassignee: brian.curtin -> brett.cannon
2012-04-11 01:17:55brett.cannonsetmessages: + msg158002
2012-04-11 01:13:07brett.cannonlinkissue2377 dependencies
2012-03-07 19:56:48brett.cannonunlinkissue8754 dependencies
2012-02-10 17:56:40r.david.murraysetmessages: + msg153070
2012-02-10 16:00:39brett.cannonsetmessages: + msg153060
2012-02-10 14:55:54brett.cannonsetfiles: + importerror.diff

messages: + msg153054
2012-02-10 14:46:50brett.cannonsetmessages: + msg153051
2012-02-10 14:41:19brett.cannonsetmessages: + msg153048
2012-02-10 14:40:44brett.cannonsetmessages: + msg153047
2012-02-09 22:48:17brett.cannonlinkissue8754 dependencies
2012-02-09 20:02:53r.david.murraysetmessages: + msg152978
2012-02-09 19:37:45brett.cannonsetmessages: + msg152977
2012-02-09 18:59:59brian.curtinsetfiles: + issue1559549_v2.diff

messages: + msg152974
2012-02-09 16:18:08brett.cannonsetmessages: + msg152956
2012-02-09 05:03:06brian.curtinsetmessages: + msg152930
2012-02-09 00:43:03brett.cannonsetassignee: brett.cannon -> brian.curtin
messages: + msg152921
2011-12-16 15:56:43brian.curtinsetmessages: + msg149628
2011-12-16 09:11:26pitrousetmessages: + msg149603
2011-12-16 05:20:44brian.curtinsetmessages: + msg149593
2011-12-16 05:15:44eric.snowsetmessages: + msg149592
2011-12-15 12:13:43ncoghlansetmessages: + msg149539
2011-12-15 09:57:49pitrousetnosy: + pitrou

messages: + msg149521
stage: test needed -> patch review
2011-12-15 08:20:21eric.snowsetmessages: + msg149517
2011-11-11 16:30:13brian.curtinlinkissue10854 dependencies
2011-10-26 00:03:34brian.curtinsetfiles: + issue1559549.diff
nosy: + brian.curtin
messages: + msg146404

2011-08-17 03:59:14brett.cannonsetassignee: brett.cannon
messages: + msg142251
2011-07-13 21:05:36brett.cannonsetmessages: + msg140307
2011-07-13 16:12:24eric.snowsetnosy: + eric.snow
2011-07-13 15:17:56eric.araujosetnosy: + eric.araujo
messages: + msg140266
2011-03-18 22:56:53gruszczysetfiles: + 1559549_3.patch
nosy: loewis, brett.cannon, ncoghlan, belopolsky, haypo, techtonik, giampaolo.rodola, nedbat, r.david.murray, Trundle, gruszczy, cool-RR
messages: + msg131368
2011-03-18 22:54:47gruszczysetfiles: - 1559549_2.patch
nosy: loewis, brett.cannon, ncoghlan, belopolsky, haypo, techtonik, giampaolo.rodola, nedbat, r.david.murray, Trundle, gruszczy, cool-RR
2011-03-18 22:54:43gruszczysetfiles: - 1559549_1.patch
nosy: loewis, brett.cannon, ncoghlan, belopolsky, haypo, techtonik, giampaolo.rodola, nedbat, r.david.murray, Trundle, gruszczy, cool-RR
2011-03-16 18:15:08r.david.murraysetnosy: loewis, brett.cannon, ncoghlan, belopolsky, haypo, techtonik, giampaolo.rodola, nedbat, r.david.murray, Trundle, gruszczy, cool-RR
messages: + msg131153
2011-03-16 17:41:46brett.cannonsetnosy: loewis, brett.cannon, ncoghlan, belopolsky, haypo, techtonik, giampaolo.rodola, nedbat, r.david.murray, Trundle, gruszczy, cool-RR
messages: + msg131147
2011-03-15 19:38:29gruszczysetnosy: loewis, brett.cannon, ncoghlan, belopolsky, haypo, techtonik, giampaolo.rodola, nedbat, r.david.murray, Trundle, gruszczy, cool-RR
messages: + msg131027
2011-03-15 19:35:38ncoghlansetnosy: + ncoghlan
2011-03-15 03:58:52Trundlesetnosy: loewis, brett.cannon, belopolsky, haypo, techtonik, giampaolo.rodola, nedbat, r.david.murray, Trundle, gruszczy, cool-RR
messages: + msg130950
2011-03-07 11:49:18gruszczysetfiles: + 1559549_2.patch
nosy: loewis, brett.cannon, belopolsky, haypo, techtonik, giampaolo.rodola, nedbat, r.david.murray, Trundle, gruszczy, cool-RR
messages: + msg130250
2011-03-07 09:18:07hayposetnosy: loewis, brett.cannon, belopolsky, haypo, techtonik, giampaolo.rodola, nedbat, r.david.murray, Trundle, gruszczy, cool-RR
messages: + msg130243
2011-03-07 02:01:04Trundlesetnosy: + Trundle, haypo
messages: + msg130218
2011-03-06 20:58:49gruszczysetfiles: + 1559549_1.patch

messages: + msg130190
keywords: + patch
nosy: loewis, brett.cannon, belopolsky, techtonik, giampaolo.rodola, nedbat, r.david.murray, gruszczy, cool-RR
2011-03-02 16:45:21gruszczysetnosy: + gruszczy
2011-03-01 09:54:33cool-RRsetnosy: + cool-RR
2011-03-01 06:35:45georg.brandllinkissue11356 superseder
2011-01-07 19:09:02giampaolo.rodolasetnosy: + giampaolo.rodola
2011-01-07 17:34:32belopolskysetnosy: + belopolsky
messages: + msg125662
2011-01-07 17:31:24r.david.murraysetnosy: + r.david.murray
messages: + msg125661
2011-01-07 17:10:30brian.curtinsetnosy: loewis, brett.cannon, techtonik, nedbat
versions: + Python 3.3, - Python 3.2
2011-01-07 17:07:40techtoniksetnosy: + techtonik
messages: + msg125660
2011-01-07 16:28:22brian.curtinlinkissue10857 superseder
2010-08-09 03:29:02terry.reedysetversions: + Python 3.2, - Python 3.1, Python 2.7
2009-03-30 03:08:47ajaksu2setnosy: + brett.cannon
versions: + Python 3.1, Python 2.7, - Python 2.6

stage: test needed
2008-01-06 12:11:06christian.heimessetversions: + Python 2.6
2006-09-15 19:55:04nedbatcreate