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

Don't error when exception_check is set to True when return type is PyObject. #4433

Merged
merged 5 commits into from
Dec 14, 2021

Conversation

matusvalo
Copy link
Contributor

This PR fixes issues in pure python mode when Python object is
annotated as returned value - e.g.:

@cython.cfunc
def foo(s) -> str:

It raises following exception: Exception clause not allowed for function returning Python object.
This exception is raised because Pure python mode defaults exception_check to True:

# for Python annotations, prefer safe exception handling by default
if return_type_node is not None and except_val is None:
except_val = (None, True) # except *

I have tried to fix the issue in this place by checking of returned type but I have found it challenging because
during this stage of compilation we don't have annotation parsed. Hence, I decided to ignore the error and just reset
the exception_check value because:

Cython language is ignoring exception_check when python object is
returned - e.g. following code is compiled successfully:

cdef object ftang(object i) except *:
    return 5

Hence the pure python mode will have the same behavior as cython language. Moreover I was not able to raise the error in cython language.

Fixes #2529 (If I read comments in the issue correctly all other cases were or fixed or other issues were created)

…yObject.

Cython language is ignoring exception_check when python object is
returned. On the other hand this error occurs problems in pure python
mode when PyObject is annotated as return type. Instead raising error
just reset exception_check to False.
Copy link
Contributor

@0dminnimda 0dminnimda left a comment

Choose a reason for hiding this comment

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

  1. As stated in [ENH] C functions should propagate exceptions by default #4280 we want cdef / cpdef to propagate exceptions by defaut and not otherwise for @cfunc / @ccall
  2. If I understand correctly, with this pr cython it will overwrite the exception_check behavior even in case if we specified it explicitly and that's not the best user experience
@cython.cfunc
@cython.exceptval(check=True)
def foo(s) -> str:

@matusvalo
Copy link
Contributor Author

As stated in #4280 we want cdef / cpdef to propagate exceptions by default and not otherwise for @cfunc / @CCall

This PR is still keeping this. Exceptions are propagated by default. It only do not error out when object is returned.

If I understand correctly, with this pr cython it will overwrite the exception_check behaviour even in case if we specified it explicitly and that's not the best user experience

As mentioned in description of PR, this behaviour is present in cython language:

cdef object ftang(object i) except *:

example above in cython is equivalent to your example and still is compiled correctly with no error (hence the except * is ignored. So this PR is unifying pure python and cython semantics.

tests/run/pure_py3.py Outdated Show resolved Hide resolved
Cython/Compiler/Nodes.py Outdated Show resolved Hide resolved
@scoder scoder added this to the 3.0 milestone Oct 31, 2021
@matusvalo
Copy link
Contributor Author

Based on message of last commit I have checked whether we can avoid setting self.exception_check to False:

        elif return_type.is_pyobject and self.exception_check:
            pass

        if (return_type.is_pyobject
                and (self.exception_value)
                and self.exception_check != '+'):
            error(self.pos, "Exception clause not allowed for function returning Python object")

Unfortunately, with this change Cython compiler crashes for pure_py3 test:

ERROR: runTest (__main__.CythonRunTestCase)
compiling (c/cy2) and running pure_py3
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ness-skmac3/dev/cython/runtests.py", line 1449, in run
    ext_so_path = self.runCompileTest()
  File "/Users/ness-skmac3/dev/cython/runtests.py", line 1092, in runCompileTest
    return self.compile(
  File "/Users/ness-skmac3/dev/cython/runtests.py", line 1370, in compile
    self._match_output(expected_errors, errors, tostderr)
  File "/Users/ness-skmac3/dev/cython/runtests.py", line 1422, in _match_output
    self.assertEqual(None, unexpected)
AssertionError: None != '89:0: Compiler crash in AnalyseDeclarationsTransform'

Hence, I suppose we need to leave it there. @scoder is there anything in this PR to be done?

@da-woods
Copy link
Contributor

da-woods commented Nov 6, 2021

You may be able to get to the bottom of the compiler crash by compiling it outside the test runner. Usually python3 cythonize.py tests/run/pure_py.py (maybe with -2 as well since most of the tests use this by default). That should give you a more extended traceback.

I don't think there's a good reason to remove the self.exception_check = False though. I think the message from the last commit was about avoiding testing self.exception_check with the is operator rather than removing the assignment.

@scoder scoder merged commit 6c80233 into cython:master Dec 14, 2021
@matusvalo matusvalo deleted the exception_check_fix branch December 14, 2021 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(@cython.cfunc + Type Annotation) Fragility
4 participants