Skip to content
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

Fix exception handling #844

Open
wants to merge 10 commits into
base: master
from
@@ -912,3 +912,78 @@ def test_try_multiple_except_else_finally(self):
print("Do final cleanup")
print('Done.')
""", exits_early=True)

def test_except_an_expr(self):
self.assertCodeExecution("""
f = lambda: ValueError
try:
raise ValueError("YO")
except f():
print('Got a dynamic exception')
else:
print("Should do not print this")
finally:
print("Do final cleanup")
print('Done.')
""")

self.assertCodeExecution("""
f = lambda: ValueError
try:
raise ValueError
except f() as e:
print('Got a dynamic exception', e)
else:
print("Should do not print this")
finally:
print("Do final cleanup")
print('Done.')
""")

self.assertCodeExecution("""
f = lambda: ValueError
try:
raise TypeError("YO")
except f():
print("Should not print this")
else:
print("Do else handling")
finally:
print("Do final cleanup")
print('Done.')
""", exits_early=True)

self.assertCodeExecution("""
x = ValueError
try:
raise ValueError
except x as e:
print('Got a dynamic exception', e)
else:
print("Should do not print this")
finally:
print("Do final cleanup")
print('Done.')
""")

@expectedFailure
def test_except_tuple_custom_exception(self):
self.assertCodeExecution("""
class MyException(Exception):
pass
try:
raise MyException
except (MyException,):
print("Got a custom exception")
print('Done.')
""")

self.assertCodeExecution("""

This comment has been minimized.

Copy link
@eliasdorneles

eliasdorneles Sep 7, 2018

Collaborator

Can you please put this test in a separate method, so that it can run as well?

This seems to be a regression (i.e., if i'm not mistaken, this second assertCodeExecution passes on master)

This comment has been minimized.

Copy link
@vincent-prz

vincent-prz Oct 13, 2018

Author

Done. Indeed, there seems to be a regression on master. But it seems it's the other way around: the second assertCodeExecution passes, while the first fails.

Do you think it's blocking the merge?

This comment has been minimized.

Copy link
@eliasdorneles

eliasdorneles Oct 15, 2018

Collaborator

Thanks!
Yeah, that's what I said -- the second passes. :)

So, while the patch seems to indeed already make things better, I would like to have a look with more patience to see what's causing that regression.
I'm a bit swamped lately but I'll have some days off soon and will look into it!

This comment has been minimized.

Copy link
@eliasdorneles

eliasdorneles Oct 15, 2018

Collaborator

I noticed that there are some tests failing related to the time module -- can you please sync w/ master?
Thank you!

class MyException(Exception):
pass
try:
raise MyException
except (Exception,):
print("Got a custom exception")
print('Done.')
""")
@@ -1,7 +1,6 @@
import ast
import copy
import sys
import traceback

from ..java import opcodes as JavaOpcodes
from .modules import Module
@@ -1796,7 +1795,7 @@ def visit_Yield(self, node):
'Lorg/python/Object;'
)
)

yield_point = len(self.context.yield_points) + 1

# Save the current stack and yield index
@@ -2474,9 +2473,128 @@ def visit_ExtSlice(self, node):
def visit_Index(self, node):
self.visit(node.value)

def _should_visit_core_exception(self, node):
builtin_exceptions = [
"ArithmeticError",
"AssertionError",
"AttributeError",
"BaseException",
"BlockingIOError",
"BrokenPipeError",
"BufferError",
"BytesWarning",
"ChildProcessError",
"ConnectionAbortedError",
"ConnectionError",
"ConnectionRefusedError",
"ConnectionResetError",
"DeprecationWarning",
"EOFError",
"Exception",
"FileExistsError",
"FileNotFoundError",
"FloatingPointError",
"FutureWarning",
"GeneratorExit",
"ImportError",
"ImportWarning",
"IndentationError",
"IndexError",
"InterruptedError",
"IsADirectoryError",
"KeyError",
"KeyboardInterrupt",
"LookupError",
"MemoryError",
"NameError",
"NotADirectoryError",
"NotImplementedError",
"OSError",
"OverflowError",
"PendingDeprecationWarning",
"PermissionError",
"ProcessLookupError",
"ReferenceError",
"ResourceWarning",
"RuntimeError",
"RuntimeWarning",
"StandardError",
"StopIteration",
"SyntaxError",
"SyntaxWarning",
"SystemError",
"SystemExit",
"TabError",
"TimeoutError",
"TypeError",
"UnboundLocalError",
"UnicodeDecodeError",
"UnicodeEncodeError",
"UnicodeError",
"UnicodeTranslateError",
"UnicodeWarning",
"UserWarning",
"ValueError",
"Warning",
"ZeroDivisionError",
]
return node.type is None or (
isinstance(node.type, ast.Name) and
(node.type.id in builtin_exceptions or
node.type.id in self.symbol_namespace)
)

@node_visitor
# [FIXME] - make it work for Tuples

This comment has been minimized.

Copy link
@eliasdorneles

eliasdorneles Sep 7, 2018

Collaborator

leftover to remove?

def visit_ExceptHandler(self, node):
# expr? type, identifier? name, stmt* body):
if self._should_visit_core_exception(node):
self._visitCoreException(node)
else:
exception = self.full_classref('BaseException', default_prefix='org.python.exceptions')
self.context.add_opcodes(
CATCH(exception),
)
self.context.add_opcodes(
JavaOpcodes.DUP()
)
self.visit(node.type)
self.context.add_opcodes(
JavaOpcodes.INVOKESTATIC(
'org/Python',
'isinstance',
args=[
'Lorg/python/Object;',
'Lorg/python/Object;',
],
returns='Lorg/python/types/Bool;'
)
)
self.context.add_opcodes(
IF([python.Object.as_boolean()], JavaOpcodes.IFEQ),
)
if node.name:
# The exception has been named. Store it as that name.
self.context.store_name(node.name)
else:
self.context.add_opcodes(
JavaOpcodes.POP()
)

for child in node.body:
self.visit(child)
self.context.add_opcodes(
ELSE()
)
self.context.add_opcodes(
JavaOpcodes.CHECKCAST('java/lang/Throwable'),
JavaOpcodes.ATHROW(),
)
self.context.add_opcodes(
END_IF(),
)

def _visitCoreException(self, node):
if isinstance(node.type, ast.Tuple):
exception = [
self.full_classref(exc.id, default_prefix='org.python.exceptions')
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.