Skip to content

gh-127610: Added validation for more than one var positional and var keyword parameters in inspect.Signature #127657

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 10 commits into from
Dec 8, 2024
Merged

gh-127610: Added validation for more than one var positional and var keyword parameters in inspect.Signature #127657

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e4ae535
Tests to check validation of more than one var positional and var key…
ApostolFet Dec 5, 2024
8aac6f1
Added validation for more than one var positional and var keyword par…
ApostolFet Dec 5, 2024
64b53a8
Add name in ACKS
ApostolFet Dec 5, 2024
97c086c
Fix review remarks
ApostolFet Dec 6, 2024
a7a190a
📜🤖 Added by blurb_it.
blurb-it[bot] Dec 6, 2024
0f827ed
Update Misc/NEWS.d/next/Library/2024-12-06-17-28-55.gh-issue-127610.c…
ApostolFet Dec 7, 2024
539d7bc
Moved up a new check, using set instead of bool variables
ApostolFet Dec 7, 2024
fb25db2
Update Misc/NEWS.d/next/Library/2024-12-06-17-28-55.gh-issue-127610.c…
ApostolFet Dec 7, 2024
942a448
Added caption
ApostolFet Dec 7, 2024
0de21a2
Using f-string instead of the format function
ApostolFet Dec 7, 2024
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
16 changes: 16 additions & 0 deletions Lib/inspect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2943,6 +2943,8 @@ def __init__(self, parameters=None, *, return_annotation=_empty,
params = OrderedDict()
top_kind = _POSITIONAL_ONLY
seen_default = False
seen_var_positional = False
seen_var_keyword = False

for param in parameters:
kind = param.kind
Expand Down Expand Up @@ -2971,6 +2973,20 @@ def __init__(self, parameters=None, *, return_annotation=_empty,
# There is a default for this parameter.
seen_default = True

if kind == _VAR_POSITIONAL:
if seen_var_positional:
msg = 'more than one var-positional parameter'
raise ValueError(msg)

seen_var_positional = True

if kind == _VAR_KEYWORD:
if seen_var_keyword:
msg = 'more than one var-keyword parameter'
raise ValueError(msg)

seen_var_keyword = True

if name in params:
msg = 'duplicate parameter name: {!r}'.format(name)
raise ValueError(msg)
Expand Down
8 changes: 8 additions & 0 deletions Lib/test/test_inspect/test_inspect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2992,6 +2992,14 @@ def test2(pod=42, /):
with self.assertRaisesRegex(ValueError, 'follows default argument'):
S((pkd, pk))

second_args = args.replace(name="second_args")
with self.assertRaisesRegex(ValueError, 'more than one var-positional parameter'):
S((args, second_args))
Copy link
Member

Choose a reason for hiding this comment

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

Add also a test for the case when there are other parameters (keyword-only or var-keyword) between two var-positional parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case we will get error: ValueError: wrong parameter order: keyword-only parameter before variadic positional parameter. Should we catch it in the test?

My test:S((args, ko, second_args))

Copy link
Member

Choose a reason for hiding this comment

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

Technically, there are two errors: "keyword-only parameter before variadic positional parameter" and "more than one variadic positional parameter". Which of them are preferable to report? I think the latter, therefore we should change the order of checks. And tests are needed to ensure that the correct error message is used.

Copy link
Contributor Author

@ApostolFet ApostolFet Dec 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In that case we all need the variables seen_var_positional and seen_var_keyword, to check that these parameters have not occurred before even if the parameter order is wrong, right?
if kind == top_kind and kind in (_VAR_POSITIONAL, _VAR_KEYWORD) - this check is designed for the correct order of parameters

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, yes, you are right. So you need to simply move the new checks up. I still suggest to use kind.description to format the error message.

You can unify the code for var-positional and var-keyword if use a set instead of two boolean variables.

if kind in (_VAR_POSITIONAL, _VAR_KEYWORD):
    if kind in seen_var_parameters:
        raise ...
    seen_var_parameters.add(kind)


second_kwargs = kwargs.replace(name="second_kwargs")
with self.assertRaisesRegex(ValueError, 'more than one var-keyword parameter'):
S((kwargs, second_kwargs))

def test_signature_object_pickle(self):
def foo(a, b, *, c:1={}, **kw) -> {42:'ham'}: pass
foo_partial = functools.partial(foo, a=1)
Expand Down
1 change: 1 addition & 0 deletions Misc/ACKS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ Eitan Adler
Anton Afanasyev
Ali Afshar
Nitika Agarwal
Maxim Ageev
Anjani Agrawal
Pablo S. Blum de Aguiar
Jim Ahlstrom
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2024-12-06-17-28-55.gh-issue-127610.ctv_NP.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Added validation for more than one var positional and var keyword parameters in ``inspect.Signature.__init__``
Loading