Add a function to check whether binary oprands are nontrivial #11907

cocodery · 2023-12-02T17:09:28Z

It's hard to check whether operator is overrided through context of lint.
So, assume non-trivial structure like tuple, array or sturt, using a overrided binary operator in this lint, which might cause a side effict.
This is not detected before.
Althrough this might weaken the ability of this lint, it may more useful than before. Maybe this lint will cause an error, but now, it not. And assuming side effect of non-trivial structure with operator is not a bad thing, right?

changelog: Fix: [no_effect] check if binary operands are nontrivial

It's hard to check whether oprator is overrided through context of lint So assume nontrivial has overrided binary operator

rustbot · 2023-12-02T17:09:33Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

cocodery · 2023-12-02T17:14:47Z

Ahhh, I forget to add the case of the issue to test...
Is it needed to add?

xFrednet · 2023-12-02T21:26:54Z

Yes, every bug and code should be tested. You can just push a new commit with the tests 👍

y21 · 2023-12-02T21:52:36Z

clippy_lints/src/no_effect.rs

+                    },
+                    _ => false,
+                };
+                false


Does this actually work? It looks like this always returns false in any case. The match ends in a semicolon so its value looks to be effectively ignored. The false here should probably go?

Thanks! I have fixed this.

y21 · 2023-12-02T21:57:35Z

clippy_lints/src/no_effect.rs

