bpo-37034: Display argument name on errors with keyword-only arguments with Argument Clinic

[remilapeyre]

https://bugs.python.org/issue37034

[vstinner]

Is _PyArg_BadArgument() used with positional-only argument? If yes, your PR is wrong. The name of positional-only arguments must not be used.

[remilapeyre]

Is _PyArg_BadArgument() used with positional-only argument? If yes, your PR is wrong. The name of positional-only arguments must not be used.

I tried to take care of this case in https://github.com/python/cpython/pull/13593/files#diff-5e15637e15d50f49f6cb1d7f69fcab4bR854. Did I miss something? If so, happy to start again :D

[vstinner]

argnum = repr(p.name) if p.is_keyword_only() else str(i + 1)

I don't think that this line is correct. You should also use the parameter name for positional-or-keyword. I propose the following change on top of your PR:

diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py
index f6f8704ba6..68303f5b19 100755
--- a/Tools/clinic/clinic.py
+++ b/Tools/clinic/clinic.py
@@ -928,7 +928,7 @@ class CLanguage(Language):
 
             add_label = None
             for i, p in enumerate(parameters):
-                argnum = repr(p.name) if p.is_keyword_only() else str(i + 1)
+                argnum = p.name if p.is_positional_or_keyword() else str(i + 1)
                 parsearg = p.converter.parse_arg(argname_fmt % i, argnum)
                 if parsearg is None:
                     #print('Cannot convert %s %r for %s' % (p.converter.__class__.__name__, p.converter.format_unit, p.converter.name), file=sys.stderr)
@@ -2267,6 +2267,9 @@ class Parameter:
     def __repr__(self):
         return '<clinic.Parameter ' + self.name + '>'
 
+    def is_positional_or_keyword(self):
+        return self.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD
+
     def is_keyword_only(self):
         return self.kind == inspect.Parameter.KEYWORD_ONLY
 

I'm not even sure that my change is correct. Maybe is_positional_only() should be used to decide if the name must not be used.

I removed repr() to let the compiler merge duplicated strings. If you care of '...' format, please add use 2 exclusive parameters in _PyArg_BadArgument(): one for argument position, one for positional-only arguments.

[vstinner]

If you care of '...' format, please add use 2 exclusive parameters in _PyArg_BadArgument(): one for argument position, one for positional-only arguments.

By the way, that would be my preference: it's strange to store an integer as a bytes string ("2") rather than just an int (2).

[remilapeyre]

You should also use the parameter name for positional-or-keyword.

Thanks, I wasn't sure what to do here so I made a conservative choice but I agree this is better.

In the implementation of _PyArg_BadArgument(), the position of the argument is checked so that it is not displayed when it is the first argument. I think it was introduced in this commit : 4fa9591#diff-8aaf09dbd5eefbcdceb804ab2143a907. This seems weird to me, you get the position of the argument in the error message if an error happened with the second, the third, etc. but not if it's the first. Should I keep this behavior?

[vstinner]

In the implementation of _PyArg_BadArgument(), the position of the argument is checked so that it is not displayed when it is the first argument.

If the parameter is a keyword-only or positional-or-keyword, IHMO we must display its name. it's helpful.

[remilapeyre]

Yes but for positional only parameters, the current message is

>>> b = BytesIO(b"foobar")
>>> b.readinto(4)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: readinto() argument must be read-write bytes-like object, not int

without indications about which parameter (when it is the first one), instead of the more useful

>>> b.readinto(4)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: readinto() argument 0 must be read-write bytes-like object, not int

This could be confusing when there is multiple arguments, should I keep the first one or make it display the second one?

[vstinner]

TypeError: readinto() argument must be read-write bytes-like object, not int

is fine if the function accepts 0 or 1 argument.

TypeError: readinto() argument 0 must be read-write bytes-like object, not int

is bad if the function accepts more than 1 argument.

[remilapeyre]

Hi Victor, thanks for your feedback. I updated the PR based on your comments, it should be better now.

[auvipy]

please re base

[remilapeyre]

please re base
Is this recommanded? I thought that we needed to avoid rebasing to keep the comments in the review meaningful and that everything would get squashed anyway.

[vstinner]

LGTM. I just ask a minor change, add a comment :-)

@serhiy-storchaka: Would you mind to double check this change?

[remilapeyre]

Hi, I made the last changes and regenerated the clinic files. I tried to do a squashed merge but that made GitHub noisy-ing nearly everybody, sorry about that :/

[miss-islington]

Thanks @remilapeyre for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

[miss-islington]

Sorry, @remilapeyre and @serhiy-storchaka, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 4901fe274bc82b95dc89bcb3de8802a3dfedab32 3.8

[bedevere-bot]

GH-15599 is a backport of this pull request to the 3.8 branch.