Repository: cython/cython
Pull request: Don't error when exception_check is set to True when return type is PyObject.
This PR fixes issues in pure python mode when Python object is
annotated as returned value - e.g.:
```python
@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`:

https://github.com/cython/cython/blob/09cbf492dfcf1ad022e79660d6aec0be3f444244/Cython/Compiler/ParseTreeTransforms.py#L2508-L2510

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:
```cython
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)


Problem statement:
(@cython.cfunc + Type Annotation) Fragility
Going through some of pandas' cython modules to try to make code valid-python where possible, I'm seeing cythonize-time errors.  It seems like some combinations are invalid for surprising reasons, e.g:

```
cpdef object get_rule_month(object source, object default='DEC')
# existing, works as expected

cpdef object get_rule_month(source: object, default: object='DEC'):
# seems OK

cpdef get_rule_month(source: object, default: object='DEC') -> object:
# breaks --> Syntax error in C variable declaration

@cython.ccall
def get_rule_month(source: object, default: object='DEC') -> object:
# breaks --> Exception clause not allowed for function returning Python object

@cython.ccall
def object get_rule_month(source: object, default: object='DEC'):
# breaks --> Expected '(', found 'get_rule_month'

@cython.ccall
@cython.returns(object)
def get_rule_month(source: object, default: object='DEC'):
# appears OK
```

Another example with a slightly different set of errors:

```
cdef inline bint is_null_datetime64(v):
# existing, OK

cdef inline is_null_datetime64(v) -> bint:
# breaks --> Syntax error in C variable declaration

@cython.cfunc
def inline is_null_datetime64(v) -> bint:
# breaks --> Expected '(', found 'is_null_datetime64

@cython.cfunc
@cython.inline
def is_null_datetime64(v) -> bint:
# breaks --> Function signature does not match previous declaration
```

This last one here is the one that is noticeably different from the first example.

Is this behavior intentional?

Hints from issue discussion:
Thanks for that list (and for trying it in the first place). It means we clearly lack tests here, and it's good to have real-world feedback about where people encounter real-world problems.

Regarding the "obvious" cases:

> `def inline func()`

This can never work, because `def` requires Python syntax. Similarly, `cdef`/`cpdef` syntax cannot be combined with `->` return type annotations, which are a Python `def` function feature. While `cpdef` + Python syntax is a debatable edge case, I'd suggest that providing the return type like this should stay illegal for now. Either you use Python syntax (`def ... -> type`) or Cython syntax (`cpdef type ...`), not both. There are probably ways to give better error messages here (I created #2530 for that).

There is an `@cython.inline` decorator, so the (currently) only correct way to use `inline` in Python code is
```cython
@cython.cfunc
@cython.inline
def is_null_datetime64(v) -> bint:
```
There is a (similar) test for this in our test suite, so I don't know right now why this would fail. Probably something trivial.

https://github.com/cython/cython/blob/0fb73f66696e52f6614305d0ad6d42521fd128b0/tests/run/pure_py3.py#L68-L70

Similarly, `cpdef` should be expressed like this:
```cython
@cython.ccall
def get_rule_month(source: object, default: object='DEC') -> object:
```
which also fails for you, as you said. I remember noticing this kind of error, too, and fixing something in that corner recently. Could you try the latest master on that?
> There is a (similar) test for this in our test suite, so I don't know right now why this would fail. Probably something trivial.

Possibly relevant: there is an accompanying .pxd file that was unchanged throughout all these iterations with "cdef  bint is_null_datetime64(v)`

> I remember noticing this kind of error, too, and fixing something in that corner recently. Could you try the latest master on that?

Will do.  I'll also add to the list permutations involving `nogil` and `except? -1`

> Similarly, cdef/cpdef syntax cannot be combined with -> return type annotations, which are a Python def function feature.

Good to know, thanks.  My intuition was that the parser would translate each of the supported forms to a canonical representation, allowing users to mix and match (even though doing so would be really weird in some cases).  Not the first time intuition has led me astray.  (Can you point me towards the part of the code responsible for this?  If it doesn't require learning the entire code-base, I'd be happy to help with fleshing out the tests)

