classification
Title: isinstance swallows exceptions
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.1, Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: BreamoreBoy, belopolsky, benjamin.peterson, ferringb, georg.brandl, greppo, nnorwitz, r.david.murray, rhettinger, schmir, tseaver
Priority: high Keywords: patch

Created on 2006-10-10 02:55 by ferringb, last changed 2010-12-11 04:03 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
test.py ferringb, 2006-10-10 02:55 example.py
patch ferringb, 2006-11-05 04:06 clear just AttributeError
issue1574217_dont_mask_errors.txt schmir, 2008-01-11 23:31 updated fix+unittest
issue1574217.diff belopolsky, 2010-07-14 15:41 patch against py3k revision 82889 review
Messages (19)
msg30197 - (view) Author: Ferringb (ferringb) * Date: 2006-10-10 02:55
Attached is a simple example; yes, a bit contrived, but
it's exactly what bit me in the ass for a week or two :)

nestled within abstract.c's recursive_isinstance, is
this lil nugget-

icls = PyObject_GetAttr(inst, __class__);
if (icls == NULL) {
  PyErr_Clear();
  retval = 0;
}
else {

No surrouding comments to indicate *why* it's
swallowing exceptions, but best explanation I've heard
was that it was attempting to swallow just
AttributeError... which would make sense.

So the question is, whats the purpose of it swallowing
exceptions there?  Bad form of AttributeError catching,
or some unstated reason?
msg30198 - (view) Author: Ferringb (ferringb) * Date: 2006-10-10 02:56
Logged In: YES 
user_id=874085

addition note; this is both 2.5 and 2.4, probably stretches
bit further back also.
msg30199 - (view) Author: Ferringb (ferringb) * Date: 2006-11-05 04:06
Logged In: YES 
user_id=874085

quicky patch for this; basically, wipe the exception only if
it's AttributeError, else let it bubble it's way up.
msg30200 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2007-02-01 21:05
Neal, do you have time to look at this one?
msg30201 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2007-02-02 01:00
Possibly in a week or so.  It's unlikely I'll get to it sooner.
msg56986 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2007-10-31 05:06
Yeah, this seems like a bug and the patch seems correct.  The patch also
needs a test and add a space between if (.
msg59760 - (view) Author: Ralf Schmitt (schmir) Date: 2008-01-11 23:31
The return value should be -1 in case of errors. There's also a second
code path swallowing all errors.
I've converted brian's test.py to a unit test testing both code paths
and added an updated patch for this one.
This patch is against trunk.
All tests in Lib/test/test_isinstance.py pass.
msg66943 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2008-05-16 13:35
There are more locations in abstract.c where exceptions are
unconditionally masked after a GetAttr operation:

* line 1630, 1717 in PyNumber_{Long,Int} when looking for .__trunc__
* line 2932, in PyObject_IsInstance
msg66969 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-05-16 22:30
I'd like to clarify the approach to fixing these types of problems. I
fixed a issue (#2196) like this by only propagating exceptions that
didn't descend from Exception because of this sentence in the hasattr docs:

This is implemented by calling getattr(object, name) and seeing whether
it raises an exception or not.)

However, I like constricting it to AttributeError only as that would
make it much less confusing. This might be something to bring up on
python-dev.
msg67084 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2008-05-20 04:05
> I'd like to clarify the approach to fixing these types of problems.
> ...
> However, I like constricting it to AttributeError only as that would
> make it much less confusing. This might be something to bring up on
> python-dev.

I suspect that it might be on a case by case basis whether we want to
constrain more or less.  Bringing these cases up on python-dev should
lead to speedy decisions.
msg104471 - (view) Author: Tres Seaver (tseaver) * Date: 2010-04-28 22:13
I can confirm that the patch applies cleanly to the 2.6 branch, that the new test fails before rebuilding, and that the test passes afterwards:

 $ hg summary
 parent: 41597:295c02a21979 tip
  [svn r80597] Merged revisions 80596 via svnmerge from
 branch: release26-maint
 commit: (clean)
 update: (current)
 $ ./configure && make
 ...
 $ ./python -E -tt Lib/test/regrtest.py test_isinstance
 test_isinstance
 1 test OK.
 $ patch -p1 < /tmp/issue1574217_dont_mask_errors.patch
 patching file Lib/test/test_isinstance.py
 patching file Objects/abstract.c
 Hunk #1 succeeded at 2854 (offset 619 lines).
 Hunk #2 succeeded at 2878 (offset 600 lines).
 test_isinstance
 test test_isinstance failed -- Traceback (most recent call last):
   File "/home/tseaver/projects/hg-repo/issue1574217/Lib/test/test_isinstance.py", line 94, in test_isinstance_dont_mask_non_attribute_error
    self.assertRaises(RuntimeError, isinstance, c, bool)
 AssertionError: RuntimeError not raised
 
 1 test failed:
     test_isinstance
 $ make
 ...
 $ ./python -E -tt Lib/test/regrtest.py test_isinstance
 test_isinstance
 1 test OK.
msg110282 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-14 13:59
Can we get this closed as the patch is in 2.6 already, just needs to be applied to the later versions.  Or have I missed something?
msg110284 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-07-14 14:20
I don't see it being applied.  Tres Seaver above reports that issue1574217_dont_mask_errors.txt applies cleanly to 2.6 branch, which means that it has not been applied yet.

Someone needs to check if the patch is current for 2.7 and py3k branches to move this forward.
msg110289 - (view) Author: Tres Seaver (tseaver) * Date: 2010-07-14 15:10
This bug exists in Python 2.6 and 3.1, which are still being maintained, AFAIK.
msg110290 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-07-14 15:30
This looks like a borderline case between a bug and a feature request.  If this is deemed to be a feature, it is not appropriate for 2.x or 3.1.  In any case, I think the first step should be to consider this for py3k branch.
msg110292 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-07-14 15:37
I have verified that the issue is present in py3k.  The unit test portion of the patch applies cleanly in py3k an the added tests fail.  The code portion applies with patch -l and fixes the issue.  I'll upload a patch with fixed white space.
msg121664 - (view) Author: Graham Poulter (greppo) Date: 2010-11-20 15:28
issue1574217.diff still applies against py3k as revision 86545  (offset -8 lines)

patching file Objects/abstract.c
Hunk #1 succeeded at 2500 (offset -8 lines).
Hunk #2 succeeded at 2523 (offset -8 lines).

And tests pass (./python ./Lib/test/test_isinstance.py)
msg121679 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-11-20 16:40
I've committed this (with the whitespace fix) in r86577.  I've made myself a note to backport it when the maint branches unfreeze.
msg123775 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-12-11 04:03
Upon reflection I think the risk of breaking apparently working programs is higher than the benefit to be obtained from backporting this.
History
Date User Action Args
2010-12-11 04:03:51r.david.murraysetmessages: + msg123775
2010-11-20 16:40:40r.david.murraysetversions: - Python 2.6
2010-11-20 16:40:21r.david.murraysetstatus: open -> closed

assignee: nnorwitz ->

nosy: + r.david.murray
messages: + msg121679
resolution: fixed
stage: patch review -> resolved
2010-11-20 15:28:08grepposetnosy: + greppo
messages: + msg121664
2010-07-14 15:41:18belopolskysetfiles: + issue1574217.diff
2010-07-14 15:37:16belopolskysetmessages: + msg110292
2010-07-14 15:30:37belopolskysetmessages: + msg110290
2010-07-14 15:10:36tseaversetmessages: + msg110289
versions: + Python 2.6, Python 3.1
2010-07-14 14:20:51belopolskysetnosy: + belopolsky

messages: + msg110284
versions: - Python 2.6, Python 3.1
2010-07-14 13:59:17BreamoreBoysetnosy: + BreamoreBoy

messages: + msg110282
versions: + Python 3.2
2010-04-28 22:13:49tseaversetversions: + Python 2.6
2010-04-28 22:13:26tseaversetnosy: + tseaver
messages: + msg104471
2009-03-30 07:21:21ajaksu2setkeywords: + patch
stage: patch review
type: behavior
versions: + Python 3.1, Python 2.7, - Python 2.6, Python 2.5, Python 2.4
2008-05-20 04:05:59nnorwitzsetmessages: + msg67084
2008-05-16 22:30:28benjamin.petersonsetmessages: + msg66969
2008-05-16 13:35:25georg.brandlsetnosy: + benjamin.peterson, georg.brandl
messages: + msg66943
2008-01-11 23:31:37schmirsetfiles: + issue1574217_dont_mask_errors.txt
nosy: + schmir
messages: + msg59760
versions: + Python 2.6
2007-10-31 05:06:58nnorwitzsetmessages: + msg56986
2007-08-25 10:56:51ferringbsetversions: + Python 2.5
2006-10-10 02:55:53ferringbcreate