cython icon indicating copy to clipboard operation
cython copied to clipboard

Remove "from x cimport class C"

Open da-woods opened this issue 2 years ago • 1 comments

(and "from x cimport struct S"/"from x cimport union U")

This appears to be an old Pyrex feature introduced in https://github.com/cython/cython/commit/9bac9c2e014ed63c2ac435e9b431fae124fba668 to provide a way of "forward cimporting" a class. I originally tried to make a test for it, but couldn't actually come up with a useful way of using it in the intended way, where the name would be unavailable initially but avaialble later.

It looks to be completely untested, and responsible for some missing coverage in Nodes.py (https://github.com/cython/cython/issues/4163). The large section containing

if kind == 'struct' or kind == 'union':  # 8479 ↛ 8480

I propose to fix the missing coverage by killing off the feature.

The only people I could find using this syntax were H5Py, who look to have removed their sole use of it 3 years ago https://github.com/h5py/h5py/commit/8d2498c7f5e3fec884ff56e9aca905c325d82484 Therefore it seems a good candidate to go in Cython 3

da-woods avatar Jul 19 '22 11:07 da-woods

FWIW looking at the Pyrex changelog (https://www.csse.canterbury.ac.nz/greg.ewing/python/Pyrex/version/CHANGES.txt)

0.9.8.5
-------

[...]

Deprecations:

  - The features introducted in 0.9.8 and 0.9.8.1 for cross-forward-declaring
    extension types between .pxd files turn out to be unnecessary, since
    the circular import problems they are aimed at can be avoided using
    ordinary forward delcarations in the .pxd files ahead of any cimports.

[...]

0.9.8.1
-------

 [...]

  - There's now an even easier way to forward-declare a struct, union
    or extension type in another module:
    
    	from blarg cimport class Foo
    	
    This simultaneously cimports the name Foo and forward-declares
    it as an extension type. As well as 'class', you can also use
    'struct' or 'union'.

so it looks like this feature didn't last too long in Pyrex either.

da-woods avatar Jul 19 '22 12:07 da-woods

I propose to fix the missing coverage by killing off the feature.>

+1, I also knew nothing about it.

scoder avatar Oct 11 '22 07:10 scoder

I'll merge this and the 0.29.x warning then...

da-woods avatar Oct 11 '22 07:10 da-woods