New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Attempting to use class with both __enter__ & __exit__ undefined yields __exit__ attribute error #71287
Comments
Attempting to use class that has both __exit__() and __enter__() undefined as a context manager yields an AttributeError referring to __exit__: >>> class A():
... pass
...
>>> with A():
... pass
...
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: A instance has no attribute '__exit__' Knowing that the 'with' statement should first execute __enter__, this is unexpected. The patch ensures the attribute error refers to __enter__() when both methods are undefined. |
This seems to be designed. From PEP-343 it tells clearly:
But currently I don't find out why. |
Nick, I've seen the come-up several times with people learning about context managers. The current error message seems cause confusion because it checks for __exit__ before __enter__, implying that the __enter__ is optional which it isn't. Do you have any objections testing for __enter__ first? |
I don't completely understand. If you get an AttributeError for __enter__, will you think only an __enter__ is required? |
As Jonathan notes, the cause for confusion here is that we currently retrieve the attributes in the opposite order from the way we execute them, prompting the "Why is it looking for __exit__ first?", "Oh, __enter__ must be optional, so it didn't care that it was missing" train of thought. If I recall correctly, rather than stemming from any particular design principle, the current behaviour is an artifact of the initial implementation that didn't use any dedicated opcodes: since CPython uses a stack-based VM, we needed __enter__ to be above __exit__ on the stack, and the most efficient way to achieve that was to do the attribute lookups in the reverse order of invocation. When SETUP_WITH was introduced, that original method lookup ordering (and associated error message precedence) was preserved, even though the technical rationale for that ordering no longer applied. Given that, I don't see any problem with switching the lookup order now, and I agree that should be less confusing, given that the most obvious explanation for getting an error message about __enter__ when both are missing is that the interpreter tries to call __enter__ before it tries to do anything with __exit__. (That intuitive explanation still isn't 100% accurate, but it's closer than thinking __enter__ is optional) |
Thanks Nick. I concur as well and will apply this. Would it be reasonable to apply this to 3.6? IMO, it isn't a new feature, it is more of a usability bug due to an oversight. |
+1 for 3.6 from me - as you say, this is fixing a usability bug in the error handling rather than being a new feature. |
Alternatively, SETUP_WITH could additionally check for the other method, and raise an AttributeError with a custom message mentioning both methods. |
Due to the expected semantics of AttributeError being relatively precise, changing to a protocol check rather than just changing the order we look up the protocol attributes would also involving changing from AttributeError to TypeError. Since that's a potentially more disruptive change and just changing the order of the checks is likely sufficient to reduce confusion, the latter approach is preferable, at least for now (if folks are still getting confused by the time 3.7 rolls around, then that option would be worth reconsidering). |
New changeset 3aafb232f2db by Raymond Hettinger in branch '3.6': |
Thanks for the patch Jonathan. Nick, thanks for chiming in. |
https://mail.python.org/pipermail/python-checkins/2016-November/147171.html test_collections leaked [0, 0, 7] memory blocks, sum=7 Command line was: ['./python', '-m', 'test.regrtest', '-uall', '-R', '3:3:/home/psf-users/antoine/refleaks/reflogWMjqsJ', '--timeout', '7200'] Looking at the code change, it looks like you aren’t releasing __enter__ if the __exit__ lookup fails. A separate question: why not swap the “async with” __aenter__ and __aexit__ code at the same time? |
Hmm, it seems I already discovered this leak six months ago: https://bugs.python.org/review/27100 |
Serhiy, do you care to double check this patch? Martin, I'll leave the async_with for Yuri to fix-up. I believe that is his code, so he should do the honors. |
The patch LGTM. |
New changeset 749c5d6c4ba5 by Raymond Hettinger in branch '3.6': |
Yuri, do you want to do the same for async-with? |
The tests changes also produce a DeprecationWarning: ====================================================================== Traceback (most recent call last):
File "/home/proj/python/cpython/Lib/test/test_with.py", line 120, in testEnterAttributeError1
self.assertRaisesRegexp(AttributeError, '__enter__', fooLacksEnter)
File "/home/proj/python/cpython/Lib/unittest/case.py", line 1311, in deprecated_func
DeprecationWarning, 2)
DeprecationWarning: Please use assertRaisesRegex instead. The fix is simple and just affects the tests, but since most people don’t seem to worry about warnings, I guess this can wait for 3.6 to be released before committing the fix. |
Oh, how I missed this? I usually run tests with -Wa. Since the patch is simple and fixes a regression introduced just before releasing beta 4, I'm fine with fixing it. We are trying to support tests warnings-free. Some buildbots can run tests with -We. |
+1 |
New changeset e11df6aa5bf1 by Raymond Hettinger in branch '3.6': |
Can this be closed now? |
Reviewing the discussion, I assume this was left open to cover reordering the __aenter__ and __aexit__ checks for async with, but that can just as easily be handled as a separate issue (which would also be clearer at the NEWS level). |
Here it is: https://bugs.python.org/issue39048 |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: