-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 bug where methods defined using lambdas were flagged by FURB118 #14639
Conversation
|
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
FURB118 | 1 | 0 | 1 | 0 | 0 |
Hmm, the |
Expr::Lambda(lambda_expr) => { | ||
if checker.enabled(Rule::ReimplementedOperator) { | ||
refurb::rules::reimplemented_operator(checker, &lambda_expr.into()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needed to be moved from deferred_lambdas.rs
to expressions.rs
because otherwise the semantic model doesn't understand that the lambda it's analysing is ever inside a class scope. As far as I can tell, there's no particular advantage associated with deferring the analysis of this rule to after the semantic model has been fully built (what we currently do). The rule never analyses any bindings or uses of bindings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing this comment. It saved me quiet some time guessing why it was moved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it took me a little while to figure out why the semantic model didn't think lambdas inside class scopes were inside class scopes 😅
7e13565 got rid of the new false negatives this PR initially introduced on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
Expr::Lambda(lambda_expr) => { | ||
if checker.enabled(Rule::ReimplementedOperator) { | ||
refurb::rules::reimplemented_operator(checker, &lambda_expr.into()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing this comment. It saved me quiet some time guessing why it was moved.
7e13565
to
9090af0
Compare
Summary
Fixes #13829.
Replacing an instance method definition with an
operator
-module function doesn't work, because instance methods need to be callable objects with__get__
methods ("descriptors"), and while user-defined functions are descriptors, this is not true for theoperator
-module functions. We already avoid flagging user-defined methods that are defined usingdef
statements, but we currently incorrectly flag user-defined functions that are defined by assigninglambda
s to variables in class bodies. I.e., this is a perfectly valid definition of an instance method, and FURB118's suggestion to replace the lambda withoperator.eq
function would break it:As a demonstration in the REPL:
This PR fixes things so that
lambda
assignments in class bodies are considered off-limits for the rule in the same way asdef
statements in class bodies. It also makes the fix-safety docs more expansive, so that they're clearer that there's a wider range of reasons why the rule might be unsafe than just the keyword-argument issue.Test Plan
I added a new fixture that confirms that all function definitions in class bodies are ignored by the rule.