ENH: allow @property decorator on external ctypedef classes #2640

mattip · 2018-10-02T08:37:15Z

Following up the discussion on the mailing list. I got to the point where I can use the new syntax:

Write a srctree test that creates various types of external ctypedef classes, add one that overrides a python-level @Property with a cython one. This is a continuation of the test added in DOC, TST: test and document external extension type attribute aliasing #2629
Add decorator child and outer attributes to CFuncDefNode. ~~I simply copied this from DefNode, is it required?~~ - yes, seems to be required
~~Add a DecoratorTransform.visit_CFuncDefNode function based on DecoratorTransform.visit_DefNode and verify it is reached while parsing the ctypedef class.~~
~~Found the point at which the attribute lookup should become a property function call (I think) is in AttributeNode.analyse_attribute. But it seems the entry is wrong, I need to either~~
~~fix the entry in PropertyNode.analyse_declarations and somehow make sure it is used instead of the old entry~~
~~change the code in CFuncDefNode.analyse_declarations and remove the DecoratorTransform.visit_CFuncDefNode function~~

Any hints would be welcome.

Edit: added a new pass, ~~still not working~~

Cython/Compiler/Nodes.py

mattip · 2018-10-29T16:46:05Z

xref #2498 and numpy/numpy#11803

Cython/Compiler/Nodes.py

Cython/Compiler/ParseTreeTransforms.py

tests/run/ext_attr_getter.srctree

mattip · 2018-11-09T02:47:35Z

I added a ReplacePropertyNode pass and verified that the CFuncDefNode was changed to a SimpleCallNode. However it seems the pass is too late or I need to propogate the changes further.

The AddNode from "f.field0 + f.field1" has operands that are AttributeNodes, and those contain the old CFuncDefNodes. In which pass are the AttributeNodes updated or how can I trigger re-evaluating them?

mattip · 2018-11-09T07:45:08Z

It seems the "magic" is in getting from the CDefExternNode.stats to the CClassScope.entries, there the field0 is an Entry with an almost-correct but slightly incorrect type. So when the AddNode looks up its operands (in AttributeNode.analyse_attribute), the AttributeNode uses the field0 entry, and fails.

mattip · 2018-11-09T08:22:16Z

The CClassScope.cfunc_entries are first set in the AnalyseDeclarationsTransform, which was where I modified the entries in commit 726ebd8 (the first attempt in this PR). After I modify the entries in a later pass, how do I retrigger analyse_declarations for this node?

mattip · 2018-11-14T03:26:48Z

Rebased off master to get the check_size option.

I needed to add a property kwarg to add_cfunction so that the entry ended up on the property_entries not the cfunc_entries, which is determined during the AnalyseDeclarations pass. Entries in the cfunc_entries end up on the vfunctab, and in the test I create a class with no class methods so there is no vfunctab.

Other than that, I added a pass to mark the function entry that it is a cgetter function. The AttributeNode handles this and outputs the appropriate code.

mattip · 2018-11-14T03:27:59Z

btw, the no-capture option PR #2701 was essential in allowing me to debug the various passes in a strctree test

mattip · 2018-11-16T02:43:05Z

tests pass

mattip · 2018-11-26T17:26:14Z

Is there something more I should do to move this forward?

scoder

Sorry for the (end of year) silence. There seem to be some problems with the implementation that I commented on.

scoder · 2019-01-02T14:12:26Z

Cython/Compiler/Nodes.py

+        property = False
+        if self.decorators:
+            for _node in self.decorators:
+                if _node.decorator.is_name and _node.decorator.name == 'property':


The repetition of this looping code seems to scream for a helper function, e.g. one that searches for a specific decorator (and maybe applies some safety checks internally).

Merged with decorator analysis at the beginning of the function. Did you mean that I should refactor that into a helper function?

Cython/Compiler/ExprNodes.py

Cython/Compiler/Symtab.py

tests/run/ext_attr_getter.srctree

Cython/Compiler/ParseTreeTransforms.py

Cython/Compiler/Symtab.py

tests/run/cpp_class_attrib.srctree

…ions

scoder · 2019-01-14T20:47:08Z

Cython/Compiler/ExprNodes.py

@@ -7146,6 +7146,8 @@ def calculate_result_code(self):
        obj_code = obj.result_as(obj.type)
        #print "...obj_code =", obj_code ###
        if self.entry and self.entry.is_cmethod:
+            if self.entry.is_cgetter:
+                return "%s(%s)" %(self.entry.func_cname, obj_code)


Whitespace missing after "%".

scoder · 2019-01-14T20:51:33Z

Cython/Compiler/Nodes.py

+                    elif func.name == 'staticmethod':
+                        pass
+                    else:
+                        error(self.pos, "Cannot handle %s decorators yet" % func.name)


I think we must also reject decorators that are not plain names, e.g. attributes or function calls. Anything that's not @property or @staticmethod.

done, extra test added. Since I want to print the type of the unhandled instance, I needed to rename type -> type in that function.

scoder · 2019-01-14T21:07:07Z

Cython/Compiler/Nodes.py

