Add new checker `kwarg-superseded-by-positional-arg` and fix a false positive #8644

mbyrnepr2 · 2023-05-01T11:22:38Z

Type of Changes

	Type
✓	🐛 Bug fix
✓	✨ New feature
	🔨 Refactoring
✓	📜 Docs

Description

Add a new checker kwarg-superseded-by-positional-arg to warn when a function is called 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.

This prevents a false positive, in the above scenario, for redundant-keyword-arg.

Closes #8558

codecov · 2023-05-01T12:28:27Z

Codecov Report

Merging #8644 (332363d) into main (8776ba0) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8644   +/-   ##
=======================================
  Coverage   95.80%   95.80%           
=======================================
  Files         172      172           
  Lines       18301    18301           
=======================================
  Hits        17534    17534           
  Misses        767      767

Impacted Files	Coverage Δ
pylint/checkers/typecheck.py	`96.45% <100.00%> (+<0.01%)`	⬆️

... and 3 files with indirect coverage changes

jacobtylerwalls · 2023-05-06T15:06:34Z

tests/functional/a/arguments.py

+def name2(apple, /, orange, **kwargs):
+    """
+    Positional-only parameter with positional-or-keyword parameter and `**kwargs`.
+    """
+
+
+# +1:[redundant-keyword-arg]
+name2("Red apple", "Green pear", apple="Green apple", orange="Yellow pear")


Nice. Wondering why the oranges are actually pears?

jacobtylerwalls · 2023-05-06T15:12:10Z

pylint/checkers/typecheck.py

+            # Skip if `keyword` is the same name as a positional-only-parameter
+            # and a `**kwargs` parameter exists.
+            if called.args.kwarg and keyword in [
+                arg.name for arg in called.args.posonlyargs
+            ]:
+                continue


What do you think about a followup PR for 3.0 that emits a new message for this scenario? The code works, but it's pretty suspect (it's more likely a user made a mistake than a user is trying to call a signature that intentionally uses this weird semantics).

In that case, we could leave the linked issue open until we create the "lower severity message" it asks for.

I'm open to that. So for now I can use #Refs instead of #Closes in the fragment and we can separately address adding the lower-severity message in a follow-up.

I think your last comment on the issue made me think you were leaning against the idea of adding a new message and were considering this primarily as a false positive @jacobtylerwalls but happy to leave the issue open anyway until we reach a conclusion.

I think your last comment on the issue made me think you were leaning against the idea of adding a new message and were considering this primarily as a false positive @jacobtylerwalls but happy to leave the issue open anyway until we reach a conclusion.

I was extraordinarily indecisive!

But on second thought, yeah, I think a new message would be good. This is such an edge case, though, it doesn't need to take a high priority.

