Skip to content

gh-103556: [inspect.Signature] disallow pos-or-kw params without default after pos-only with default #103557

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

Merged
merged 4 commits into from
Apr 22, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
7 changes: 6 additions & 1 deletion Lib/inspect.py
Original file line number Diff line number Diff line change
Expand Up @@ -3021,7 +3021,12 @@ def __init__(self, parameters=None, *, return_annotation=_empty,
kind.description)
raise ValueError(msg)
elif kind > top_kind:
kind_defaults = False
if (top_kind != _POSITIONAL_ONLY
and kind != _POSITIONAL_OR_KEYWORD):
# We still have to maintain defaults in cases like
# def some(pod=42, /, pk=1): ...
# Here `pk` must have a default value.
kind_defaults = False
Copy link
Member

Choose a reason for hiding this comment

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

non-default-after-default is only checked for _POSITIONAL_ONLY and _POSITIONAL_KEYWORD (see line 3032.) So if we want to keep the value of kind_defaults across the transition from PO to POK (which I do agree is correct and matches the parser behavior), then really we always want to keep it, and it is no longer kind_defaults but simply seen_default. So I think we can eliminate all of this, here's the full version I'd recommend (which passes all tests): https://gist.github.com/carljm/6779e601d04e0b1135444b4e0263e281

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I agree, thank you for the suggestion.

top_kind = kind

if kind in (_POSITIONAL_ONLY, _POSITIONAL_OR_KEYWORD):
Expand Down
40 changes: 34 additions & 6 deletions Lib/test/test_inspect.py
Original file line number Diff line number Diff line change
Expand Up @@ -2462,18 +2462,43 @@ def test_signature_object(self):
self.assertEqual(str(S()), '()')
self.assertEqual(repr(S().parameters), 'mappingproxy(OrderedDict())')

def test(po, pk, pod=42, pkd=100, *args, ko, **kwargs):
def test(po, /, pk, pkd=100, *args, ko, kod=10, **kwargs):
pass

sig = inspect.signature(test)
po = sig.parameters['po'].replace(kind=P.POSITIONAL_ONLY)
pod = sig.parameters['pod'].replace(kind=P.POSITIONAL_ONLY)
self.assertTrue(repr(sig).startswith('<Signature'))
self.assertTrue('(po, /, pk' in repr(sig))

# We need two functions, because it is impossible to represent
# all param kinds in a single one.
def test2(pod=42, /):
pass

sig2 = inspect.signature(test2)
self.assertTrue(repr(sig2).startswith('<Signature'))
self.assertTrue('(pod=42, /)' in repr(sig2))

po = sig.parameters['po']
pod = sig2.parameters['pod']
pk = sig.parameters['pk']
pkd = sig.parameters['pkd']
args = sig.parameters['args']
ko = sig.parameters['ko']
kod = sig.parameters['kod']
kwargs = sig.parameters['kwargs']

S((po, pk, args, ko, kwargs))
S((po, pk, ko, kod))
S((po, pod, ko))
S((po, pod, kod))
S((pod, ko, kod))
S((pod, kod))
S((pod, args, kod, kwargs))
# keyword-only parameters without default values
# can follow keyword-only parameters with default values:
S((kod, ko))
S((kod, ko, kwargs))
S((args, kod, ko))

with self.assertRaisesRegex(ValueError, 'wrong parameter order'):
S((pk, po, args, ko, kwargs))
Expand All @@ -2494,15 +2519,18 @@ def test(po, pk, pod=42, pkd=100, *args, ko, **kwargs):
with self.assertRaisesRegex(ValueError, 'follows default argument'):
S((pod, po))

with self.assertRaisesRegex(ValueError, 'follows default argument'):
S((pod, pk))

with self.assertRaisesRegex(ValueError, 'follows default argument'):
S((po, pod, pk))

with self.assertRaisesRegex(ValueError, 'follows default argument'):
S((po, pkd, pk))

with self.assertRaisesRegex(ValueError, 'follows default argument'):
S((pkd, pk))

self.assertTrue(repr(sig).startswith('<Signature'))
self.assertTrue('(po, pk' in repr(sig))

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
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Now creating :class:`inspect.Signature` objects with positional-only
parameter with a default followed by a positional-or-keyword parameter
without one is impossible.