Adding minimal support for Cython functions #8631

pbotros · 2020-04-04T18:48:36Z

Minimal support for Cython functions in stubgen for generating .pyi
files from a C module. Based on #7542.

@ilevkivskyi || @JukkaL (or others): PTAL? First time contributing to Mypy. Thanks!

pbotros · 2020-04-04T18:53:12Z

If people are curious what the full .pyi is that is outputted from the test, it's:

from typing import Any, List

import typing

def __pyx_unpickle_MyClass(__pyx_type, __pyx_checksum, __pyx_state) -> Any: ...
def f(path: str, a: int = ..., b: bool = ...) -> typing.List[str]: ...

class MyClass:
    @classmethod
    def __init__(self, *args, **kwargs) -> None: ...
    def run(self, action: str) -> None: ...
    def __reduce__(self) -> Any: ...
    def __reduce_cython__(self) -> Any: ...
    def __setstate__(self, state) -> Any: ...
    def __setstate_cython__(self, __pyx_state) -> Any: ...

JukkaL · 2020-04-08T16:59:13Z

Thanks for the PR! Cython support for stubgen sounds like a useful thing.

Before continuing with the code review, could you fix the CI failures? If something is unclear, feel free to ask here.

Minimal support for Cython functions in stubgen for generating .pyi files from a C module. Based on python#7542.

pbotros · 2020-04-19T04:08:55Z

@JukkaL CI failures are now fixed; take a look if you have a chance. Thanks!

pbotros · 2020-06-01T04:35:28Z

ping @JukkaL @dgrunwald ?

JukkaL

Thanks for the PR! Mostly looks good. Left some comments about testing and questions about PEP 484 type support in Cython.

Better Cython support would be quite useful.

JukkaL · 2020-06-08T14:37:09Z

test-requirements.txt

@@ -15,3 +15,4 @@ py>=1.5.2
 virtualenv<20
 setuptools
 importlib-metadata==0.20
+Cython


I'd rather not add Cython as a dependency. Instead, it would be better to have the test automatically skipped if Cython is not installed.

JukkaL · 2020-06-08T14:40:33Z

mypy/test/teststubgen.py

+def f(path: str, a: int = ..., b: bool = ...) -> typing.List[str]: ...
+        """.strip() in outfile_txt or """
+def f(path: str, a: int = ..., b: bool = ...) -> List: ...
+        """.strip() in outfile_txt


Why are there two signatures that we accept?

This check was a bit confusing. I think that it would be easier to read if it was written along these lines:

sig1 = """...""" sig2 = """...""" assert sig1.strip() in outfile_txt or sig2.strip() in outfile_txt

(Also it would look nicer with textwrap.dedent().)

JukkaL · 2020-06-08T14:41:53Z

mypy/test/teststubgen.py

@@ -804,6 +809,52 @@ def __init__(self, arg0: str) -> None:
            'def __init__(*args, **kwargs) -> Any: ...'])
        assert_equal(set(imports), {'from typing import overload'})

+    def test_cython(self) -> None:
+        pyx_source = """


Style nit: What about indenting the source and using textwrap.dedent() to dedent it?

JukkaL · 2020-06-08T14:42:59Z

mypy/stubgenc.py

+        if hasattr(annotation, '__str__'):
+            return annotation.__str__()
+        # Can't do anything here, so ignore
+        return None


I wonder what sorts of things Cython might add to the signature. Have you tried that this does something reasonable with all types supported by Cython? It would be good to include all relevant sorts of types in the test case, in case something isn't covered yet.

JukkaL · 2020-06-08T14:46:11Z

mypy/test/teststubgen.py

+
+import typing
+
+def f(path: str, a: int = 0, b: bool = True) -> typing.List[str]:


I have some questions about how Cython supports PEP 484 types:

What if some argument has no type annotation?

Do you know how Cython supports optional types?

What would happen if this function would return None (with a list return type)?

What if f is called with a None argument for path, for example?

