From 4082a7c5e95795240dc3257a57b969516b4deb0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Rzepecki?= Date: Mon, 9 Nov 2026 19:19:18 +0100 Subject: [PATCH] fix: record positional optional params and **kwargs correctly Parameters with a default were only looked up in kwargs, so when passed positionally they were recorded as their default value, and the positional values spilled over into *args instead. E.g. for f(self, a, b=None) called as f('A', 'B'), b was recorded as None. **kwargs was recorded as the full keyword argument dict, including arguments bound to named parameters. It now only contains the ones Python would actually put there. In particular, keyword arguments named like positional-only parameters now go to **kwargs rather than being recorded as the value of the positional-only parameter. Assisted-by: Claude:claude-opus-5-5 --- _appmap/event.py | 33 +++++--- _appmap/test/data/params.py | 7 ++ _appmap/test/test_params.py | 158 ++++++++++++++++++++++++++++++++++++ 3 files changed, 185 insertions(+), 13 deletions(-) diff --git a/_appmap/event.py b/_appmap/event.py index 9dc446ea..c284e6e7 100644 --- a/_appmap/event.py +++ b/_appmap/event.py @@ -156,10 +156,11 @@ def __repr__(self): class Param: - __slots__ = ["name", "kind", "default", "default_class"] + __slots__ = ["name", "kind", "positional_only", "default", "default_class"] def __init__(self, sigp): self.name = sigp.name + self.positional_only = sigp.kind == Parameter.POSITIONAL_ONLY has_default = sigp.default is not Signature.empty if sigp.kind in (Parameter.POSITIONAL_ONLY, Parameter.POSITIONAL_OR_KEYWORD): self.kind = "opt" if has_default else "req" @@ -275,28 +276,34 @@ def set_params(params, instance, args, kwargs, display_value=False): # we're popping the front repeatedly, lists are better at this args = list(args) + # Names of keyword arguments bound to named parameters. Python + # doesn't include them in **kwargs, so neither do we. + bound = set() for p in params: - if p.kind == "req": - # A 'req' argument can be either keyword or - # positional. - if p.name in kwargs: + if p.kind in ("req", "opt"): + # These can be either keyword or positional, unless + # they're positional-only. + if not p.positional_only and p.name in kwargs: value = kwargs[p.name] + bound.add(p.name) + elif args: + value = args.pop(0) + elif p.kind == "opt": + value = p.default else: - if args: - value = args.pop(0) - else: - continue # required argument missing - elif p.kind == "keyreq": + continue # required argument missing + elif p.kind in ("keyreq", "key"): if p.name in kwargs: value = kwargs[p.name] + bound.add(p.name) + elif p.kind == "key": + value = p.default else: continue # required argument missing - elif p.kind in ("opt", "key"): - value = kwargs.get(p.name, p.default) elif p.kind == "rest": value = tuple(args) elif p.kind == "keyrest": - value = kwargs + value = {k: v for k, v in kwargs.items() if k not in bound} if bound else kwargs else: # If all the parameter types are handled, this # shouldn't ever happen... diff --git a/_appmap/test/data/params.py b/_appmap/test/data/params.py index 7d0f846a..46768b31 100644 --- a/_appmap/test/data/params.py +++ b/_appmap/test/data/params.py @@ -22,6 +22,9 @@ def args_kwargs(self, *args, **kwargs): def with_defaults(self, p1=1, p2=2): return "%s %s" % (p1, p2) + def mixed(self, a, b=None, c="dflt", *args, d, e="e", **kwargs): + return "%s %s %s %s %s %s %s" % (a, b, c, args, d, e, kwargs) + if sys.version_info >= (3, 8): exec( @@ -33,5 +36,9 @@ def positional_only(self, p1, p2, /): def keyword_only(self, *, p1, p2): return '%s %s' % (p1, p2) C.keyword_only = keyword_only + +def positional_only_kwargs(self, a, b="dflt", /, **kwargs): + return '%s %s %s' % (a, b, kwargs) +C.positional_only_kwargs = positional_only_kwargs """ ) diff --git a/_appmap/test/test_params.py b/_appmap/test/test_params.py index ed6deae1..2d87cf10 100644 --- a/_appmap/test/test_params.py +++ b/_appmap/test/test_params.py @@ -321,3 +321,161 @@ def test_keywords_defaults_with_args(self, params): self.assert_parameter( evt, 1, {"class": "builtins.int", "kind": "opt", "name": "p2", "value": "4"} ) + + @pytest.mark.parametrize("params", ["with_defaults"], indirect=True) + def test_defaults_positional(self, params): + evt = params.C().with_defaults(3, 4) + assert len(evt.parameters) == 2 + + self.assert_parameter( + evt, 0, {"class": "builtins.int", "kind": "opt", "name": "p1", "value": "3"} + ) + + self.assert_parameter( + evt, 1, {"class": "builtins.int", "kind": "opt", "name": "p2", "value": "4"} + ) + + @pytest.mark.parametrize("params", ["with_defaults"], indirect=True) + def test_defaults_positional_and_keyword(self, params): + evt = params.C().with_defaults(3, p2=4) + assert len(evt.parameters) == 2 + + self.assert_parameter( + evt, 0, {"class": "builtins.int", "kind": "opt", "name": "p1", "value": "3"} + ) + + self.assert_parameter( + evt, 1, {"class": "builtins.int", "kind": "opt", "name": "p2", "value": "4"} + ) + + +class TestMixedParams(TestMethodBase): + """ + C.mixed has the signature (self, a, b=None, c="dflt", *args, d, e="e", **kwargs), + covering every parameter kind. + """ + + def assert_values(self, evt, expected): + actual = {p["name"]: (p["kind"], p["value"]) for p in evt.parameters} + assert actual == expected + + @pytest.mark.parametrize("params", ["mixed"], indirect=True) + def test_positional_opt(self, params): + evt = params.C().mixed("A", "B", "C", d="D") + self.assert_values( + evt, + { + "a": ("req", "A"), + "b": ("opt", "B"), + "c": ("opt", "C"), + "args": ("rest", "()"), + "d": ("keyreq", "D"), + "e": ("key", "e"), + "kwargs": ("keyrest", "{}"), + }, + ) + + @pytest.mark.parametrize("params", ["mixed"], indirect=True) + def test_partial_positional_opt(self, params): + evt = params.C().mixed("A", "B", d="D") + self.assert_values( + evt, + { + "a": ("req", "A"), + "b": ("opt", "B"), + "c": ("opt", "dflt"), + "args": ("rest", "()"), + "d": ("keyreq", "D"), + "e": ("key", "e"), + "kwargs": ("keyrest", "{}"), + }, + ) + + @pytest.mark.parametrize("params", ["mixed"], indirect=True) + def test_keyword_opt(self, params): + evt = params.C().mixed("A", c="C", d="D") + self.assert_values( + evt, + { + "a": ("req", "A"), + "b": ("opt", "None"), + "c": ("opt", "C"), + "args": ("rest", "()"), + "d": ("keyreq", "D"), + "e": ("key", "e"), + "kwargs": ("keyrest", "{}"), + }, + ) + + @pytest.mark.parametrize("params", ["mixed"], indirect=True) + def test_rest_after_opt(self, params): + evt = params.C().mixed("A", "B", "C", "X", "Y", d="D") + self.assert_values( + evt, + { + "a": ("req", "A"), + "b": ("opt", "B"), + "c": ("opt", "C"), + "args": ("rest", "('X', 'Y')"), + "d": ("keyreq", "D"), + "e": ("key", "e"), + "kwargs": ("keyrest", "{}"), + }, + ) + + @pytest.mark.parametrize("params", ["mixed"], indirect=True) + def test_keyword_only(self, params): + evt = params.C().mixed("A", d="D", e="E", f="F") + self.assert_values( + evt, + { + "a": ("req", "A"), + "b": ("opt", "None"), + "c": ("opt", "dflt"), + "args": ("rest", "()"), + "d": ("keyreq", "D"), + "e": ("key", "E"), + "kwargs": ("keyrest", "{'f': 'F'}"), + }, + ) + + @pytest.mark.parametrize("params", ["mixed"], indirect=True) + def test_keyword_req(self, params): + evt = params.C().mixed(a="A", d="D", f="F") + self.assert_values( + evt, + { + "a": ("req", "A"), + "b": ("opt", "None"), + "c": ("opt", "dflt"), + "args": ("rest", "()"), + "d": ("keyreq", "D"), + "e": ("key", "e"), + "kwargs": ("keyrest", "{'f': 'F'}"), + }, + ) + + @pytest.mark.parametrize("params", ["positional_only_kwargs"], indirect=True) + def test_positional_only_name_in_kwargs(self, params): + # Keyword arguments named like positional-only parameters go to **kwargs + evt = params.C().positional_only_kwargs("A", "B", a="X", b="Y") + self.assert_values( + evt, + { + "a": ("req", "A"), + "b": ("opt", "B"), + "kwargs": ("keyrest", "{'a': 'X', 'b': 'Y'}"), + }, + ) + + @pytest.mark.parametrize("params", ["positional_only_kwargs"], indirect=True) + def test_positional_only_default_name_in_kwargs(self, params): + evt = params.C().positional_only_kwargs("A", b="Y") + self.assert_values( + evt, + { + "a": ("req", "A"), + "b": ("opt", "dflt"), + "kwargs": ("keyrest", "{'b': 'Y'}"), + }, + )