classification
Title: AC: Accept None as a Py_ssize_t default value
Type: Stage: resolved
Components: Argument Clinic Versions: Python 3.7
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: larry, mdk, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2016-12-10 17:12 by mdk, last changed 2016-12-13 13:37 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
issue28933.diff mdk, 2016-12-10 17:17 review
Messages (13)
msg282861 - (view) Author: Julien Palard (mdk) * (Python committer) Date: 2016-12-10 17:12
Today, writing an AC declaration like:

    something: Py_ssize_t(c_default="-1") = None

Leads to the almost obvious "Py_ssize_t_converter: default value None for field something is not of type int".

But it actually make sense: 
 - Accept None as a default python value
 - Document "something=None" in the docstring
 - Write `Py_ssize_t something = -1` in the C code
 - Don't try to parse the argument if it's the default value, keeping the value from the C initialization

In other words, it's a "Give -1 to the C implementation when argument is not given or None, and it may be usefull, typically I'll use it in issue28754.
msg282862 - (view) Author: Julien Palard (mdk) * (Python committer) Date: 2016-12-10 17:17
Proposed a patch, but I'm not a huge fan of modifying getargs.c. If it's accepted, I'll obviously need to write tests before this is merged.
msg282873 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2016-12-10 20:36
You propose an automatic conversion of "None" into "-1"?  That's awful.  Please don't commit that patch to CPython.
msg282874 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2016-12-10 20:43
It looks like you did it with a converter for 28754.  That's okay.  But not in the default implementation.
msg282903 - (view) Author: Julien Palard (mdk) * (Python committer) Date: 2016-12-11 05:12
> It looks like you did it with a converter for 28754.  That's okay.  But not in the default implementation.

It's not by default, we have to declare "… = None" in the AC declaration, which was an error before my patch (incompatible types…).

Before:

    something: Py_ssize_t(c_default="-1") = None

    > Py_ssize_t_converter: default value None for field something is not of type int

After:

    something: Py_ssize_t(c_default="-1") = None

    > If the None (default) value is provided, the c_default is untouched.
msg282904 - (view) Author: Julien Palard (mdk) * (Python committer) Date: 2016-12-11 05:24
> You propose an automatic conversion of "None" into "-1"?  That's awful.  Please don't commit that patch to CPython.

Not really, I propose a way to do it with AC when needed. And the AC semantics introduced are not "Automatic conversion of None into -1" but "automatic "don't touch the C default" when the python default is given", so it's reusable for other values:

    something: Py_ssize_t(c_default="-1") = None
    something: Py_ssize_t(c_default="0") = None
    something: Py_ssize_t(c_default="42") = None
    …

And this semantic can be extended to other types too if needed:

    something: a_type(c_default="a_c_side_default_value") = None

To get "a_c_side_default_value" when None is given.

I however did not introduced a way to keep the C default value by using something else than None in the Python side:

 - I'm not sure this is usefull to allow it but it can still be implemented if needed
 - Limiting to None permits to keep a type check so ``something: Py_ssize_t(c_default="-1") = "bar"`` will still fail at clinic.py runtime.
msg283042 - (view) Author: Julien Palard (mdk) * (Python committer) Date: 2016-12-12 19:57
Hi Larry,

Did you take more time to review this patch and my last comments? I don't think it that awful as it does _not_ apply until explicitly asked for, but I'm open to discuss it.
msg283043 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-12 20:10
Agreed with Larry. "Special cases aren't special enough to break the rules." General converter shouldn't be changed for just one use case. Write your own special converter. Or just use the "O" converter and manually convert Python object to C value. That can be simpler and more readable.
msg283049 - (view) Author: Julien Palard (mdk) * (Python committer) Date: 2016-12-12 21:31
> for just one use case

I don't think that using None in a default argument is "one use case", nor a "special case" it's more like a "widly used pattern" that I'd like to make simple to implement (also see http://bugs.python.org/issue28754#msg282891).

I'm not sure you're against my idea of accepting None like:

    something: Py_ssize_t(c_default="-1") = None

Or you're against my implementation, which I can understand, I'm not a fan of modifying getargs.c for this, it may be doable by implementing a new converter in clinic.py like "Py_ssize_t_optional", and it may not require more work than modifying getargs.c.

But AC is already defining 4 different "default values", counting the "= value" from the AC DSL, I'm not sure that adding the "optional" string somewhere make any of this clearer, typically what about the semantic difference between:

    something: Py_ssize_t_optional()

and

    something: Py_ssize_t() = None

So my original idea was to accept "= None" indistinctly of the used type, I mean allowing:

    something: Py_ssize_t(c_default=…) = None
    something: float(c_default=…) = None
    something: double(c_default=…) = None
    …
    …

Which is, I think, one of the clearest way to express "It's an optional parameter, on the Python side the default value is None, but c-side the default value is of the right type an is …".

In other words, I'm proposing to optionally extend the "c_default" usage from "not given" to "not given or given None".

I failed to implement if for any types, mainly because AC is delegating to getargs for Py_ssize_t, so I though starting with my specific need (Py_ssize_t for issue28754) was a good start and POC.

Another solution may be to change the Py_ssize_t_converter to drop the format_unit, allowing us to implement the converter in clinic instead of getargs.c. It's opening a long term door of completly dropping non-"O" format units. I mean, if we drop every non-"O" format units in clinic we'll be able to use a faster implementation of _PyArg_ParseStack, one not parsing a mini-arg-definition-language at runtime (the "OO|nO&:").

I'm willing to implement something clean for this but please be more precise in your feedback.
msg283056 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-12 21:58
First, it looks weird if the default value affects accepted types. Second, how many use cases for your converter besides the bisect module?
msg283060 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2016-12-12 22:25
I don't want this change committed to CPython, you can do what you need with a converter so do that.
msg283076 - (view) Author: Julien Palard (mdk) * (Python committer) Date: 2016-12-13 06:27
Hi Larry,

Do you mean a new converter in clinic.py like Nullable_Py_ssizze_t, or a converter that I copy/paste every time I need it like http://bugs.python.org/review/28754/patch/19417/76440 ?
msg283098 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2016-12-13 12:18
Since this is the first time anybody has needed it, I suggest the latter.
History
Date User Action Args
2016-12-13 13:37:19serhiy.storchakasetresolution: rejected
stage: resolved
2016-12-13 13:30:22mdksetstatus: open -> closed
2016-12-13 12:18:34larrysetmessages: + msg283098
2016-12-13 06:27:05mdksetmessages: + msg283076
2016-12-12 22:25:45larrysetmessages: + msg283060
2016-12-12 21:58:51serhiy.storchakasetmessages: + msg283056
2016-12-12 21:31:39mdksetmessages: + msg283049
2016-12-12 20:10:29serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg283043
2016-12-12 19:57:06mdksetmessages: + msg283042
2016-12-11 05:24:04mdksetmessages: + msg282904
2016-12-11 05:12:03mdksetmessages: + msg282903
2016-12-10 20:43:09larrysetmessages: + msg282874
2016-12-10 20:36:31larrysetmessages: + msg282873
2016-12-10 17:17:14mdksetfiles: + issue28933.diff
keywords: + patch
messages: + msg282862
2016-12-10 17:14:49mdksettitle: AC: Accept None as a default value for any type -> AC: Accept None as a Py_ssize_t default value
2016-12-10 17:12:56mdkcreate