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

unnecessary-lambda false positive (expression variable reassigned) #8192

Open
belm0 opened this issue Feb 6, 2023 · 2 comments
Open

unnecessary-lambda false positive (expression variable reassigned) #8192

belm0 opened this issue Feb 6, 2023 · 2 comments
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code Needs PR This issue is accepted, sufficiently specified and now needs an implementation

Comments

@belm0
Copy link
Contributor

belm0 commented Feb 6, 2023

Bug description

pylint unnecessary-lambda doesn't seem to understand that reassignment of a variable containing a class instance affects bound methods can necessitate use of a lambda:

def test_foo():
    foo = {'hello': 42}
    some_mock = unittest.mock.Mock()
    some_mock.bar.side_effect = lambda k: foo.get(k)  # warning: unnecessary-lambda
    assert some_mock.bar('hello') == 42
    foo = {'goodbye': 0}
    assert some_mock.bar('goodbye') == 0

Configuration

No response

Command used

pylint test.py

Pylint output

test.py:50:32: W0108: Lambda may not be necessary (unnecessary-lambda)

Expected behavior

no warning, since the variable holding the class instance (in this case, a dict) used in the lambda expression is reassigned locally

Pylint version

pylint 2.15.5
astroid 2.12.12
Python 3.8.12 (default, Sep 23 2021, 08:42:37)
[Clang 12.0.0 (clang-1200.0.32.29)]

OS / Environment

No response

Additional dependencies

No response

@belm0 belm0 added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Feb 6, 2023
@clavedeluna clavedeluna added False Positive 🦟 A message is emitted but nothing is wrong with the code 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 Feb 6, 2023
@nickdrozd
Copy link
Contributor

This is an interesting one. Actually it has nothing to with class attributes. Instead, the issue is when variables are looked up.

I suppose Pylint is suggesting that lambda x: foo.get(x) should be replaced with just foo.get. But with the lambda expression, the value of foo is not determined until the lambda expression is called, whereas with foo.get the value of foo is determined immediately. By waiting until call time, the lambda might get different values of foo, or no value at all. Consider:

f = lambda x: foo.get(x)  # false positive unnecessary-lambda

# an object is created and given the name 'foo'
foo = {1: 2}
assert f(1) == 2

# a new object is created and given the name 'foo'; first object is lost
foo = {1: 3}
assert f(1) == 3

# the name 'foo' is deleted; second object is lost
del foo

assert f(1) == 3  # NameError: name 'foo' is not defined

Changing the lambda expression to a full function definition does not change this behavior.

The current unnecessary-lambda warning is an error, since it suggests making a bad change. But I wonder if Pylint should issue some kind of warning for these situations where a function refers to a variable outside of its definition and then that variable is reassigned. This behavior is a little unnerving, and I'm sure somebody has encountered problems because of it.

@belm0
Copy link
Contributor Author

belm0 commented Feb 6, 2023

The message for unnecessary-lambda is the tentative, "Lambda may not be necessary", but generally I'd like pylint to try harder to avoid false positives, else remove the check.

In this case, I wonder if it would be hard for the checker to suppress the error if any variable referenced in the expression is reassigned.

@belm0 belm0 changed the title unnecessary-lambda false positive (class instance reassigned) unnecessary-lambda false positive (expression variable reassigned) Feb 6, 2023
charliermarsh pushed a commit to astral-sh/ruff that referenced this issue Oct 20, 2023
This is my first PR and I'm new at rust, so feel free to ask me to
rewrite everything if needed ;)

The rule must be called after deferred lambas have been visited because
of the last check (whether the lambda parameters are used in the body of
the function that's being called). I didn't know where to do it, so I
did what I could to be able to work on the rule itself:

 - added a `ruff_linter::checkers::ast::analyze::lambda` module
 - build a vec of visited lambdas in `visit_deferred_lambdas`
 - call `analyze::lambda` on the vec after they all have been visited
 
Building that vec of visited lambdas was necessary so that bindings
could be properly resolved in the case of nested lambdas.

Note that there is an open issue in pylint for some false positives, do
we need to fix that before merging the rule?
pylint-dev/pylint#8192

Also, I did not provide any fixes (yet), maybe we want do avoid merging
new rules without fixes?

## Summary

Checks for lambdas whose body is a function call on the same arguments
as the lambda itself.

### Bad

```python
df.apply(lambda x: str(x))
```

### Good

```python
df.apply(str)
```

## Test Plan

Added unit test and snapshot.
Manually compared pylint and ruff output on pylint's test cases.

## References

- [pylint
documentation](https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/unnecessary-lambda.html)
- [pylint
implementation](https://github.com/pylint-dev/pylint/blob/main/pylint/checkers/base/basic_checker.py#L521-L587)
 - #970
nickdrozd added a commit to nickdrozd/pylint that referenced this issue Sep 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
Development

No branches or pull requests

3 participants