Fix false positives for `unnecessary-dunder-call` in lambdas #9034

badsketch · 2023-09-13T03:10:50Z

Type of Changes

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

Description

Closes #8769

Went through constants.DUNDER_METHODS to try to get a subset of dunders that fit into this exception and called it DUNDER_LAMBDA_EXCEPTIONS. IIUC, constants.DUNDER_PROPERTIES and constants.EXTRA_DUNDER_METHODS are used the pylint.extensions.dunder module, and don't have corresponding operators/functions/methods anyways, so I ignored them.

Looking forward to any feedback 🙂

codecov · 2023-09-13T05:34:10Z

Codecov Report

Merging #9034 (4b8bc68) into main (3f93f1e) will increase coverage by 0.00%.
Report is 8 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9034   +/-   ##
=======================================
  Coverage   95.75%   95.75%           
=======================================
  Files         173      173           
  Lines       18652    18656    +4     
=======================================
+ Hits        17860    17864    +4     
  Misses        792      792

Files Changed	Coverage Δ
pylint/checkers/dunder_methods.py	`100.00% <100.00%> (ø)`
pylint/constants.py	`100.00% <100.00%> (ø)`

Pierre-Sassoulas · 2023-09-13T06:23:51Z

Something's wrong with the primer, could you rebase on upstream/main please ?

badsketch · 2023-09-13T22:00:22Z

Woops, done!

Also looking into the changelog portion in the docs.

Pierre-Sassoulas

Looks pretty good already, great work. I made a surface review, I'll check the list of dunder when I'm not on mobile anymore.

Pierre-Sassoulas · 2023-09-17T07:31:53Z

pylint/constants.py

