Skip to content

Preserve keyword-only defaults in namespaced_function and alias_function - #70009

Closed
eeshsaxena wants to merge 2 commits into
saltstack:3006.xfrom
eeshsaxena:fix/functools-kwdefaults
Closed

Preserve keyword-only defaults in namespaced_function and alias_function#70009
eeshsaxena wants to merge 2 commits into
saltstack:3006.xfrom
eeshsaxena:fix/functools-kwdefaults

Conversation

@eeshsaxena

Copy link
Copy Markdown

What does this PR do?

Fixes #69901.

salt.utils.functools.namespaced_function and alias_function both rebuild a function with types.FunctionType, passing argdefs=fn.__defaults__ but nothing for __kwdefaults__. The constructor has no parameter for keyword-only defaults, so the copy loses them. Calling the copy without those keyword arguments then raises TypeError even though the original had defaults:

from salt.utils.functools import namespaced_function

def foo(bar=None, *_, baz=None):
    pass

namespaced_function(foo, globals())()
# TypeError: foo() missing 1 required keyword-only argument: 'baz'

Setting __kwdefaults__ on the new function object in both helpers restores the defaults.

Tests

Added tests/pytests/unit/utils/test_functools.py covering both helpers: each asserts __kwdefaults__ is carried over and that the copy is callable without the keyword-only argument. Both assertions fail against the current code and pass with the change.

I could not run the full salt suite on my machine (Windows, no salt runtime), so I verified by executing the patched salt/utils/functools.py directly with the two salt-internal imports stubbed; both copies keep __kwdefaults__ and are callable. The new unit test is standard and should run cleanly in CI.

Changelog

Added changelog/69901.fixed.md.

@eeshsaxena
eeshsaxena requested a review from a team as a code owner August 11, 2026 12:04
@twangboy twangboy added the test:full Run the full test suite label Aug 11, 2026

@twangboy twangboy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please make this against the earliest branch where the bug exists. Most likely 3006.x

@eeshsaxena
eeshsaxena force-pushed the fix/functools-kwdefaults branch from dad776a to e88479c Compare August 11, 2026 18:24
@eeshsaxena
eeshsaxena changed the base branch from master to 3006.x August 11, 2026 18:24
@eeshsaxena

Copy link
Copy Markdown
Author

Retargeted to 3006.x and rebased. I confirmed the bug is present there too: namespaced_function/alias_function build the copy with types.FunctionType, which carries __defaults__ but not __kwdefaults__, so keyword-only defaults were dropped on that branch as well. The fix and unit tests now sit on top of 3006.x.

types.FunctionType does not carry __kwdefaults__, so copying a function via
namespaced_function or alias_function dropped keyword-only argument defaults.
Calling the copy without those keyword arguments then raised TypeError even
though the original had defaults. Copy __kwdefaults__ across explicitly and
cover both helpers with unit tests.
@twangboy
twangboy force-pushed the fix/functools-kwdefaults branch from e88479c to 89be5e9 Compare August 13, 2026 16:25
twangboy
twangboy previously approved these changes Aug 13, 2026
@eeshsaxena

Copy link
Copy Markdown
Author

Hi! Gentle nudge on this one whenever you have some bandwidth. It's a small, self-contained fix (Preserve keyword-only defaults in namespaced_function and alias_function), and it's currently mergeable with no conflicts. No urgency at all, and I'm happy to make any changes you'd like. Thanks for maintaining salt!

@twangboy

Copy link
Copy Markdown
Contributor

Looks like it's failing pre-commit and lint.

@eeshsaxena

Copy link
Copy Markdown
Author

Thanks for the heads up. I'll run pre-commit locally, clear the lint failures, and push a clean commit.

black wanted the alias_function() call on one line, and pylint's E1102
fires on the dynamically-built clone/alias, so the two call sites carry a
targeted not-callable disable.
@lkubb

lkubb commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

This is a duplicate of my PR btw #69902

@eeshsaxena

Copy link
Copy Markdown
Author

Ah you're right, #69902 is older and covers the same ground. I'll close this in favour of yours. If the keyword-only defaults regression test here is useful feel free to lift it, otherwise no worries.

@eeshsaxena eeshsaxena closed this Aug 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: namespaced_function/alias_function drop kwdefaults

3 participants