+        if is_property:
+            self.entry.is_property = 1
+            env.property_entries.append(self.entry)
+            env.cfunc_entries.remove(self.entry)


This makes me wonder why we shouldn't be calling declare_property() here in the first place, instead of declare_cfunction(). What do you think? It seems to do the right thing, pretty much.

CClassScope.declare_property does not dive into the wrapped function the way CClassScope.declare_cfuntion does. I played with this for a while but could not get it to work cleanly, without in the end calling declare_cfunction anyway.

done, extra test added. Since I want to print the type of the unhandled instance, I needed to rename type -> type in that function.

Edit: wrong comment

scoder · 2019-01-14T21:13:13Z

Cython/Compiler/Symtab.py

@@ -752,7 +754,8 @@ def register_pyfunction(self, entry):

    def declare_cfunction(self, name, type, pos,
                          cname=None, visibility='private', api=0, in_pxd=0,
-                          defining=0, modifiers=(), utility_code=None, overridable=False):
+                          defining=0, modifiers=(), utility_code=None,
+                          overridable=False):


These are now unfortunate, unnecessary changes. Could you undo the signature reformatting?

mattip · 2019-01-16T13:17:13Z

How do you easily tell which test is failing on travis? I ended up rerunning the failing build locally

Edit: ... and it passed :(

scoder · 2019-01-16T20:16:01Z

Looks ok now, thanks!

mattip · 2019-01-17T18:24:26Z

darn, this isn't quite good enough. It doesn't handle @property in a pxd file (like numpy.__init__.pxd, it doesn't handle cdef *int vec(self) nodes, and it doesn't handle obj.field1 == obj.field2 type nodes. I am working on fixes. but just wanted to give a heads up that the feature is only partially complete. There may be a design flaw that a decorated CFuncDefNode is different from a Property node, maybe they should be the same.

Or maybe this is beyond the scope of where you want cython to go, and the whole thing should be reverted?

scoder · 2019-01-18T07:34:25Z

a decorated CFuncDefNode is different from a Property node

Yeah, that sounds wrong. We should reuse the property infrastructure that we have. IIUC, the only difference is that these properties are resolved at compile time and the underlying method gets called. They are essentially "inline" properties.

mattip · 2019-01-18T07:38:30Z

Is there any assumption Property nodes are on python classes and not c classes? Or maybe that's what you mean about "inline".

scoder · 2019-01-18T08:05:38Z

They are always on cdef classes. Properties on Python classes are just arbitrary objects and not special syntax tree constructs.
The missing feature is that property nodes were not previously useful on external classes, since there was no code that Cython needed to generate for them. The fact that the usage of properties could be inlined into direct method calls (not only for external classes) was not really considered. But it could apply to properties marked as @cython.inline as well as any property on final classes, whether implemented in the module or externally cimported.

scoder · 2019-01-18T08:08:33Z

(BTW, thanks for asking the right questions and digging into the details of this feature. I think we are slowly getting an idea of where this is really going.)

mattip · 2019-05-26T16:05:02Z

I want to get back to this and finish it up.

But it could apply to properties marked as @cython.inline as well as any property on final classes, whether implemented in the module or externally cimported

I am not sure what you mean by this. Looking at the implementation of @cython.inline, it seems more like a construct used in a .py file to create a "local" c-extension module with a single function than a general-purpose decorator on a cdef class or function

Supersedes GH-2640 and GH-3095. Closes GH-3521.

* Rewrite C property support (GH-2640) based on inline C methods. Supersedes GH-2640 and GH-3095. Closes GH-3521. * Test fix for `numpy_parallel.pyx`: avoid depending on whether "nd.shape" requires the GIL or not. * Turn NumPy's "ndarray.data" into a property to avoid direct struct access. * Make "ndarray.size" accessible without the GIL.

mattip commented Oct 2, 2018

View reviewed changes

Cython/Compiler/Nodes.py Show resolved Hide resolved

mattip commented Oct 2, 2018

View reviewed changes

Cython/Compiler/Nodes.py Outdated Show resolved Hide resolved

mattip commented Oct 2, 2018

View reviewed changes

Cython/Compiler/Nodes.py Show resolved Hide resolved

mattip mentioned this pull request Oct 2, 2018

ENH: Generalized ufunc signature expansion for frozen and flexible dimensions numpy/numpy#11175

Merged

robertwb reviewed Oct 3, 2018

View reviewed changes

Cython/Compiler/Nodes.py Show resolved Hide resolved

Cython/Compiler/Nodes.py Show resolved Hide resolved

This was referenced Oct 29, 2018

ENH: supply our version of numpy.pxd, requires cython>=0.29 numpy/numpy#12284

Merged

ENH: Do not emit compiler warning if forcing old API numpy/numpy#12286

Merged

scoder reviewed Nov 3, 2018

View reviewed changes

WIP ENH: allow @Property decorator on external ctypedef classes

726ebd8

mattip force-pushed the ctypedef-class-getter2 branch 2 times, most recently from 2d60950 to 83d5bdd Compare November 9, 2018 02:33

mattip added 5 commits November 13, 2018 14:29

add a new ReplacePropertyNode pass

fdddf34

ENH: handle c-level property decorator to produce a cfunc call

a6ec7fb

BUG: fix test to use correct macros, functions for c-level getters

07aaffa

fix for fused nodes

bee7b69

whoops, staticmethod is a valid cdef decorator

3b931f1

mattip force-pushed the ctypedef-class-getter2 branch from 83d5bdd to 3b931f1 Compare November 14, 2018 03:19

fix one more delcare_cfunction

2816f78

mattip changed the title ~~WIP ENH: allow @property decorator on external ctypedef classes~~ ENH: allow @property decorator on external ctypedef classes Nov 16, 2018

scoder added this to the 3.0 milestone Nov 18, 2018

mattip mentioned this pull request Nov 26, 2018

add method to silence old API deprecation build warnings numpy/numpy#11653

Closed

scoder reviewed Jan 2, 2019

View reviewed changes

mattip added 2 commits January 7, 2019 16:35

MAINT: refactor: is_cgetter now lives on the entry, use find_decorator

f58cc9a

MAINT: refactor decorator analysis

d136c75

scoder reviewed Jan 7, 2019

View reviewed changes

Cython/Compiler/ParseTreeTransforms.py Show resolved Hide resolved

MAINT: remove dead useless code

e936829

scoder reviewed Jan 7, 2019

View reviewed changes

Cython/Compiler/Symtab.py Outdated Show resolved Hide resolved

Cython/Compiler/Symtab.py Outdated Show resolved Hide resolved

scoder reviewed Jan 13, 2019

View reviewed changes

tests/run/cpp_class_attrib.srctree Outdated Show resolved Hide resolved

MAINT: localize is_property handling to CFuncDefNode.analyse_declarat…

ccb6708

…ions

mattip force-pushed the ctypedef-class-getter2 branch from 261ad4d to ccb6708 Compare January 13, 2019 13:32

scoder reviewed Jan 14, 2019

View reviewed changes

MAINT: fixes from review

499fd67

scoder merged commit 51ac02b into cython:master Jan 16, 2019

scoder added feature Cython Language Feature labels Jan 16, 2019

mattip deleted the ctypedef-class-getter2 branch May 26, 2019 13:38

mattip restored the ctypedef-class-getter2 branch May 26, 2019 13:39

mattip mentioned this pull request Jun 10, 2019

BUG: prefer sys.path before Cython/Include when searching for pxd files #2910

Merged

mattip mentioned this pull request Jun 23, 2019

MAINT: ship common.pxd, bit_generator.pxd in the wheel mattip/numpy#40

Merged

This was referenced Aug 25, 2019

BUG: support wider use of @property decorator on CClassDef AttributeNodes #3095

Merged

Refactor @property on C #3107

Open

scoder added a commit that referenced this pull request May 4, 2020

Rewrite C property support (GH-2640) based on inline C methods.

35a93d1

Supersedes GH-2640 and GH-3095. Closes GH-3521.

scoder mentioned this pull request May 4, 2020

Rewrite the C property feature #3571

Merged

ENH: allow @property decorator on external ctypedef classes #2640

ENH: allow @property decorator on external ctypedef classes #2640

mattip commented Oct 2, 2018 •

edited

mattip commented Oct 29, 2018

mattip commented Nov 9, 2018

mattip commented Nov 9, 2018

mattip commented Nov 9, 2018

mattip commented Nov 14, 2018

mattip commented Nov 14, 2018

mattip commented Nov 16, 2018

mattip commented Nov 26, 2018

scoder left a comment

scoder Jan 2, 2019

mattip Jan 5, 2019

scoder Jan 14, 2019

mattip Jan 16, 2019

scoder Jan 14, 2019

mattip Jan 16, 2019

scoder Jan 14, 2019

mattip Jan 16, 2019

mattip Jan 16, 2019 •

edited

scoder Jan 14, 2019

mattip Jan 16, 2019

mattip commented Jan 16, 2019 •

edited

scoder commented Jan 16, 2019

mattip commented Jan 17, 2019

scoder commented Jan 18, 2019

mattip commented Jan 18, 2019

scoder commented Jan 18, 2019

scoder commented Jan 18, 2019

mattip commented May 26, 2019

ENH: allow @property decorator on external ctypedef classes #2640

ENH: allow @property decorator on external ctypedef classes #2640

Conversation

mattip commented Oct 2, 2018 • edited

mattip commented Oct 29, 2018

mattip commented Nov 9, 2018

mattip commented Nov 9, 2018

mattip commented Nov 9, 2018

mattip commented Nov 14, 2018

mattip commented Nov 14, 2018

mattip commented Nov 16, 2018

mattip commented Nov 26, 2018

scoder 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

mattip Jan 16, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattip commented Jan 16, 2019 • edited

scoder commented Jan 16, 2019

mattip commented Jan 17, 2019

scoder commented Jan 18, 2019

mattip commented Jan 18, 2019

scoder commented Jan 18, 2019

scoder commented Jan 18, 2019

mattip commented May 26, 2019

mattip commented Oct 2, 2018 •

edited

mattip Jan 16, 2019 •

edited

mattip commented Jan 16, 2019 •

edited