Skip to content

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

Merged
Pierre-Sassoulas merged 1 commit intopylint-dev:mainfrom
clemux:fix_unnecessary_lambda
Oct 29, 2023
Merged

Fix false positive for unnecessary-lambda with kwargs in the call but not the lambda#9149
Pierre-Sassoulas merged 1 commit intopylint-dev:mainfrom
clemux:fix_unnecessary_lambda

Conversation

@clemux
Copy link
Copy Markdown
Contributor

@clemux clemux commented Oct 15, 2023 •
edited
Loading

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

@clemux clemux force-pushed the fix_unnecessary_lambda branch from 2a1c900 to e3c0cc6 Compare October 15, 2023 02:11
@github-actions

This comment has been minimized.

Sign in to view
@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
Copy Markdown

codecov Bot commented Oct 15, 2023 •
edited
Loading

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.

Sign in to view
@clemux
Fix false positive for unnecessary-lambda.
e5757d0 
This simplifies the check for the call having kwargs but not the lambda.
@clemux clemux force-pushed the fix_unnecessary_lambda branch from 1828b94 to e5757d0 Compare October 15, 2023 13:49
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Oct 15, 2023

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

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

Details
  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/4.0.x labels Oct 29, 2023
@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.0.3 milestone Oct 29, 2023
Pierre-Sassoulas
Pierre-Sassoulas approved these changes Oct 29, 2023
View reviewed changes
Copy link
Copy Markdown
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
github-actions Bot pushed a commit that referenced this pull request Oct 29, 2023
@clemux @github-actions
Fix false positive for unnecessary-lambda. (#9149)
f34d1dd 
This simplifies the check for the call having kwargs but not the lambda.

(cherry picked from commit 2289c71)
@github-actions github-actions Bot mentioned this pull request Oct 29, 2023
Merged
Pierre-Sassoulas pushed a commit that referenced this pull request Oct 29, 2023
@github-actions @clemux
Fix false positive for unnecessary-lambda. (#9149) (#9200)
0273db7 
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

Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

Assignees

No one assigned

Labels

Backported False Positive 🦟 A message is emitted but nothing is wrong with the code

Projects

None yet

Milestone

3.0.3

Development

Successfully merging this pull request may close these issues.

False positive: unnecessary lambda with kwargs in the call

2 participants

@clemux @Pierre-Sassoulas