What would the new message be ? It feels like this should be a warning, the value in kwargs is misleading (is it going to be superseded by the one in args ?

My idea was that it needs to be a lower severity message than redundant-keyword-arg because it's a valid language feature, as opposed to something that will raise TypeError when tried, like the other use cases for redundant-keyword-arg.

However, it's a very, very unlikely language feature, and more likely that someone just wrote a bad call to a function that didn't have the kind of signature shown in the test cases. We could in that case emit a lower-severity info message that's like "hey, are you sure you're actually dealing with a function that does this strange thing?"

Maybe hidden-kwarg?

redundant-keyword-arg would describes both the error and the warning well imo. In fact redundant-keyword-arg describes the warning better, while the error would be better described by multiple-value-for-argument (Like the text for the type error raised by python in this case: got multiple values for argument 'x'). Our message name need to be unique and retro-compatible so a possible solution would be to rename redundant-keyword-arg => multiple-value-for-argument, and create redundant-keyword-argument. That's convoluted and confusing in particular for redundant-keyword-arg / redundant-keyword-argument though. Maybe we can live with redundant-keyword-arg for both ?

There's good reasons for either of the two approaches now mentioned.
How about we: Revert the code-change in this merge-request (i.e. we emit redundant-keyword-arg for this case) and merge the rest (the test-cases and the updated test output) which can be useful if we re-visit this to add a new low-severity message?

We could also name the warning kwargs-hidden-by-positional-args or hidden-kwarg or something else (?) like @jacobtylerwalls suggested so there's no confusion between the two. (And we can rename the error anyway, or not)

Pierre-Sassoulas · 2023-05-06T15:18:20Z

Moving to 2.17.5 because 2.17.4 is already generated in #8664

…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

… a new Pylint message in a separate merge request.

Pierre-Sassoulas

Let's discuss the name in the next MR then.

mbyrnepr2 · 2023-05-08T19:29:16Z

I was just about to update with the new checker :D.

Pierre-Sassoulas · 2023-05-08T19:29:58Z

If we want to be able to backport we need to separate the false positive fix and the new messages I think. But this isn't an urgent fix so let's not backport.

…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

mbyrnepr2 · 2023-05-08T20:23:16Z

tests/functional/a/arguments_positional_only.py

Moved the contents of this module into tests/functional/a/arguments.py since Python 3.8 is the minimum version on the main branch now; the original reason for having this module was to let the positional-only argument syntax be used which required python 3.8 minimum as configured in the arguments_positional_only.rc file.

mbyrnepr2 · 2023-05-08T21:20:16Z

doc/data/messages/h/hidden-kwarg/bad.py

Shamelessly stealing Jacob's examples :D

tests/functional/p/positional_only_arguments_expected.py

tests/functional/h/hidden_kwarg.py

Pierre-Sassoulas

Logic look good 👍 I made suggestion regarding the message name and content.

Pierre-Sassoulas · 2023-05-09T17:26:14Z

pylint/checkers/typecheck.py

+        "The keyword argument %r will be added to the keyword variadic "
+        "parameter dictionary %r since it has the same name as a positional-only parameter",


Suggested change

"The keyword argument %r will be added to the keyword variadic "

"parameter dictionary %r since it has the same name as a positional-only parameter",

"%r will be included in %r since a positional-only parameter with this name already exists",

I think "keyword variadic parameter" is likely to confuse, and I would favor a shorter message.

Pierre-Sassoulas · 2023-05-09T18:09:12Z

pylint/checkers/typecheck.py

+    "W1117": (
+        "The keyword argument %r will be added to the keyword variadic "
+        "parameter dictionary %r since it has the same name as a positional-only parameter",
+        "hidden-kwarg",


Suggested change

"hidden-kwarg",

"duplicated-arg-hidden-in-kwarg",

Suggested change

"hidden-kwarg",

"duplicate-keyword-argument",

Suggested change

"hidden-kwarg",

"kwarg-hidden-by-posarg",

I'm trying to come up with something more descriptive. We have https://pylint.pycqa.org/en/latest/user_guide/messages/error/duplicate-argument-name.html that is close.

Thanks for thinking along on this @Pierre-Sassoulas. I found it difficult to think of an accurate name :D.

I think hidden may be a bit confusing as a term.

Regarding duplicate-keyword-argument; it suggests to me 2 or more keywords were passed to the function call explicitly which isn't the case - also the message is specifically emitted when positional-only parameters are present, so I thought it would be good to make that explicit.

What do you think of this one:
same-keyword-as-positional-with-kwargs (inspired by the name of a unittest in cpython)
Bit of a mouthful but I like that it uses the words positional and kwargs which are required for the message to be emitted.

Maybe "kwarg-overridden-by-positional-arg", "kwarg-superseded-by-positional-arg" ?

I'd say the latter suggestion, kwarg-superseded-by-positional-arg, is more accurate; let's try it out.

…arg-superseded-by-positional-arg`. Co-authored-by: Pierre Sassoulas <[email protected]>

Co-authored-by: Pierre Sassoulas <[email protected]>

pylint/checkers/typecheck.py

Pierre-Sassoulas · 2023-05-14T20:24:40Z

Great new check, the doc's bad/good example is especially good.

github-actions · 2023-05-14T21:08:02Z

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 332363d

Pierre-Sassoulas added False Positive 🦟 A message is emitted but nothing is wrong with the code backport maintenance/3.3.x labels May 1, 2023

This comment has been minimized.

Sign in to view

mbyrnepr2 marked this pull request as ready for review May 1, 2023 12:29

mbyrnepr2 requested a review from jacobtylerwalls May 1, 2023 15:47

jacobtylerwalls previously approved these changes May 6, 2023

View reviewed changes

jacobtylerwalls added this to the 2.17.4 milestone May 6, 2023

Pierre-Sassoulas modified the milestones: 2.17.4, 2.17.5 May 6, 2023

mbyrnepr2 dismissed jacobtylerwalls’s stale review via e428661 May 6, 2023 19:15

mbyrnepr2 force-pushed the 8558_redundant_keyword_arg branch from cf6e1ef to 82068ab Compare May 6, 2023 19:32

This comment has been minimized.

Sign in to view

jacobtylerwalls previously approved these changes May 6, 2023

View reviewed changes

mbyrnepr2 added 4 commits May 8, 2023 21:27

Fix a false positive for redundant-keyword-arg when a function, w…

00be90f

…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

Fix unittest typo.

716bd12

Update fragment to use #Refs# instead of #Closes since we can add…

a950c0e

… a new Pylint message in a separate merge request.

Update unittest expected output.

7afec3e

mbyrnepr2 force-pushed the 8558_redundant_keyword_arg branch from 82068ab to 7afec3e Compare May 8, 2023 19:28

Pierre-Sassoulas previously approved these changes May 8, 2023

View reviewed changes

mbyrnepr2 marked this pull request as draft May 8, 2023 19:30

Pierre-Sassoulas removed the backport maintenance/3.3.x label May 8, 2023

Pierre-Sassoulas modified the milestones: 2.17.5, 3.0.0b1 May 8, 2023

Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label May 8, 2023

mbyrnepr2 marked this pull request as ready for review May 8, 2023 19:31

mbyrnepr2 marked this pull request as draft May 8, 2023 19:32

mbyrnepr2 dismissed stale reviews from Pierre-Sassoulas and jacobtylerwalls via 12efb00 May 8, 2023 20:19

mbyrnepr2 changed the title ~~Fix a false positive for redundant-keyword-arg~~ Add new checker hidden-kwarg May 8, 2023

mbyrnepr2 commented May 8, 2023

View reviewed changes

Add documentation examples.

7fb7059

mbyrnepr2 commented May 8, 2023

View reviewed changes

mbyrnepr2 marked this pull request as ready for review May 8, 2023 21:26

This comment has been minimized.

Sign in to view

jacobtylerwalls reviewed May 9, 2023

View reviewed changes

tests/functional/p/positional_only_arguments_expected.py Outdated Show resolved Hide resolved

tests/functional/h/hidden_kwarg.py Outdated Show resolved Hide resolved

Add a further functional test.

7786935

jacobtylerwalls previously approved these changes May 9, 2023

View reviewed changes

Pierre-Sassoulas reviewed May 9, 2023

View reviewed changes

mbyrnepr2 and others added 2 commits May 14, 2023 22:10

A more informative naming - rename checker from hidden-kwarg to `kw…

08eceb9

…arg-superseded-by-positional-arg`. Co-authored-by: Pierre Sassoulas <[email protected]>

Reword message description.

0e3377e

Co-authored-by: Pierre Sassoulas <[email protected]>

mbyrnepr2 dismissed jacobtylerwalls’s stale review via 0e3377e May 14, 2023 20:12

mbyrnepr2 changed the title ~~Add new checker hidden-kwarg~~ Add new checker kwarg-superseded-by-positional-arg and fix a false positive May 14, 2023

mbyrnepr2 commented May 14, 2023

View reviewed changes

pylint/checkers/typecheck.py Outdated Show resolved Hide resolved

Update pylint/checkers/typecheck.py

332363d

Pierre-Sassoulas approved these changes May 14, 2023

View reviewed changes

mbyrnepr2 merged commit d392ea5 into pylint-dev:main May 16, 2023

mbyrnepr2 deleted the 8558_redundant_keyword_arg branch May 16, 2023 08:34

jacobtylerwalls modified the milestones: 3.0.0b1, 3.0.0a7 Jul 2, 2023

		"The keyword argument %r will be added to the keyword variadic "
		"parameter dictionary %r since it has the same name as a positional-only parameter",

	"The keyword argument %r will be added to the keyword variadic "
	"parameter dictionary %r since it has the same name as a positional-only parameter",
	"%r will be included in %r since a positional-only parameter with this name already exists",

Uh oh!

Add new checker kwarg-superseded-by-positional-arg and fix a false positive #8644

Add new checker kwarg-superseded-by-positional-arg and fix a false positive #8644

Uh oh!

Conversation

mbyrnepr2 commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of Changes

Description

Uh oh!

This comment has been minimized.

codecov bot commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacobtylerwalls May 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas commented May 6, 2023

Uh oh!

This comment has been minimized.

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

mbyrnepr2 commented May 8, 2023

Uh oh!

Pierre-Sassoulas commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Pierre-Sassoulas commented May 14, 2023

Uh oh!

github-actions bot commented May 14, 2023

Uh oh!

Uh oh!

Add new checker `kwarg-superseded-by-positional-arg` and fix a false positive #8644

Add new checker `kwarg-superseded-by-positional-arg` and fix a false positive #8644

mbyrnepr2 commented May 1, 2023 •

edited

Loading

codecov bot commented May 1, 2023 •

edited

Loading

jacobtylerwalls May 6, 2023 •

edited

Loading

Pierre-Sassoulas commented May 8, 2023 •

edited

Loading