Skip to content
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

Lower severity message than redundant-keyword-arg for calling distinct positional-only arg and variadic keyword arg with same name #8558

Closed
jacobtylerwalls opened this issue Apr 9, 2023 · 8 comments · Fixed by #8644 or #9093
Labels
Bug 🪲 Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Milestone

Comments

@jacobtylerwalls
Copy link
Member

Current problem

When writing a function, you can defeat the point of a PEP 570 positional-only argument (to the left of /) if:

  • it has a default
  • later, there is a variadic positional-or-keyword argument e.g. **kwargs

GOOD example (adapted from PEP 570, "Semantic Corner Case"):

def foo(name, /, **kwds):
    return name

See that name is actually positional-only:

>>> foo(name="Jacob")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: foo() missing 1 required positional argument: 'name'

BAD example:

def foo(name="Sarah", /, **kwds):
    return name

See that name is completely ignored:

>>> foo(name="Jacob")
'Sarah'

Desired solution

I suggest a new warning e.g. defeated-positional-only-argument

Additional context

Inspired by #8555

PEP 570

@jacobtylerwalls jacobtylerwalls added Enhancement ✨ Improvement to a component Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Apr 9, 2023
@mbyrnepr2 mbyrnepr2 added Needs PR This issue is accepted, sufficiently specified and now needs an implementation and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Apr 9, 2023
@mbyrnepr2
Copy link
Member

The idea for this checker also crossed my mind while pondering #8555 but I dismissed the idea as noise if it is expected Python behaviour. But now that you are also having the same thought then I think there must be some value to it!

@Pierre-Sassoulas
Copy link
Member

I'm on mobile so I can't link to the proper documentation but it seems close to what 'keyword arg before vararg' (w1113) does.

@jacobtylerwalls
Copy link
Member Author

Interestingly, if you change the **kwargs to *args, Python complains. I'm starting to think this is a bug in the PEP 570 implementation. @mbyrnepr2 do you want first dibs writing the issue? :D

def foo(name="Sarah", /, *args):
  print(name)

foo(name="Jacob")
TypeError: foo() got some positional-only arguments passed as keyword arguments: 'name'

I'm on mobile so I can't link to the proper documentation but it seems close to what 'keyword arg before vararg' (w1113) does.

Yes, we could consider raising that very message, except that the argument we're talking about is supposedly not a keyword arg.

@jacobtylerwalls
Copy link
Member Author

Even more interestingly, even inspect.signature doesn't like it when you do the bad stuff:

>>> import inspect
>>> def foo(name="Sarah", /, **kwds):
...     return name
... 
>>> inspect.signature(foo).bind(name="Jacob")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/inspect.py", line 3211, in bind
    return self._bind(args, kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/inspect.py", line 3107, in _bind
    raise TypeError(msg) from None
TypeError: 'name' parameter is positional only, but was passed as a keyword
>>> foo(name="Jacob")
'Sarah'

@mbyrnepr2
Copy link
Member

Thanks for looking into it @jacobtylerwalls; please feel free to go ahead with opening an issue since you had the insight and idea to do so. Also I'm tired as heck today :D

@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented Apr 9, 2023

I'm changing my mind now; I see there's no issue in CPython. Mark's comments on #8556 were helpful, as was python/cpython#87106.

I want to suggest instead warning on the problematic call, not on the function definition itself. We should probably just craft a pylint warning for the TypeError mentioned at the top of #8558 (comment). I'll update the title of this issue when I have more time.

@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented Apr 10, 2023

Okay, finally figured out what to suggest here. #8556 makes this situation better; we should merge it. Then, we still have two independent problems:

     1  # pylint: disable=missing-function-docstring, missing-module-docstring
     2  def required_positional_with_var_kwargs(a, /, **_kwargs):
     3      return a
     4
     5  def optional_positional_with_var_kwargs(a=1, /, **_kwargs):
     6      return a
     7
     8
     9  # 3 of these 4 succeed, but it's a bit smelly
    10  required_positional_with_var_kwargs(42, a=43)  # returns 42
    11  required_positional_with_var_kwargs(a=43)  # TypeError: should raise no-value-for-parameter
    12
    13  optional_positional_with_var_kwargs(42, a=43)  # returns 42
    14  optional_positional_with_var_kwargs(a=43)  # returns 1

Currently only gives:

************* Module a
a.py:10:0: E1124: Argument 'a' passed by position and keyword in function call (redundant-keyword-arg)
a.py:13:0: E1124: Argument 'a' passed by position and keyword in function call (redundant-keyword-arg)

------------------------------------------------------------------
Your code has been rated at 0.00/10 (previous run: 0.00/10, +0.00)

1
The redundant-keyword-arg is misleading: it should be some new message, since they're not redundant, they're actually two different arguments, and it should also be emitted on line 14. Something like same-named-positional-and-keyword-args?

2

We should raise no-value-for-parameter on line 13. I'll open a new issue for that, since it's more straightforward. EDIT: done here: #8559.

This card can be for the first suggestion. Retitling.

@jacobtylerwalls jacobtylerwalls changed the title Warn when positional-only arguments defeated by variadic keyword arguments Lower severity message than redundant-keyword-arg for calling distinct positional-only arg and variadic keyword arg with same name Apr 10, 2023
@jacobtylerwalls
Copy link
Member Author

Enh, or we could just not emit any message at all for 1. One way or another, it's a bug, not an enhancement.

@jacobtylerwalls jacobtylerwalls added Bug 🪲 and removed Enhancement ✨ Improvement to a component labels Apr 10, 2023
mbyrnepr2 added a commit to mbyrnepr2/pylint that referenced this issue May 1, 2023
…ith a keyword-only-parameter and ``**kwargs``, is called with a positional argument and a keyword argument where the keyword argument has the same name as the positional-only-parameter.

Closes pylint-dev#8558
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.17.5 milestone May 6, 2023
mbyrnepr2 added a commit to mbyrnepr2/pylint that referenced this issue May 8, 2023
…ith a keyword-only-parameter and ``**kwargs``, is called with a positional argument and a keyword argument where the keyword argument has the same name as the positional-only-parameter.

Closes pylint-dev#8558
mbyrnepr2 added a commit to mbyrnepr2/pylint that referenced this issue May 8, 2023
…with a keyword argument which shares a name with a positional-only parameter and the function contains a keyword variadic parameter dictionary. It may be surprising behaviour when the keyword argument is added to the keyword variadic parameter dictionary.

Closes pylint-dev#8558
@jacobtylerwalls jacobtylerwalls modified the milestones: 2.17.5, 3.0.0b1 May 12, 2023
mbyrnepr2 added a commit that referenced this issue May 16, 2023
…positive (#8644)

* Fix a false positive for ``redundant-keyword-arg`` when a function, with a keyword-only-parameter and ``**kwargs``, is called with a positional argument and a keyword argument where the keyword argument has the same name as the positional-only-parameter.
* Add new checker ``kwarg-superseded-by-positional-arg`` which emits a warning message for the above scenario.

Closes #8558

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
@jacobtylerwalls jacobtylerwalls modified the milestones: 3.0.0b1, 3.0.0a7 Jul 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
3 participants