+                match res {
+                    Res::Def(defkind, _) => match defkind {
+                        // user-defined
+                        DefKind::Struct | DefKind::Ctor(_, _) => true,


What about references to structs? Or function calls that return a struct? Or slices of structs? It might be easier to just get the type of the expression and check if it's a scalar value (char, bool, float, int).
Or does this introduce too many false negatives?

Yeah, you're right. Thanks!
When I fixing this issue, I have the same question. But I'm not familiar with these utils functions clearly.
I'll fix this in next commit.

Hi, @y21 :)
I use method is_method_call in rust/compiler/rustc_middle/src/ty/typeck_results.rs for ExprKind::Binary to check whether operator is overrided or not. It's quit simple.
As you say, if excluding scalar value of operands may introduce too many false negatives, in fact.
So I think maybe only check Struct Enum Union is better? Not many people override operator with other structure, and only few of them have side effects?
I don't know what you think about it. Wait your reply!

Ah I wasn't aware of that is_method_call method, that looks useful here :)
Sounds like a good idea to me

Yeah, it helps me reduce a lot of work to check operator override !
I have pushed my code just now. You can review it.

Check whether operator is overrided with a `struct` operand. The struct here refers to `struct`, `enum`, `union`. Add and fix test for `no_effect` lint.

y21 · 2023-12-04T18:24:27Z

clippy_lints/src/no_effect.rs

+            let ud_lhs = match ty_lhs {
+                ty::Adt(adt_def, _) => adt_def.is_struct() || adt_def.is_enum() || adt_def.is_union(),
+                _ => false,
+            };


Small nit but: an ADT is always either a struct, enum or union, so the condition in the arm always returns true ^^
This could just be let ud_lhs = matches!(ty_lhs, ty::Adt(..));

Ahh, I forgot this, I'll fix it.

y21 · 2023-12-04T18:30:29Z

clippy_lints/src/no_effect.rs

+
+            // reference: rust/compiler/rustc_middle/src/ty/typeck_results.rs: `is_method_call`.
+            // use this function to check whether operator is overrided in `ExprKind::Binary`.
+            (ud_lhs || ud_rhs) && tyck_result.is_method_call(expr)


Isn't the is_method_call already implied by ud_lhs || ud_rhs? If either the LHS or RHS is an adt, then that must mean that it's a custom operator impl, right?

Actually thinking about it, can't we use just tyck_result.is_method_call(expr), without checking if lhs and rhs is an ADT? I think that does the same but is simpler.
is_method_call returns false for operators on scalar types like integers, so it would still lint correctly for things like int + int;.

Absoulutely! Fixed in next commit!

Simpfy code of `is_operator_overrided`, directly use `is_method_call` to check if operator is overrided, at least one oprand of binary-expr must be ADT-type So no need to check type of lhs and rhs

y21

This logic of checking for a call to a custom operator impl looks good to me! I have one more suggestion (and the typo), hopefully @xFrednet doesn't mind 😅

clippy_lints/src/no_effect.rs

y21 · 2023-12-05T19:30:56Z

clippy_lints/src/no_effect.rs

-fn has_no_effect(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
-    if expr.span.from_expansion() {
-        return false;
+fn is_operator_overrided(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {


Suggested change

fn is_operator_overrided(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {

fn is_operator_overriden(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {

I have added check for unary operator and fix my typo, test for unary operator is added to ui/test/no_effect.* also.
It's nice of you to give a lot reliable suggestions.
Ready to be merged !

Fix typo and add test for unary-opeator

xFrednet

Three small nits, but the rest looks good to me.

Also big thanks to @y21 for the review ❤️

xFrednet · 2023-12-08T09:42:59Z

clippy_lints/src/no_effect.rs

-fn has_no_effect(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
-    if expr.span.from_expansion() {
-        return false;
+fn is_operator_overriden(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {


NIT:

Suggested change

fn is_operator_overriden(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {

fn is_operator_overridden(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {

xFrednet · 2023-12-08T09:44:28Z

clippy_lints/src/no_effect.rs

+        // assume nontrivial oprand of `Binary` Expr can skip `check_unnecessary_operation`
+        if is_operator_overriden(cx, expr) {
+            return true;
+        }


NIT(1): I find it a bit weird that this line also influences unnecessary_operation but I have no idea how to rewrite it better. I think this adjustment will make it a bit clearer and then it should be fine:

Suggested change

// assume nontrivial oprand of `Binary` Expr can skip `check_unnecessary_operation`

if is_operator_overriden(cx, expr) {

return true;

}

if is_operator_overridden(cx, expr) {

// Return `true`, to prevent `check_unnecessary_operation` from

// linting on this statement as well.

return true;

}

NIT(2): Could you also add tests to the file for unnecessary_operation to ensure that it doesn't lint on this expression?

xFrednet · 2023-12-08T13:38:28Z

Looks good to me, thank you for the fix!

@y21 Thank you for the review as well!

@bors r=y21,xFrednet

bors · 2023-12-08T13:38:30Z

📌 Commit 56d20c2 has been approved by y21,xFrednet

It is now in the queue for this repository.

bors · 2023-12-08T13:39:49Z

⌛ Testing commit 56d20c2 with merge 1c8cbe7...

cocodery · 2023-12-08T13:40:04Z

Thanks for you two too!

bors · 2023-12-08T13:47:02Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21,xFrednet
Pushing 1c8cbe7 to master...

Add a function to check whether binary oprands are nontrivial

6d40b10

It's hard to check whether oprator is overrided through context of lint So assume nontrivial has overrided binary operator

rustbot assigned xFrednet Dec 2, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 2, 2023

y21 reviewed Dec 2, 2023

View reviewed changes

Rewrite logic of has_nontrivial_oprand.

8977423

Check whether operator is overrided with a `struct` operand. The struct here refers to `struct`, `enum`, `union`. Add and fix test for `no_effect` lint.

cocodery requested a review from y21 December 4, 2023 10:17

y21 reviewed Dec 4, 2023

View reviewed changes

Rename has_nontrivial_oprand to is_operator_overrided

2e3c031

Simpfy code of `is_operator_overrided`, directly use `is_method_call` to check if operator is overrided, at least one oprand of binary-expr must be ADT-type So no need to check type of lhs and rhs

cocodery requested a review from y21 December 5, 2023 10:52

y21 reviewed Dec 5, 2023

View reviewed changes

Add check for unary-operator

ee2354b

Fix typo and add test for unary-opeator

xFrednet reviewed Dec 8, 2023

View reviewed changes

Fix nits and add test for unnecessary_operation

56d20c2

cocodery force-pushed the issue11885 branch from 1d34bfa to 56d20c2 Compare December 8, 2023 13:34

xFrednet approved these changes Dec 8, 2023

View reviewed changes

bors merged commit 1c8cbe7 into rust-lang:master Dec 8, 2023

cocodery deleted the issue11885 branch December 8, 2023 14:50

	fn is_operator_overrided(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
	fn is_operator_overriden(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {

	fn is_operator_overriden(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
	fn is_operator_overridden(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {

Add a function to check whether binary oprands are nontrivial #11907

Add a function to check whether binary oprands are nontrivial #11907

Uh oh!

Conversation

cocodery commented Dec 2, 2023 • edited by xFrednet Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Dec 2, 2023

Uh oh!

cocodery commented Dec 2, 2023

Uh oh!

xFrednet commented Dec 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cocodery Dec 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cocodery Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

y21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xFrednet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xFrednet Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xFrednet commented Dec 8, 2023

Uh oh!

bors commented Dec 8, 2023

Uh oh!

bors commented Dec 8, 2023

Uh oh!

cocodery commented Dec 8, 2023

Uh oh!

bors commented Dec 8, 2023

Uh oh!

Uh oh!

cocodery commented Dec 2, 2023 •

edited by xFrednet

Loading

cocodery Dec 3, 2023 •

edited

Loading

cocodery Dec 4, 2023 •

edited

Loading

xFrednet Dec 8, 2023 •

edited

Loading