+
+# C2801 rule exceptions as their corresponding function/method/operator
+# is not valid python syntax in a lambda definition
+DUNDER_LAMBDA_EXCEPTIONS = [


Suggested change

DUNDER_LAMBDA_EXCEPTIONS = [

UNNECESSARY_DUNDER_CALL_LAMBDA_EXCEPTIONS = [

Pierre-Sassoulas · 2023-09-17T07:37:39Z

doc/whatsnew/fragments/8769.bugfix

@@ -0,0 +1,3 @@
+Add special cases of dunder methods that should not trigger ``unnecessary-dunder-call`` if it's in a lambda definition.


Suggested change

Add special cases of dunder methods that should not trigger ``unnecessary-dunder-call`` if it's in a lambda definition.

Dunder methods defined in lambda do not trigger ``unnecessary-dunder-call`` anymore, if they cannot be replaced by the non-dunder call.

Pierre-Sassoulas · 2023-09-17T07:43:35Z

The fail on pypy3.8 seems to be due to the ast not being the same (column not calculated the same way). It's going to requires two functional tests files one for each interpreter. (Maybe we could permit an expected output per interpreter but it's not that much better). Or we can exclude the interpréter from the one file we have.

badsketch · 2023-09-18T00:31:30Z

Thanks for the suggestions!

The fail on pypy3.8 seems to be due to the ast not being the same (column not calculated the same way). It's going to requires two functional tests files one for each interpreter. (Maybe we could permit an expected output per interpreter but it's not that much better). Or we can exclude the interpréter from the one file we have.

I see the test fails for pypy3.8, but succeeds for pypy3.9. I looked into test options like exclude_implementations which only supports "PyPy" as a whole. If there's no way to specify which version of PyPy, I'm guessing our only option is to exclude PyPy entirely for this functional test file? If we try to do two separate files, the file for pypy would pass for pypy3.8, but now fails for pypy3.9. Could I be mistaken?

jacobtylerwalls · 2023-09-18T00:48:41Z

pylint/checkers/dunder_methods.py

+    @staticmethod
+    def is_lambda_rule_exception(ancestor: nodes.NodeNG, node: nodes.NodeNG) -> bool:
+        return (
+            isinstance(ancestor, nodes.Lambda)


We shouldn't backport this, since in 2.x of astroid, FunctionDef inherits from Lambda, see pylint-dev/astroid#2115.

(And I don't think it's important enough to backport a different solution.)

jacobtylerwalls · 2023-09-18T00:53:57Z

I see the test fails for pypy3.8, but succeeds for pypy3.9. I looked into test options like exclude_implementations which only supports "PyPy" as a whole. If there's no way to specify which version of PyPy, I'm guessing our only option is to exclude PyPy entirely for this functional test file? If we try to do two separate files, the file for pypy would pass for pypy3.8, but now fails for pypy3.9. Could I be mistaken?

Right, I think the pragmatic thing to do is just skip PyPy entirely with the exclude_implementations flag. pylint 3.0 won't see many months (single digits for sure) of python 3.8 support anyhow.

…y resulting in failing tests

badsketch · 2023-09-18T03:37:16Z

Gotcha, SGTM

Pierre-Sassoulas · 2023-09-18T11:13:52Z

It's also possible to do a combination of min_version > 3.8 / exclude_implementations = PyPY so we remove the pypy specificity when we drop python 3.8 support later on.

jacobtylerwalls · 2023-09-18T11:51:31Z

How would that work?

Pierre-Sassoulas · 2023-09-18T12:22:19Z

A file with everything py39.py: min_version = 3.9
A file with everything py38.py : max_version = 3.8, exclude_implementations = PyPY
A file with everything py38_pypy.py : max_version = 3.8, exclude_implementations = CPython

Edit: Or just the one problematic line in the py38 files

Noisy/duplicated in the short term but in the long run we're going to keep testing on pypy 3.9+ without having to think about it (we're going to delete the 38.py duplication when removing 3.8 support later on).

…ve finer control

badsketch · 2023-09-24T05:56:31Z

Created separate .py/.txt/.rc files for each version as suggested. Took me a surprisingly long time to get pypy38 set up so I could generate the output. 😓

I think that should wrap up the comments; were you going to take a look at the excluded dunder method list, Pierre?

github-actions · 2023-09-24T06:32:04Z

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

This comment was generated for commit 4b8bc68

Pierre-Sassoulas

LGTM, great first contribution 👍 We'll release the change in 3.0

Pierre-Sassoulas added Bug 🪲 backport maintenance/3.3.x labels Sep 13, 2023

Pierre-Sassoulas added this to the 2.17.6 milestone Sep 13, 2023

This comment has been minimized.

Sign in to view

badsketch force-pushed the fix/8769-lambda-dunder branch from 74d1d9e to d7dd8c6 Compare September 13, 2023 21:57

badsketch force-pushed the fix/8769-lambda-dunder branch from 9536285 to 259ee0d Compare September 17, 2023 01:27

This comment has been minimized.

Sign in to view

Pierre-Sassoulas reviewed Sep 17, 2023

View reviewed changes

jacobtylerwalls reviewed Sep 18, 2023

View reviewed changes

jacobtylerwalls removed the backport maintenance/3.3.x label Sep 18, 2023

jacobtylerwalls modified the milestones: 2.17.6, 3.0.0b1 Sep 18, 2023

badsketch added 4 commits September 17, 2023 23:35

Add exceptions to C2801 for certain dunders in a lambda definition

635fed4

Add changelog

eac6f23

Tweak naming and doc wording

7b2dd1e

Exclude pypy because ast implementation calculates columns differentl…

47123c0

…y resulting in failing tests

badsketch force-pushed the fix/8769-lambda-dunder branch from 259ee0d to 47123c0 Compare September 18, 2023 03:36

This comment has been minimized.

Sign in to view

Split tests based on implementation and version to pass checks and ha…

4b8bc68

…ve finer control

Pierre-Sassoulas approved these changes Sep 24, 2023

View reviewed changes

Pierre-Sassoulas merged commit 8231e06 into pylint-dev:main Sep 24, 2023

Pierre-Sassoulas changed the title ~~Fix C2801 special cases in lambda~~ Fix false positives for unnecessary-dunder-call in lambdas Sep 24, 2023

Pierre-Sassoulas modified the milestones: 3.0.0b1, 3.0.0a8 Sep 24, 2023

jacobtylerwalls mentioned this pull request Jul 7, 2024

Cleanup test skips #9773

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix false positives for `unnecessary-dunder-call` in lambdas #9034

Fix false positives for `unnecessary-dunder-call` in lambdas #9034

badsketch commented Sep 13, 2023

codecov bot commented Sep 13, 2023 •

edited

Loading

This comment has been minimized.

Pierre-Sassoulas commented Sep 13, 2023

badsketch commented Sep 13, 2023

This comment has been minimized.

Pierre-Sassoulas left a comment

Pierre-Sassoulas Sep 17, 2023

Pierre-Sassoulas Sep 17, 2023

Pierre-Sassoulas commented Sep 17, 2023

badsketch commented Sep 18, 2023

jacobtylerwalls Sep 18, 2023

jacobtylerwalls Sep 18, 2023

jacobtylerwalls commented Sep 18, 2023

badsketch commented Sep 18, 2023

Pierre-Sassoulas commented Sep 18, 2023

This comment has been minimized.

jacobtylerwalls commented Sep 18, 2023

Pierre-Sassoulas commented Sep 18, 2023 •

edited

Loading

badsketch commented Sep 24, 2023

github-actions bot commented Sep 24, 2023

Pierre-Sassoulas left a comment

	DUNDER_LAMBDA_EXCEPTIONS = [
	UNNECESSARY_DUNDER_CALL_LAMBDA_EXCEPTIONS = [

		@@ -0,0 +1,3 @@
		Add special cases of dunder methods that should not trigger ``unnecessary-dunder-call`` if it's in a lambda definition.

	Add special cases of dunder methods that should not trigger ``unnecessary-dunder-call`` if it's in a lambda definition.
	Dunder methods defined in lambda do not trigger ``unnecessary-dunder-call`` anymore, if they cannot be replaced by the non-dunder call.

Fix false positives for unnecessary-dunder-call in lambdas #9034

Fix false positives for unnecessary-dunder-call in lambdas #9034

Conversation

badsketch commented Sep 13, 2023

Type of Changes

Description

codecov bot commented Sep 13, 2023 • edited Loading

Codecov Report

This comment has been minimized.

Pierre-Sassoulas commented Sep 13, 2023

badsketch commented Sep 13, 2023

This comment has been minimized.

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas Sep 17, 2023

Choose a reason for hiding this comment

Pierre-Sassoulas Sep 17, 2023

Choose a reason for hiding this comment

Pierre-Sassoulas commented Sep 17, 2023

badsketch commented Sep 18, 2023

jacobtylerwalls Sep 18, 2023

Choose a reason for hiding this comment

jacobtylerwalls Sep 18, 2023

Choose a reason for hiding this comment

jacobtylerwalls commented Sep 18, 2023

badsketch commented Sep 18, 2023

Pierre-Sassoulas commented Sep 18, 2023

This comment has been minimized.

jacobtylerwalls commented Sep 18, 2023

Pierre-Sassoulas commented Sep 18, 2023 • edited Loading

badsketch commented Sep 24, 2023

github-actions bot commented Sep 24, 2023

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Fix false positives for `unnecessary-dunder-call` in lambdas #9034

Fix false positives for `unnecessary-dunder-call` in lambdas #9034

codecov bot commented Sep 13, 2023 •

edited

Loading

Pierre-Sassoulas commented Sep 18, 2023 •

edited

Loading