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

Fix false positive for unnecessary-lambda with kwargs in the call but not the lambda #9149

Merged
merged 1 commit into from Oct 29, 2023

Conversation

clemux
Copy link
Contributor

@clemux clemux commented Oct 15, 2023

I might have missed something because I don't understand why call_site.keyword_arguments has been used here instead of simply comparing node.args.kwarg with call.keywords. Maybe this is not the right way to fix the issue.

        if call.keywords:
            # Look for additional keyword arguments that are not part
            # of the lambda's signature
            lambda_kwargs = {keyword.name for keyword in node.args.defaults}
            if len(lambda_kwargs) != len(call_site.keyword_arguments):
                # Different lengths, so probably not identical
                return
            if set(call_site.keyword_arguments).difference(lambda_kwargs):
                return

As far as I can tell, lambda_kwargs here is always empty because of this condition at the beginning of the check

        if node.args.defaults:
            return

Also, call_site.keyword_arguments is empty in the case of the example I added to the tests, with causes the false positive.

Type of Changes

Type
🐛 Bug fix

Description

Closes #9148

@github-actions

This comment has been minimized.

@clemux clemux marked this pull request as draft October 15, 2023 03:04
@clemux clemux marked this pull request as ready for review October 15, 2023 12:51
@codecov
Copy link

codecov bot commented Oct 15, 2023

Codecov Report

Merging #9149 (e5757d0) into main (0796dfa) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #9149      +/-   ##
==========================================
+ Coverage   95.76%   95.78%   +0.01%     
==========================================
  Files         173      173              
  Lines       18680    18675       -5     
==========================================
- Hits        17889    17887       -2     
+ Misses        791      788       -3     
Files Coverage Δ
pylint/checkers/base/basic_checker.py 98.08% <100.00%> (+0.21%) ⬆️

... and 1 file with indirect coverage changes

@github-actions

This comment has been minimized.

This simplifies the check for the call having kwargs but not the lambda.
@github-actions
Copy link
Contributor

🤖 Effect of this PR on checked open source code: 🤖

Effect on pandas:
The following messages are no longer emitted:

  1. unnecessary-lambda:
    Lambda may not be necessary
    https://github.com/pandas-dev/pandas/blob/c9a98f0f0fe3a1ff2ce549b2f2c7551cc9afc58a/pandas/tests/window/test_base_indexer.py#L178
  2. unnecessary-lambda:
    Lambda may not be necessary
    https://github.com/pandas-dev/pandas/blob/c9a98f0f0fe3a1ff2ce549b2f2c7551cc9afc58a/pandas/tests/window/test_base_indexer.py#L188
  3. unnecessary-lambda:
    Lambda may not be necessary
    https://github.com/pandas-dev/pandas/blob/c9a98f0f0fe3a1ff2ce549b2f2c7551cc9afc58a/pandas/tests/io/parser/test_parse_dates.py#L950
  4. unnecessary-lambda:
    Lambda may not be necessary
    https://github.com/pandas-dev/pandas/blob/c9a98f0f0fe3a1ff2ce549b2f2c7551cc9afc58a/pandas/tests/io/parser/test_parse_dates.py#L974

This comment was generated for commit e5757d0

@Pierre-Sassoulas Pierre-Sassoulas added False Positive 🦟 A message is emitted but nothing is wrong with the code backport maintenance/3.2.x labels Oct 29, 2023
@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.0.3 milestone Oct 29, 2023
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Thank you for opening an issue and fixing the problem directly ! Not only do the primers looks great, but also you simplified the code and actually removed more lines than you added even though you added a changelog and new tests. Great first contribution !

@Pierre-Sassoulas Pierre-Sassoulas merged commit 2289c71 into pylint-dev:main Oct 29, 2023
48 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 29, 2023
This simplifies the check for the call having kwargs but not the lambda.

(cherry picked from commit 2289c71)
Pierre-Sassoulas pushed a commit that referenced this pull request Oct 29, 2023
This simplifies the check for the call having kwargs but not the lambda.

(cherry picked from commit 2289c71)

Co-authored-by: Clément Schreiner <clement@mux.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive: unnecessary lambda with kwargs in the call
2 participants