Skip to content

bpo-30627: Fix error message when keyword arguments are used #2115

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 15, 2017
Merged

bpo-30627: Fix error message when keyword arguments are used #2115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e116e0b
bpo-20627: Fix error message when keywod arguments are used
SylvainDe Jun 10, 2017
c1e597d
bpo-20627: Fix error message when keyword arguments are used - bis
SylvainDe Jun 11, 2017
7f8e621
bpo-20627: Fix error message for struct.Struct.pack
SylvainDe Jun 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions Lib/test/test_call.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import _testcapi
except ImportError:
_testcapi = None
import struct
import collections

# The test cases here cover several paths through the function calling
# code. They depend on the METH_XXX flag that is used to define a C
Expand Down Expand Up @@ -160,6 +162,30 @@ def test_varargs5_kw(self):
msg = r"^hasattr\(\) takes no keyword arguments$"
self.assertRaisesRegex(TypeError, msg, hasattr, x=2)

def test_varargs6_kw(self):
msg = r"^getattr\(\) takes no keyword arguments$"
self.assertRaisesRegex(TypeError, msg, getattr, x=2)

def test_varargs7_kw(self):
msg = r"^next\(\) takes no keyword arguments$"
self.assertRaisesRegex(TypeError, msg, next, x=2)

def test_varargs8_kw(self):
msg = r"^pack\(\) takes no keyword arguments$"
self.assertRaisesRegex(TypeError, msg, struct.pack, x=2)

def test_varargs9_kw(self):
msg = r"^pack_into\(\) takes no keyword arguments$"
self.assertRaisesRegex(TypeError, msg, struct.pack_into, x=2)

def test_varargs10_kw(self):
msg = r"^index\(\) takes no keyword arguments$"
self.assertRaisesRegex(TypeError, msg, collections.deque().index, x=2)

def test_varargs11_kw(self):
msg = r"^pack\(\) takes no keyword arguments$"
self.assertRaisesRegex(TypeError, msg, struct.Struct.pack, struct.Struct(""), x=2)

def test_oldargs0_1(self):
msg = r"keys\(\) takes no arguments \(1 given\)"
self.assertRaisesRegex(TypeError, msg, {}.keys, 0)
Expand Down
4 changes: 2 additions & 2 deletions Modules/_hashopenssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -931,11 +931,11 @@ generate_hash_name_list(void)
Py_buffer view = { 0 }; \
PyObject *ret_obj; \
\
if (!_PyArg_ParseStack(args, nargs, "|O:" #NAME , &data_obj)) { \
if (!_PyArg_NoStackKeywords(#NAME, kwnames)) { \
return NULL; \
} \
\
if (!_PyArg_NoStackKeywords(#NAME, kwnames)) { \
if (!_PyArg_ParseStack(args, nargs, "|O:" #NAME , &data_obj)) { \
return NULL; \
} \
\
Expand Down
20 changes: 14 additions & 6 deletions Modules/_struct.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1823,6 +1823,9 @@ s_pack(PyObject *self, PyObject **args, Py_ssize_t nargs, PyObject *kwnames)
PyObject *result;

/* Validate arguments. */
if (!_PyArg_NoStackKeywords("pack", kwnames)) {
return NULL;
}
soself = (PyStructObject *)self;
assert(PyStruct_Check(self));
assert(soself->s_codes != NULL);
Expand All @@ -1832,9 +1835,6 @@ s_pack(PyObject *self, PyObject **args, Py_ssize_t nargs, PyObject *kwnames)
"pack expected %zd items for packing (got %zd)", soself->s_len, nargs);
return NULL;
}
if (!_PyArg_NoStackKeywords("pack", kwnames)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now Struct.pack() just ignores keyword arguments.

Copy link
Contributor Author

@SylvainDe SylvainDe Jun 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I did not know this could be called via a different path. Thanks for spotting this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the PR, thanks for catching this!

return NULL;
}

/* Allocate a new buffer */
result = PyBytes_FromStringAndSize((char *)NULL, soself->s_size);
Expand Down Expand Up @@ -1866,6 +1866,9 @@ s_pack_into(PyObject *self, PyObject **args, Py_ssize_t nargs, PyObject *kwnames
Py_ssize_t offset;

/* Validate arguments. +1 is for the first arg as buffer. */
if (!_PyArg_NoStackKeywords("pack_into", kwnames)) {
return NULL;
}
soself = (PyStructObject *)self;
assert(PyStruct_Check(self));
assert(soself->s_codes != NULL);
Expand All @@ -1886,9 +1889,6 @@ s_pack_into(PyObject *self, PyObject **args, Py_ssize_t nargs, PyObject *kwnames
}
return NULL;
}
if (!_PyArg_NoStackKeywords("pack_into", kwnames)) {
return NULL;
}

/* Extract a writable memory buffer from the first argument */
if (!PyArg_Parse(args[0], "w*", &buffer))
Expand Down Expand Up @@ -2131,6 +2131,10 @@ pack(PyObject *self, PyObject **args, Py_ssize_t nargs, PyObject *kwnames)
PyObject *s_object = NULL;
PyObject *format, *result;

if (!_PyArg_NoStackKeywords("pack", kwnames)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is needed if we want the pack (and pack_into) function to have the same behavior as other function, ie checking for (unsupported) keyword arguments is the first check we perform. If we remove it, the tests test_varargs8_kw and test_varargs9_kw fail with error "missing format argument" as the check for keyword happened afterward (in the "s_"-prefixed functions).
I'm happy to get rid of the check and remove/update the test if you think it is better.

Thanks for your feedback!

return NULL;
}

if (nargs == 0) {
PyErr_SetString(PyExc_TypeError, "missing format argument");
return NULL;
Expand Down Expand Up @@ -2159,6 +2163,10 @@ pack_into(PyObject *self, PyObject **args, Py_ssize_t nargs, PyObject *kwnames)
PyObject *s_object = NULL;
PyObject *format, *result;

if (!_PyArg_NoStackKeywords("pack_into", kwnames)) {
return NULL;
}

if (nargs == 0) {
PyErr_SetString(PyExc_TypeError, "missing format argument");
return NULL;
Expand Down
12 changes: 6 additions & 6 deletions Python/bltinmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -997,13 +997,13 @@ builtin_getattr(PyObject *self, PyObject **args, Py_ssize_t nargs,
PyObject *v, *result, *dflt = NULL;
PyObject *name;

if (!_PyArg_UnpackStack(args, nargs, "getattr", 2, 3, &v, &name, &dflt))
return NULL;

if (!_PyArg_NoStackKeywords("getattr", kwnames)) {
return NULL;
}

if (!_PyArg_UnpackStack(args, nargs, "getattr", 2, 3, &v, &name, &dflt))
return NULL;

if (!PyUnicode_Check(name)) {
PyErr_SetString(PyExc_TypeError,
"getattr(): attribute name must be string");
Expand Down Expand Up @@ -1307,13 +1307,13 @@ builtin_next(PyObject *self, PyObject **args, Py_ssize_t nargs,
PyObject *it, *res;
PyObject *def = NULL;

if (!_PyArg_UnpackStack(args, nargs, "next", 1, 2, &it, &def))
return NULL;

if (!_PyArg_NoStackKeywords("next", kwnames)) {
return NULL;
}

if (!_PyArg_UnpackStack(args, nargs, "next", 1, 2, &it, &def))
return NULL;

if (!PyIter_Check(it)) {
PyErr_Format(PyExc_TypeError,
"'%.200s' object is not an iterator",
Expand Down