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

[BUG] not None semantics is not supported in Pure Python mode for memory views #5612

Closed
matusvalo opened this issue Aug 11, 2023 · 5 comments · Fixed by #5622
Closed

[BUG] not None semantics is not supported in Pure Python mode for memory views #5612

matusvalo opened this issue Aug 11, 2023 · 5 comments · Fixed by #5622

Comments

@matusvalo
Copy link
Contributor

matusvalo commented Aug 11, 2023

Describe the bug

Pure python mode does not support not None when used with memory views. See examples below.

Code to reproduce the behaviour:

import cython
import typing
import numpy as np

def process_buffer(input_view: cython.int[:,:],
                   output_view: typing.Optional[cython.int[:,:]] = None):

    if output_view is None:
        # Creating a default view, e.g.
        output_view = np.empty_like(input_view)
    
    # process 'input_view' into 'output_view'
    return output_view

process_buffer(None, None)

Note
There is additional issue that memoryviews does not support typing.Optional(). Compiling the example fails with error: typing.Optional[...] cannot be applied to non-Python type int[:]

Expected behaviour

TypeError is raised in the same way as it is raised in cython version:

import numpy as np

def process_buffer(int[:,:] input_view not None,
                   int[:,:] output_view=None):

   if output_view is None:
       # Creating a default view, e.g.
       output_view = np.empty_like(input_view)

   # process 'input_view' into 'output_view'
   return output_view


process_buffer(None, None)

Also typing.Optional[] of memoryview succeeds without error.

OS

any

Python version

3.9

Cython version

master

Additional context

Similar semantics is already present in Extension types: https://cython.readthedocs.io/en/latest/src/userguide/extension_types.html#extension-types-and-none

@da-woods
Copy link
Contributor

The difficulty is backwards compatibility (i.e. Cython 3 was probably the time to make this change is we wanted to). I think we'd probably have to introduce a cython.NonOptional decorator (name to be debated...) rather than change the behaviour of existing annotated memoryviews.

We should definitely make sure typing.Optional[cython.int[:]] is accepted (if it isn't already)

Much as it'd be nice to make it consistent.

@scoder
Copy link
Contributor

scoder commented Aug 12, 2023 via email

@matusvalo
Copy link
Contributor Author

OK I have created PoC PR fixing this issue. Not tested yet. I will wait for final decision whether we will go with it to 3.0.1 (as @scoder suggested) or not.

@scoder
Copy link
Contributor

scoder commented Aug 15, 2023

On a loosely related note, should we allow Optional[cpointer] as well, as a no-op? It seems reasonable to me to allow explicitly marking NULL as a valid value for pointers. I wouldn't want to reject NULL if Optional is not used, but IMHO, there's nothing wrong with an explicit Optional[cpointer].

Or do you think that we should limit the applicable range of Optional to "things that can actually be None"?

@matusvalo
Copy link
Contributor Author

On a loosely related note, should we allow Optional[cpointer] as well, as a no-op? It seems reasonable to me to allow explicitly marking NULL as a valid value for pointers. I wouldn't want to reject NULL if Optional is not used, but IMHO, there's nothing wrong with an explicit Optional[cpointer].

I don't have strong opinion about this. But I would rather do it later not in this PR or 3.0.1 release (to keep it light). This change is not breaking anything so can be added anytime.

@scoder scoder added this to the 3.0.1 milestone Aug 22, 2023
scoder pushed a commit that referenced this issue Aug 23, 2023
…ng `None` without it (GH-5622)

This is a backwards incompatible change that matches the intention of Cython 3.0 to make argument values safer.

Closes #5612
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants