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

__init__() call from cython.declare and cython.cast in Shadow.py #3244

Merged
merged 2 commits into from Mar 25, 2020

Conversation

black-square
Copy link
Contributor

The following code:

# cython: infer_types=True
import cython

@cython.cclass
class Foo:
    a: cython.double
    b: cython.double
    c: cython.double

    def __init__(self, a: cython.double, b: cython.double ,c: cython.double):
        self.a = a
        self.b = b
        self.c = c
   
def bar():
    l = []
    l.append(Foo(10, 20, 30))
    
    v = cython.declare(Foo, l[0])
    r = v.a + v.b
    print( r )

    v2 = cython.cast(Foo, l[0]) #Faster - No __Pyx_TypeTest() call
    r = v2.b + v2.c
    print( r )
    
bar()

works fine when compiled and throws an exception when interpreted: TypeError: __init__() missing 2 required positional arguments: 'b' and 'c'

It could be fixed if we change implementations as shown in the patch. However, I don't think I understand the reason behind the current implementation to change it that way.

Should the issue be fixed and if not what's the reason behind that implementation?

@black-square
Copy link
Contributor Author

Failed unit tests helped me understand the reasons behind the implementation :)
I've fixed the errors and added more tests for the cases I was trying to fix

Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

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

Tests are great, the change looks nice, just one issue below.

Cython/Shadow.py Outdated
Comment on lines 187 to 197
elif (isinstance(t, type) and issubclass(t, (StructType, UnionType))) or isinstance(t, typedef):
return t()
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this change a bit questionable. I mean, it might be ok, it's just not obvious that this is the ultimate list of types that should be handled this way, and nothing else. I'd be happier if we could avoid introducing a regression here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scoder I totally understand your concern, but I don't see other solution which doesn't include the list of types. However, I don't feel like this solution is fragile as they are very special types. I'll try to explain my logic:

  1. This branch is used for declaring a variable without any initializations i.e. a = declare(cython.int)
  2. The result variable becomes uninitialized in case of simple types like int from C compiler perspective and it looks reasonable if it becomes None when interpreted
  3. In the case of the classes, the value None matches the generated C code
  4. The hardest case is structs, unions, and arrays. They are allocated on the stack in C and we cannot just return None. In such cases, we actually return the instance of the type witch exactly matches C code. That's why we need this check against the list of types
  5. The last case is the typedef. It is handled on the basis of the base_type (in the new commit which has been just added)

I've added more tests to verify everything. Please take a look and let me know what do you think. I'm totally flexible to change the implementation as long as all the new tests pass.

The following code:
```
# cython: infer_types=True
import cython

@cython.cclass
class Foo:
    a: cython.double
    b: cython.double
    c: cython.double

    def __init__(self, a: cython.double, b: cython.double ,c: cython.double):
        self.a = a
        self.b = b
        self.c = c

def bar():
    l = []
    l.append(Foo(10, 20, 30))

    v = cython.declare(Foo, l[0])
    r = v.a + v.b
    print( r )

    v2 = cython.cast(Foo, l[0]) #Faster - No __Pyx_TypeTest() call
    r = v2.b + v2.c
    print( r )

bar()
```
works fine when compiled and throws an exception when interpreted: `TypeError: __init__() missing 2 required positional arguments: 'b' and 'c'`

It could be fixed if we change implementations as shown in the patch.
Also, added more tests for the cases I'm trying to fix
NB: Removed execution of `test_declare(None)` to make sure that the new `none_declare()` test works instead. `test_declare(None)` doesn't throw exception in pure mode but does it in the native mode
@black-square
Copy link
Contributor Author

Rebased my changes to make it easy to apply them to any branch

@black-square black-square force-pushed the patch-1 branch 3 times, most recently from f672a59 to 0363c46 Compare March 23, 2020 07:26
…ranch broke the implementation and the tests because the construction was used to detect typedefs. To fix that I got rid of this check completely and replaced it to exact checks which also simplified the code

* Changed `declare` implementation when initializing arguments are not provided. Now it correctly works with typedefs of the user classes and also directly support arrays:
    ```
    >>> EmptyClassSyn = cython.typedef(EmptyClass)
    >>> cython.declare(EmptyClassSyn) is None
    True
    >>> cython.declare(cython.int[2]) is not None
    True
    ```
* Added missed return statement to `index_type` which made the following assigment possible:
    ```
        a = cython.declare(cython.int[2])
        a[0] = 1
    ```
@scoder
Copy link
Contributor

scoder commented Mar 25, 2020

Thanks

@scoder scoder merged commit 48dc1f0 into cython:master Mar 25, 2020
@da-woods
Copy link
Contributor

The following used to work but now doesn't:

def somefunc():
    x = cython.declare(cython.int[20], list(range(20)))

I think in the past it just returned the list, while now it's trying to call the array type defined in Shadow.py and failing to find a suitable constructor. I don't think this is unreasonable code but it isn't obvious to me what the correct fix is.

@black-square
Copy link
Contributor Author

@da-woods Fixed that in PR: #3465

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.

None yet

3 participants