> While `cpdef` + Python syntax is a debatable edge case, I'd suggest that providing the return type like this should stay illegal for now. 

A use case I can imagine where this would be really convenient is linting (a large part of the motivation for trying this syntax in the first place).  With `cp?def` + Python syntax, we can probably cobble together something to pass `re.sub('^cp?def ', 'def', source).replace('cimport', 'import')` and be most of the way towards having something we can pass to `flake8`.
> there is an accompanying .pxd file that was unchanged throughout all these iterations with "cdef bint is_null_datetime64(v)`

Ah, yes, I'd be surprised if that didn't make a difference. But if you have a `.pxd` file with the types in it, why do you still need to define them in the actual source file?

> part of the code responsible for this

There are different aspects to this. One is the `AdjustDefByDirectives` transform, which adapts Python declarations to overrides provided in an external `.pxd` file, in decorators, or in annotations. Other tasks are done directly by certain syntax tree nodes, e.g. declaring a variable on an expression like `x: int`. Look for `annotation_typing` in the `Cython.Compiler` package.

Tests for this ended up in the pure Python mode tests (`pure_*.py`), in `cython3.pyx`, in `pep526_variable_annotations.py`, and in `annotation_typing.pyx`. The pure mode tests focus on anything cythonic that can run in Python, and the last one focusses on static typing with PEP 484. `.py` files are also tested in Python.

> `re.sub('^cp?def ', 'def', source).replace('cimport', 'import')`

For the first part, use `def` with decorators.

For the second, there isn't currently a way to use `cimport` in Python syntax. The only use case I can see is sharing declarations, and for that, it's probably best to require `.pxd` files – in which case you can get pretty far with having your implementation in `.py` files and [override](http://docs.cython.org/en/latest/src/tutorial/pure.html#augmenting-pxd) the Python declarations with C data types/functions/classes in the corresponding `.pxd` file.
> But if you have a .pxd file with the types in it, why do you still need to define them in the actual source file?

Not sure how this status quo developed; presumably easier for contributors/reviewers if they match.  

>> re.sub('^cp?def ', 'def', source).replace('cimport', 'import')
>
> For the first part, use def with decorators.
>
> For the second, there isn't currently a way to use cimport in Python syntax. 

This runs into a "if it were up to me..." issue where its hard to predict what maintainers will or won't have a strong opinion about.  Last time I tried to move to the decorator syntax the idea was shot down.  It's totally reasonable if you want to declare that Not Cython's Problem.

As for the `source.replace("cimport", "import")`, that's just a hack to trick a linter into think its valid python.  Feel free to ignore.

I'll install from master and update results in a bit.
Installed from master, no apparent change:

```
cpdef get_rule_month(source: object, default: object='DEC') -> object:
# still breaks --> Syntax error in C variable declaration

@cython.ccall
def get_rule_month(source: object, default: object='DEC') -> object:
# still breaks --> Exception clause not allowed for function returning Python object

cdef inline is_null_datetime64(v) -> bint:
# still breaks --> Syntax error in C variable declaration

@cython.cfunc
def inline is_null_datetime64(v) -> bint:
# still breaks --> Expected '(', found 'is_null_datetime64

@cython.cfunc
@cython.inline
def is_null_datetime64(v) -> bint:
# still breaks --> Function signature does not match previous declaration
```

Some more cases, these _without_ an accompanying pxd file:

```
@cython.cfunc
@cython.inline
def median_linear(a: float64_t*, n: int) -> float64_t nogil:
# breaks --> Expected an identifier or literal

@cython.cfunc
@cython.inline
@cython.nogil
def median_linear(a: float64_t*, n: int) -> float64_t:
# breaks --> Expected an identifier or literal

@cython.cfunc
@cython.inline
@cython.nogil
def median_linear(a: cython.pointer(float64_t), n: int) -> float64_t:
# breaks --> Invalid directive: 'nogil'.  (+1 on the error message)
```

So it looks like there exists no valid way to use type annotations on this function.

A weird thing a user might do is mix/match (here using type annotation only for the `labels` kwarg:
```
@cython.boundscheck(False)
@cython.wraparound(False)
def group_median_float64(ndarray[float64_t, ndim=2] out,
                         ndarray[int64_t] counts,
                         ndarray[float64_t, ndim=2] values,
                         labels: ndarray[int64_t],
                         Py_ssize_t min_count=-1):
# breaks --> 'int64_t' is not a constant, variable or function identifier
```

Of course the `ndim=2` bit isn't valid python (we're in the process of converting these to `foo[:, :]`), so trying to put that in the new format:

```
@cython.boundscheck(False)
@cython.wraparound(False)
def group_median_float64(out ndarray[float64_t, ndim=2],
                         counts: ndarray[int64_t],
                         values: ndarray[float64_t, ndim=2],
                         labels: ndarray[int64_t],
                         min_count: Py_ssize_t=-1):
# breaks --> Expected ']', found '='
```

But even with the memoryview syntax it chokes:

```
@cython.boundscheck(False)
@cython.wraparound(False)
def group_cumsum(out: numeric[:, :],
                 values: numeric[:, :],
                 labels: int64_t[:],
                 is_datetimelike,
                 skipna: bint=True):
# breaks --> Compiler crash in AnalyseDeclarationsTransform
```

Now one with an `except? -1` clause (some of these I now know will break, but listing them for thoroughness)

```
cdef inline get_datetime64_nanos(val: object) -> int64_t except? -1:
# Syntax error in C variable declaration

@cython.cfunc
@cython.inline
@cython.exceptval(-1)
def get_datetime64_nanos(val: object) -> int64_t:
# OK!  Even with an accompanying pxd file

cdef inline int64_t get_datetime64_nanos(val: object) except? -1:
# OK

@cython.inline
cdef int64_t get_datetime64_nanos(val: object) except? -1:
# OK

@cython.exceptval(-1)
cdef inline int64_t get_datetime64_nanos(val: object):
# breaks --> Function signature does not match previous declaration

@cython.exceptval(-1)
@cython.inline
cdef int64_t get_datetime64_nanos(val: object):
# breaks --> Function signature does not match previous declaration

@cython.inline
@cython.cfunc
def int64_t get_datetime64_nanos(val: object) except? -1:
# breaks --> Expected '(', found 'get_datetime64_nanos'

@cython.inline
@cython.cfunc
@cython.exceptval(-1)
def int64_t get_datetime64_nanos(val: object):
# breaks --> Expected '(', found 'get_datetime64_nanos'
```
Thanks for testing these.
```
cpdef get_rule_month(source: object, default: object='DEC') -> object:
# still breaks --> Syntax error in C variable declaration

@cython.cfunc
def inline is_null_datetime64(v) -> bint:
# still breaks --> Expected '(', found 'is_null_datetime64
```
I improved the error message for these.
```
@cython.cfunc
@cython.inline
def is_null_datetime64(v) -> bint:
# still breaks --> Function signature does not match previous declaration
```
**This is a bug and should work.**
```
@cython.cfunc
def inline is_null_datetime64(v) -> bint:
# still breaks --> Expected '(', found 'is_null_datetime64

@cython.cfunc
@cython.inline
def median_linear(a: float64_t*, n: int) -> float64_t nogil:
# breaks --> Expected an identifier or literal
```
These are not valid Python syntax (note that you say `def`) and are correctly rejected.
Using pointer types in Python syntax requires a typedef. Improving that error is difficult, because it would be perfectly ok to write `a: int * 5`, but `a: int*` is only … half of an expression.
```
@cython.cfunc
@cython.inline
@cython.nogil
def median_linear(a: cython.pointer(float64_t), n: int) -> float64_t:
# breaks --> Invalid directive: 'nogil'.  (+1 on the error message)
```
**This is a missing feature.** I created #2557 for it.
```
@cython.boundscheck(False)
@cython.wraparound(False)
def group_cumsum(out: numeric[:, :],
                 values: numeric[:, :],
                 labels: int64_t[:],
                 is_datetimelike,
                 skipna: bint=True):
# breaks --> Compiler crash in AnalyseDeclarationsTransform
```
**This is a bug and should work.** (fixed in 7bebbddd061fad77765cd77c6a9207b8bda17cc5)
```
@cython.inline
@cython.cfunc
def int64_t get_datetime64_nanos(val: object) except? -1:
# breaks --> Expected '(', found 'get_datetime64_nanos'
```
I improved the error message for this one.

I implemented support for memoryviews in type annotations.
Thanks, I'll give this a try on master and report back.
Trying out 0.29

```
@cython.cfunc
@cython.inline
def is_null_datetimelike(val : object) -> bint:
# breaks --> pandas/_libs/tslibs/nattype.pyx:627:0: Function signature does not match previous declaration
```

Tried changing this in the accompanying pxd to match

```
@cython.cfunc
def is_null_datetimelike(val : object) -> bint
# breaks --> pandas/_libs/tslibs/nattype.pxd:14:0: def statement not allowed here
```
I'm having the similar issues on cython V0.29 with return types in the .pyx file. Any python object return type breaks compilation in python compatible mode. Cython types work for returns (int, float, cython.int, cython.void, etc), np.ndarray and python builtins don't.

```
@cython.ccall
def get_object() -> object:  ...
# -> Exception clause not allowed for function returning Python object
```

```
@cython.cfunc
def get_str() -> str:  ...
# -> Exception clause not allowed for function returning Python object
```


Also returning None gives a different error:

```
@cython.ccall
def do_something_without_return() -> None:  ...
# -> Not a type
```

 which makes it not compatible with typed python even with just builtin types. If returning None is redundant, can cython just ignore it? or alias it to a cython void / NULL? (Should this be a separate issue?)

Commit message: Don't error when exception_check is set to True when return type is PyObject.

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.

Patch to review:
diff --git a/Cython/Compiler/Nodes.py b/Cython/Compiler/Nodes.py
index aa05a26b884..7ac7bef0843 100644
--- a/Cython/Compiler/Nodes.py
+++ b/Cython/Compiler/Nodes.py
@@ -715,7 +715,8 @@ def analyse(self, return_type, env, nonempty=0, directive_locals=None, visibilit
         if (return_type.is_pyobject
                 and (self.exception_value or self.exception_check)
                 and self.exception_check != '+'):
-            error(self.pos, "Exception clause not allowed for function returning Python object")
+            # Exception clause is silently ignored for functions returning Python object.
+            self.exception_check = False
         else:
             if self.exception_value is None and self.exception_check and self.exception_check != '+':
                 # Use an explicit exception return value to speed up exception checks.
diff --git a/tests/run/pure_py3.py b/tests/run/pure_py3.py
index b4728c4c694..55fa9867aec 100644
--- a/tests/run/pure_py3.py
+++ b/tests/run/pure_py3.py
@@ -85,3 +85,12 @@ def call_cdef_inline(x):
     """
     ret = cdef_inline(x)
     return ret, cython.typeof(ret)
+
+@cython.cfunc
+def test_cdef_return_object(x: object) -> object:
+    """
+    Test support of python object in annotations
+    >>> test_cdef_return_object(3)
+    3
+    """
+    return x


Code review comments:
diff_hunk: @@ -715,7 +715,8 @@ def analyse(self, return_type, env, nonempty=0, directive_locals=None, visibilit
         if (return_type.is_pyobject
                 and (self.exception_value or self.exception_check)
                 and self.exception_check != '+'):
-            error(self.pos, "Exception clause not allowed for function returning Python object")
+            # Exception clause is silently ignored for functions returning Python object.
+            self.exception_check = False
path: Cython/Compiler/Nodes.py
text: @user1:
Maybe we should only allow `except *` for functions returning PyObjects (which hopefully matches what gets added with the `cfunc` decorator). Thus _other_ exception statements (e.g. `except -1`) would still be an error?

@author:
Done. I would like to implement test for this case:
```python
@user2.exceptval(-1)
@user2.cfunc
def test_cdef_return_object_broken(x: object) -> object:
    """
    >>> test_cdef_return_object_broken(3)
    # Here I should somehow assert that error() was called.
    """
    return x
```
but I am not sure how I can assert following error:
```
=== Got: ===
105:0: Exception clause not allowed for function returning Python object
```

@user1:
The files to check for compile-time errors are in tests/errors. If you follow one of them it'll show you how to do it

@author:
Done.