Make PyDateTime::new take a &PyTzInfo

[konstin]

Change the argument of PyDateTime::new to take a Option<&PyTzInfo> instead of a Option<&PyObject> as per discussion in #377 (comment).

[pganssle]

@konstin One thing we may want to take into account would be the possible performance implications of this, I think. I'm not sure if extract is a zero-cost abstraction. I think right now &PyObject is used because it can be used with PyTzInfo objects created entirely in Rust and with tzinfo objects passed through Python, and it's safe-ish in the sense that, while not enforced at compile-time, it will not panic if something other than tzinfo is passed, it will raise a Python exception.

Unfortunately with all the procedural macros (and the fact that the overhead has not been optimized too much), it's hard for me to tell how much overhead we have on the existing functions anyway, and how much this will change it.

[konstin]

Oh, that's a good point. The conversion is not zero cost, because we need to make one call to cpython's type check method. But I don't think we should care about the performance impact for now as we don't have benchmarks nor too many optimizations.

[pganssle]

Thinking about this more, the requirement that something be a PyTzInfo is unnecessary and doesn't match the spirit of the C/Python API. I think a better approach might be to switch this over to:

    pub fn new<TZ: IntoPyTzInfo, 'p> (
        py: Python<'p>,
        year: i32,
        ...
        tzinfo: Option<TZ>,
    ) -> PyResult<&'p PyDateTime>
{

The C API enforces that whatever's there is a tzinfo object, but not for any structural reason - tzinfo is an abstract base class that defines an interface. In rust tzinfo is best expressed as a trait.

[pganssle]

I just realized the downside of the above is that it will require an explicit reference to PyTzInfo or something whenever you pass None, so:

PyDateTime::new(2019, 1, 1, 0, 0, 0, 0, None)

will have to become:

PyDateTime::new::<PyTzInfo>(2019, 1, 1, 0, 0, 0, None)

which I don't love. Maybe the best option is to switch it over to PyAny and let CPython enforce the relevant conditions on the input.

[konstin]

Using a trait sounds great! We might be able to solve the PyDateTime::new::<PyTzInfo> with a default for the generic .

[pganssle]

@konstin I wasn't aware of defaulted generic parameters, thanks for the pointer. Apparently, they are not valid for function definitions, though (the compiler points to this issue), which means we can't use them for the constructors.

I'm still kinda in favor of using PyAny or something more generic here. There's a runtime cost associated with using ::extract, and I'm not sure how much enforcing the type of the object passed buys us, considering the recommended way to take a Python object and pass it is to use ::extract, which has no way, to tell, at compile time, that the object can be interpreted as a tzinfo.

[davidhewitt]

Closing; in #1588 I changed PyDateTime::new to take Option<&PyTzInfo>.