Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PEP 590: kwnames should be non-empty tuple (or NULL) #1049

Closed
wants to merge 1 commit into from

Conversation

jdemeyer
Copy link
Contributor

For simplifying the code of the vectorcall callee, do not allow an empty tuple to be passed as kwnames. This way, there is a unique way to specify "no keyword arguments", namely passing a NULL pointer.

CC @encukou @markshannon @scoder

@encukou
Copy link
Member

encukou commented May 16, 2019

This has been argued in here and (since I seem to have gotten the threads confused) here.
There has not been a consensus, so I'll make a call: allow empty tuples.

The Robustness principle for designing interfaces has been brought up. To our case, I believe it applies like this:

  • The caller should be conservative in what it sends, i.e. it SHOULD send NULL.
  • The callee should be liberal in what it receives, i.e. it SHOULD handle both empty tuple and NULL.

Both are good advice, but neither says much about the protocol.
I believe good a design should (all else being equal) avoid undefined states. So, the empty tuple should be valid.

That means that (partially quoting the earlier discussion) the check for empty args will not be the simple:

if (kwnames != NULL)

but rather:

if (kwnames != NULL && PyTuple_GET_SIZE(kwnames))

This is slightly more complicated, but not slower as the additional check is only required during error handling.
I also believe that for someone who doesn't read the specs carefully, handling the empty tuple in the callee is more obvious than remembering to convert empty tuples to NULL in the caller.

Please reopen if you think it needs to be discussed further.

@jdemeyer
Copy link
Contributor Author

jdemeyer commented May 17, 2019

as the additional check is only required during error handling.

I don't think that the argument of speed really matters (checking whether a tuple is empty is very fast, my motivations were more about simplicity and robustness than speed), but since you brought it up: it's not that the condition is only used during error handling. It's used during error checking, also in the non-error case. If an empty tuple becomes a valid way to indicate "no keyword arguments", you need to check that the tuple is empty if you don't want keyword arguments.

I also believe that for someone who doesn't read the specs carefully, handling the empty tuple in the callee is more obvious than remembering to convert empty tuples to NULL in the caller.

I already argued that we really should optimize for the callee here: the callee is often external code while the caller is almost certainly CPython (external code should use PyObject_Vectorcall to make the call and that function will handle empty tuples, converting them to NULL). So I'd like to know why you disagree with this point.

(I know I'm repeating the same arguments here, but only to refute the same arguments that have been made before)

@jdemeyer
Copy link
Contributor Author

I also believe that for someone who doesn't read the specs carefully

Can we then at least write explicitly in the PEP that empty tuples are valid?

@jdemeyer
Copy link
Contributor Author

Please reopen if you think it needs to be discussed further.

Note: I don't actually have permissions to do that.

Of course, if you decide to allow empty tuples, I can live with that, it's actually a tiny change in the implementation of PEP 590.

I just want to know that this decision was made for the right reasons. In particular, your comment seems to indicate some kind of symmetry between the caller and callee. If you assume such symmetry, I agree that it's hard to decide what the best option is. But I do not agree with the assumption of symmetry: external code is far more likely to be callee rather than caller.

@encukou
Copy link
Member

encukou commented May 17, 2019

Note: I don't actually have permissions to do that.

Apologies, I thought the author had the power! Reopening for you.
But, as you said, I did hear the arguments.

your comment seems to indicate some kind of symmetry between the caller and callee. If you assume such symmetry, I agree that it's hard to decide what the best option is. But I do not agree with the assumption of symmetry: external code is far more likely to be callee rather than caller.

Stefan's post linked from the PEP mentions that several tools might want to use vectorcall even on CPython versions that didn't support it. These tools will necessarily need to supply both callers and callees.
But sure, let's suppose callees are more common.

we really should optimize for the callee here

We're not optimizing for performance, but for, let's say, "developer experience". What is that?
"Writing less code" is a red herring here – whether you write a caller or callee, the && PyTuple_GET_SIZE(kwnames) is a tiny part of the overall mental effort. (Unless you copy&paste from elsewhere, and then this discussion is moot.)
The trade-off is not about writing less code, but about making it easier to write correct code.
The empty dict is a much more "obvious" special case: a reviewer unfamiliar with vectorcall details is much more likely to ask what if you get a tuple but it's empty? than notice the special case for empty dict. You don't need a vectorcall expert to spot a bug – just a plain CPython expert.
That makes the argument for me, even if there are more callees than callers.

Can we then at least write explicitly in the PEP that empty tuples are valid?

Definitely!

@encukou encukou reopened this May 17, 2019
@jdemeyer
Copy link
Contributor Author

The empty dict is a much more "obvious" special case: a reviewer unfamiliar with vectorcall details is much more likely to ask what if you get a tuple but it's empty? than notice the special case for empty dict. You don't need a vectorcall expert to spot a bug – just a plain CPython expert.

Are you trying to say that it's easier for a developer A to convince a reviewer B that the code written by A is correct? Basically, to avoid the following scenario, assuming that empty tuples are forbidden:

A. writes code of the form if (kwnames == NULL) ...
A: Done! Needs review!
B: But what if kwnames is not NULL but the empty tuple? You forgot to handle that case, that's a bug in your code.
A: Sigh... Look at PEP 590! It says right there that the tuple can't be empty, so my code is correct.
B: Yes I know, but... are you sure that we can rely on that? I mean, what if somebody implements PEP 590 wrongly?
A: OK, fine. Let's add the stupid check anyway (I want my PR merged so I better go along with B)

@encukou
Copy link
Member

encukou commented May 17, 2019

No, I mean the following scenario when implementing the caller:

A. writes a caller that transforms kwargs to tuples, including empty kwargs to empty tuples
A: Done! Needs review!
B: I'm not super familiar with vectorcall, but this makes sense. Ship it!

@jdemeyer
Copy link
Contributor Author

OK, I see what you mean now but I think that your scenario is unlikely mainly because "implementing the caller" is unlikely.

But maybe we should stop discussing and "agree to disagree". I would give @scoder a chance to reply in case he has a strong opinion. In any case, this is a detail and won't make a big difference for the PEP 590 implementation.

@encukou
Copy link
Member

encukou commented May 17, 2019

But maybe we should stop discussing and "agree to disagree".

The benefit of the conversation was clarifying intended use cases, rather than actually bikeshedding on this detail. I think we now both understand the other's point of view.
So yes, let's spend our time and energy on something important.

Let's reopen if there is some new argument to be heard.

@encukou encukou closed this May 17, 2019
@encukou
Copy link
Member

encukou commented May 22, 2019

If reopened, we should also look at the the CALL_FUNCTION_KW opcode: https://bugs.python.org/issue36936 & python/cpython#13357

@jdemeyer
Copy link
Contributor Author

I reopened, we should also look at the the CALL_FUNCTION_KW opcode: https://bugs.python.org/issue36936 & python/cpython#13357

Reopened what? I don't understand what you're trying to say here.

@encukou
Copy link
Member

encukou commented May 22, 2019

Sorry! That was a bad typo. I meant "If reopened".

@jdemeyer
Copy link
Contributor Author

Bad typo indeed. I couldn't guess that you meant that :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants