[Cython] star-imports in Cython/Includes/cpython considered harmful

[Stefan Behnel]

Robert Bradshaw, 21.02.2012 21:14:
> On Mon, Feb 20, 2012 at 12:45 AM, Stefan Behnel wrote:
>> I just noticed that the star-imports in the cpython package have serious
>> side effects. The cpython/__init__.pxd file has this in it:
>>
>> """
>> from cpython.version cimport *
>> from cpython.ref cimport *
>> from cpython.exc cimport *
>> from cpython.module cimport *
>> from cpython.mem cimport *
>> ...
>> """
>>
>> This means that a direct cimport of any of those recursively cimported
>> names from the cpython package namespace will always trigger a complete
>> cimport of all cpython.* modules, thus
>>
>> 1) wasting compile time
> 
> Which is still pretty cheap (and should be made extremely cheap for
> unchanging pxd files).
> 
>> 2) polluting the cpython package namespace
> 
> It's neither inspected (by the end user) at runtime, nor in danger of
> conflicts, so I don't see this as a big deal. "Flat is better than
> nested."
> 
>> 3) polluting the internal state of Cython's list of cimported modules
> 
> This is really (1), other than that it doesn't really hurt to have
> extra keys in a dict.

Except when it has side-effects as in 4). If we can figure out what parts
of the cimported declarations are actually used, we could still drop
unnecessary declarations. But that doesn't currently happen.


>> 4) leading to things being defined in the generated C code that don't need
>> to be there, specifically the object structs of PyBoolObject and
>> PyComplexObject, which we provide definitions for in cpython.bool and
>> cpython.complex.
> 
> This is an actual issue that should be fixed, and avoiding emitting
> unused code is a worthwhile goal in many contexts.

This issue was the reason I brought this up in the first place.


>> The point where I noticed this is when I got test errors in PyPy because it
>> doesn't expose the bool/complex structs. That wasn't because the test
>> actually used them, it was purely because it had this cimport in it:
>>
>>  from cpython cimport PyObject
>>
>> If it had used this instead:
>>
>>  from cpython.object cimport PyObject
>>
>> Cython would still have parsed all of those .pxd files, but at least it
>> wouldn't have used those declarations to do any harm.
>>
>> Now, I'm 100% certain that there is user code out there that does this as
>> well. Fixing this bug by removing all the star-imports will break that
>> code, but keeping the cimports in the package will trap the average lazy
>> user into cimporting stuff from there directly.
>>
>> I'll add a big warning to the package for now, but I wonder if we shouldn't
>> accept breaking this code at some point (and sooner is always better than
>> later).
> 
> I actually don't think it's a bad thing to allow people to be "lazy"
> and import directly from cpython rather than have to hunt down the
> specific subpackages that things live in. In fact, it's cleaner in
> some sense to not have to know about the subdivisions of the cpython
> library. (It's like including "Python.h" rather than all the
> individual header files.)

As long as it doesn't have side-effects to do this, it's not a problem. And
I agree that it's better to actually fix the side-effects. Just not
importing from the cpython package doesn't make the problem go away that
your code won't compile on PyPy as soon as you cimport anything from
cpython.bool or cpython.complex.

Stefan