(If Cython doesn't properly support optional types, we may not be able to use the types in stubs, at least not without some tweaks.)

What if some argument has no type annotation?

Then it's an object.

What would happen if this function would return None (with a list return type)?

That's ok currently. Not sure if it's going to stay that way forever, given that optional types exist, but it's at least unlikely to change all too soon.

What if f is called with a None argument for path, for example?

That's currently ok, too. None is generally allowed for builtin and extension types, unless explicitly excluded. That's probably also going to change at some point.

If Cython doesn't properly support optional types

Cython currently only uses a subset of what PEP-484 allows, specifically: no container types, no optional types. Nothing that requires a [] or the typing module, basically. That's a missing feature, though, and will change at some point.

Maybe Cython should just implement Optional[] and apply not None semantics to types that use PEP-484 annotations. Probably just a little change in the type parser (if it wasn't for the typing import tracking, but we could ignore that, I guess).

If Cython doesn't yet ensure that optional types are used consistently, we can't easily know whether an argument should allow None values in a stub, and whether a function might return None. A conservative approximation would be to always use an optional type for an argument, and use Union[<type>, Any] for the return type. The latter is the best way we have of saying that a function returns <type> and potentially None (but we don't know for sure). These are still better than using plain Any or object as the types.

Also, we don't usually want to use object as a return type, and it will usually result in many false positives. Instead, it should probably be replaced with Any.

Maybe Cython should just implement Optional[] and apply not None semantics to types that use PEP-484 annotations.

This would be great, since optional types tend to be used a lot in PEP 484 annotated code. Mypy also didn't properly support optional types in the early days (and we still allow disabling strict None checks, but this is mostly for legacy code).

Also, we don't usually want to use object as a return type, and it will usually result in many false positives. Instead, it should probably be replaced with Any.

For regular functions I don't think Cython enforces anything about the return type at all. Therefore, from a Cython point of view it should probably always be treated as Any (although I guess you might chose to trust the user and use their annotation?).

For cdef functions is does have specified return types, but those are only visible from C anyway so probably not relevant to mypy.

For regular functions …

You probably mean plain Python def functions here. For cpdef / ccall functions (and cdef / cfunc, obviously), i.e. anything that has a C visible return type, the return types are enforced and checked like for an assignment.

I agree that mypy should believe the user annotations, with the above caveat regarding None.

scoder · 2020-06-14T07:58:38Z

mypy/stubgenc.py

-    return inspect.isbuiltin(obj) or type(obj) is type(ord)
+    return inspect.isbuiltin(obj) or \
+           type(obj) is type(ord) or \
+           type(obj).__name__ == 'cython_function_or_method'


This is probably not going to work forever.

Why do these need to be processed as C functions rather than Python functions? I'd just use inspect on the object and see if that produces something helpful. That wouldn't only work for Cython functions but also for Python functions in .pyc files etc. (don't know how mypy handles those currently).

I don't know if there's any particular reason. If inspect works, we should perhaps use it?

As long as Cython doesn't enforce the use of optional types, it would at least be useful to be able to detect which functions or modules were compiled by Cython, and generate wider types (as I described above) for them.

scoder · 2020-06-14T08:15:37Z

mypy/test/teststubgen.py

+
+import typing
+
+def f(path: str, a: int = 0, b: bool = True) -> typing.List[str]:


What if some argument has no type annotation?

Then it's an object.

What would happen if this function would return None (with a list return type)?

That's ok currently. Not sure if it's going to stay that way forever, given that optional types exist, but it's at least unlikely to change all too soon.

What if f is called with a None argument for path, for example?

That's currently ok, too. None is generally allowed for builtin and extension types, unless explicitly excluded. That's probably also going to change at some point.

If Cython doesn't properly support optional types

Cython currently only uses a subset of what PEP-484 allows, specifically: no container types, no optional types. Nothing that requires a [] or the typing module, basically. That's a missing feature, though, and will change at some point.

Maybe Cython should just implement Optional[] and apply not None semantics to types that use PEP-484 annotations. Probably just a little change in the type parser (if it wasn't for the typing import tracking, but we could ignore that, I guess).

msullivan

Jukka had some questions/feedback

hauntsaninja · 2023-04-26T05:25:06Z

Closing, since this has been stale for a long time.

pbotros mentioned this pull request Apr 4, 2020

stubgen and Cython modules #7542

Open

pbotros force-pushed the botros/cython-support branch from e29eeaf to 139c80f Compare April 4, 2020 18:52

pbotros force-pushed the botros/cython-support branch 3 times, most recently from 4d3dd92 to 35fc092 Compare April 5, 2020 02:28

pbotros force-pushed the botros/cython-support branch 5 times, most recently from 8681f14 to c00e677 Compare April 19, 2020 03:21

Adding minimal support for Cython functions

7e732c4

Minimal support for Cython functions in stubgen for generating .pyi files from a C module. Based on python#7542.

pbotros force-pushed the botros/cython-support branch from c00e677 to 7e732c4 Compare April 19, 2020 03:45

alippai mentioned this pull request May 19, 2020

ARROW-8175: [Python] Setup type checking with mypy apache/arrow#6676

Closed

JukkaL reviewed Jun 8, 2020

View reviewed changes

scoder reviewed Jun 14, 2020

View reviewed changes

msullivan requested changes Oct 18, 2020

View reviewed changes

marcelm mentioned this pull request Apr 23, 2021

Start adding type hints pysam-developers/pysam#1008

Merged

hauntsaninja closed this Apr 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding minimal support for Cython functions #8631

Adding minimal support for Cython functions #8631

pbotros commented Apr 4, 2020

pbotros commented Apr 4, 2020

JukkaL commented Apr 8, 2020

pbotros commented Apr 19, 2020

pbotros commented Jun 1, 2020

JukkaL left a comment

JukkaL Jun 8, 2020

JukkaL Jun 8, 2020

JukkaL Jun 8, 2020

JukkaL Jun 8, 2020

JukkaL Jun 8, 2020

scoder Jun 14, 2020

JukkaL Jun 19, 2020

da-woods Jun 19, 2020

scoder Jun 19, 2020

scoder Jun 14, 2020

JukkaL Jun 19, 2020

scoder Jun 14, 2020

msullivan left a comment

hauntsaninja commented Apr 26, 2023


		import typing

		def f(path: str, a: int = 0, b: bool = True) -> typing.List[str]:

Adding minimal support for Cython functions #8631

Adding minimal support for Cython functions #8631

Conversation

pbotros commented Apr 4, 2020

pbotros commented Apr 4, 2020

JukkaL commented Apr 8, 2020

pbotros commented Apr 19, 2020

pbotros commented Jun 1, 2020

JukkaL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msullivan left a comment

Choose a reason for hiding this comment

hauntsaninja commented Apr 26, 2023