[Linter] Combinable bool conditions #16479

tx-tomcat · 2024-03-01T12:04:35Z

Description

The rules detects and warns about comparison operations in Move code that can be simplified. It identifies comparisons over the same operands joined with a boolean operation, and suggests how to collapse them to a single operation.

Testing

New tests added

Release notes

vercel · 2024-03-01T12:04:44Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
mysten-ui	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 6, 2024 7:23pm
sui-core	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 6, 2024 7:23pm
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 6, 2024 7:23pm

4 Skipped Deployments

Name	Status	Preview	Updated (UTC)
explorer	⬜️ Ignored (Inspect)	Visit Preview	Dec 6, 2024 7:23pm
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Dec 6, 2024 7:23pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Dec 6, 2024 7:23pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Dec 6, 2024 7:23pm

tnowacki · 2024-03-14T22:00:52Z

...es/move/crates/move-compiler/src/sui_mode/linters/custom_rules/combinable_bool_conditions.rs

+
+impl TypingVisitorContext for Context<'_> {
+    fn visit_exp_custom(&mut self, exp: &mut T::Exp) -> bool {
+        if let UnannotatedExp_::BinopExp(e1, _op, _, e2) = &exp.exp.value {


I don't think this rule really works as is. Few flaws

We don't consider the _op here (wouldn't work really for &&)
Bigger flaw

We don't consider the left/right operands for op1 and op2. For example, you couldn't combine x < y || y == zintox <= y`.

The overall structure here is okay, but it will need a lot of work to come up with a good heuristic

tnowacki

Conceptually this lint has some flaws by not checking for the same e1 and e2 between comparison operations.

Otherwise we would erroneously report errors for x > y && y != z

tnowacki · 2024-03-26T17:12:51Z

external-crates/move/crates/move-compiler/src/linters/combinable_bool_conditions.rs

+
+impl TypingVisitorContext for Context<'_> {
+    fn visit_exp_custom(&mut self, exp: &mut T::Exp) -> bool {
+        if let UnannotatedExp_::BinopExp(e1, _op, _, e2) = &exp.exp.value {


We need to check for && here, yeah?

Or a different set of rules for ||

implemented

external-crates/move/crates/move-compiler/src/linters/combinable_bool_conditions.rs

tnowacki · 2024-05-16T23:32:07Z

external-crates/move/crates/move-compiler/src/linters/combinable_bool_conditions.rs

+        if let UnannotatedExp_::BinopExp(e1, op, _, e2) = &exp.exp.value {
+            if let (
+                UnannotatedExp_::BinopExp(lhs1, op1, _, rhs1),
+                UnannotatedExp_::BinopExp(lhs2, op2, _, rhs2),
+            ) = (&e1.exp.value, &e2.exp.value)


Let's switch these to let-else to reduce nesting

tnowacki · 2024-05-16T23:33:08Z

external-crates/move/crates/move-compiler/src/linters/combinable_bool_conditions.rs

+            ) = (&e1.exp.value, &e2.exp.value)
+            {
+                // Check both exp side are the same
+                if lhs1 == lhs2 && rhs1 == rhs2 {


@tzakian @awelc @cgswords thoughts on this condition? It feels a bit... unsatisfactory. But I'm not sure there is much else we can do without some form of symbolic execution.

I believe this guard will incorrectly catch this code:

let mut v = make_vector(); if (v.pop_back() == 1 && v.pop_back() == 1) { .. }

Great catch! Yeah this will not work for any function call, or any operation that has side effects, for example

{ let c = x; x = true; c } && { let c = x; x = true; c }

Its difficult to determine the effect of functions call so that I ignored the module call case.

tnowacki · 2024-05-16T23:34:05Z

external-crates/move/crates/move-compiler/src/linters/combinable_bool_conditions.rs

+                                    exp.exp.loc,
+                                    "This is always contradictory and can be simplified to false",
+                                );
+                            } else {


What about where op.value is == or !=?

Is that == AND != or == OR != ?

tnowacki · 2024-05-16T23:35:09Z

external-crates/move/crates/move-compiler/src/linters/combinable_bool_conditions.rs

+                            if op.value == BinOp_::And {
+                                add_replaceable_comparison_diag(
+                                    self.env,
+                                    exp.exp.loc,
+                                    "This is always contradictory and can be simplified to false",
+                                );
+                            } else {
+                                add_replaceable_comparison_diag(
+                                    self.env,
+                                    exp.exp.loc,
+                                    "Consider simplifying to `<=`.",
+                                );
+                            }


I'd combine these branches to something like

let msg = if op.value == BinOp_::And { ... } else { ... }; add_replaceable_comparison_diag(self.env, exp.exp.loc, msg)

tnowacki · 2024-05-16T23:36:28Z

external-crates/move/crates/move-compiler/src/linters/combinable_bool_conditions.rs

+                        (BinOp_::Neq, BinOp_::Lt) | (BinOp_::Lt, BinOp_::Neq) => {
+                            if op.value == BinOp_::And {
+                                add_replaceable_comparison_diag(
+                                    self.env,
+                                    exp.exp.loc,
+                                    "Consider simplifying to `<`.",
+                                );
+                            }
+                        }
+                        (BinOp_::Neq, BinOp_::Gt) | (BinOp_::Gt, BinOp_::Neq) => {
+                            if op.value == BinOp_::And {
+                                add_replaceable_comparison_diag(
+                                    self.env,
+                                    exp.exp.loc,
+                                    "Consider simplifying to `>`.",
+                                );
+                            }
+                        }


These cases feel a bit different. It is not really "simplifying" rather the != is already covered by the GT and LT

tnowacki · 2024-05-16T23:36:45Z

external-crates/move/crates/move-compiler/src/linters/mod.rs

-use crate::{command_line::compiler::Visitor, diagnostics::codes::WarningFilter};
-
+use crate::{
+    command_line::compiler::Visitor, diagnostics::codes::WarningFilter,
+    linters::combinable_bool_conditions::CombinableBool, typing::visitor::TypingVisitor,
+};
+pub mod combinable_bool_conditions;


This will have be redone on main

tnowacki · 2024-05-16T23:37:58Z

external-crates/move/crates/move-compiler/tests/custom_rules/combinable_bool.move

+        if (x == y || z < y) {};
+        if (x < y || x == y) {}; // should be x <= y
+        if (x == y || x > y) {}; // should be x >= y
+        if (x > y || x == y) {}; // should be x >= y
+        if (x != y || x < y) {}; // same as x < y
+        if (x < y || x != y) {}; // same as x < y
+        if (x != y || x > y) {}; // same as x > y
+        if (x > y || x != y) {}; // same as x > y
+        if (x > y || y != x) {}; // same as x > y
+        if (x > y && y == x) {}; // same as x >= y
+        if (m == n || m < n) {}; // should be m <= n


We should probably have some tests that don't involve if

tnowacki · 2024-05-16T23:38:17Z

external-crates/move/crates/move-compiler/src/linters/combinable_bool_conditions.rs

+    env: &'a mut CompilationEnv,
+}
+
+impl TypingVisitorConstructor for CombinableBool {


We should move this to the CFGIR to take advantage of constant folding and inlining

We will need to setup a non-absint visitor for you

Is that mean you setup visitor for us or we do it by ourself?

tnowacki · 2024-05-16T23:40:55Z

external-crates/move/crates/move-compiler/tests/custom_rules/combinable_bool.move

@@ -0,0 +1,25 @@
+module 0x42::M {


Please follow the pattern for tests we arrived at for the constant naming

Correct linter dir

Separate positive/negative tests

a case to the allow(lint(_)) case

tnowacki · 2024-05-16T23:41:56Z

external-crates/move/crates/move-compiler/src/linters/mod.rs

@@ -17,16 +20,33 @@ pub enum LintLevel {

 pub const ALLOW_ATTR_CATEGORY: &str = "lint";
 pub const LINT_WARNING_PREFIX: &str = "Lint ";
+pub const COMBINABLE_BOOL_FILTER_NAME: &str = "combinable_bool_conditions";


Suggested change

pub const COMBINABLE_BOOL_FILTER_NAME: &str = "combinable_bool_conditions";

pub const COMBINABLE_COMPARISON_FILTER_NAME: &str = "combinable_comparison";

vercel · 2024-05-20T22:35:19Z

@dzungdinh94 is attempting to deploy a commit to the Mysten Labs Team on Vercel.

A member of the Team first needs to authorize it.

vercel bot deployed to Preview – sui-kiosk March 1, 2024 12:05 View deployment

vercel bot deployed to Preview – sui-typescript-docs March 1, 2024 12:05 View deployment

vercel bot temporarily deployed to Preview – mysten-ui March 1, 2024 12:06 Inactive

vercel bot deployed to Preview – multisig-toolkit March 1, 2024 12:06 View deployment

vercel bot deployed to Preview – explorer March 1, 2024 12:06 View deployment

vercel bot deployed to Preview – sui-core March 1, 2024 12:08 View deployment

vercel bot deployed to Preview – sui-typescript-docs March 5, 2024 07:13 View deployment

vercel bot deployed to Preview – sui-core March 5, 2024 07:13 View deployment

tx-tomcat changed the title ~~[Linter]-Combinable bool conditions~~ [Linter] Combinable bool conditions Mar 5, 2024

vercel bot deployed to Preview – sui-typescript-docs March 5, 2024 16:02 View deployment

vercel bot deployed to Preview – explorer March 5, 2024 16:02 View deployment

vercel bot temporarily deployed to Preview – mysten-ui March 5, 2024 16:03 Inactive

vercel bot deployed to Preview – multisig-toolkit March 5, 2024 16:03 View deployment

vercel bot deployed to Preview – sui-kiosk March 5, 2024 16:03 View deployment

vercel bot deployed to Preview – sui-core March 5, 2024 16:05 View deployment

tnowacki requested changes Mar 14, 2024

View reviewed changes

vercel bot deployed to Preview – explorer March 22, 2024 19:32 View deployment

vercel bot deployed to Preview – multisig-toolkit March 22, 2024 19:32 View deployment

vercel bot deployed to Preview – sui-typescript-docs March 22, 2024 19:32 View deployment

vercel bot deployed to Preview – sui-kiosk March 22, 2024 19:33 View deployment

vercel bot deployed to Preview – sui-core March 22, 2024 19:33 View deployment

vercel bot temporarily deployed to Preview – mysten-ui March 22, 2024 19:33 Inactive

tnowacki requested changes Mar 26, 2024

View reviewed changes

vercel bot temporarily deployed to Preview – mysten-ui April 3, 2024 15:43 Inactive

vercel bot deployed to Preview – sui-core April 3, 2024 15:45 View deployment

tnowacki requested changes May 16, 2024

View reviewed changes

vercel bot deployed to Preview – sui-docs May 20, 2024 22:36 View deployment

vercel bot deployed to Preview – sui-docs May 21, 2024 08:48 View deployment

tnowacki added 3 commits December 4, 2024 14:31

rename

e2d0c86

rewrite

be60d7b

rename enums

d19e89f

tnowacki approved these changes Dec 5, 2024

View reviewed changes

tnowacki added 8 commits December 5, 2024 12:57

cleanup toml

4e22ad5

cleanup

5cdba23

spelling

7e8f0c5

first test

b297ba3

tests

4679e40

new test

b92bd09

true positive

86b18d9

false negative

e40b463

vercel bot deployed to Preview – sui-docs December 6, 2024 04:08 View deployment

simplify logic

7bec775

vercel bot deployed to Preview – sui-docs December 6, 2024 18:41 View deployment

revert toml change

c915f9d

tnowacki temporarily deployed to sui-typescript-aws-kms-test-env December 6, 2024 18:46 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 6, 2024 18:47 View deployment

fix comment

c1b5bbe

tnowacki temporarily deployed to sui-typescript-aws-kms-test-env December 6, 2024 18:47 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 6, 2024 18:49 View deployment

more tests

202a8c9

MystenLabs deleted a comment from qvmoveguy Dec 6, 2024

vercel bot deployed to Preview – sui-docs December 6, 2024 19:15 View deployment

tnowacki temporarily deployed to sui-typescript-aws-kms-test-env December 6, 2024 19:15 — with GitHub Actions Inactive

fix flipped cases

ce236aa

tnowacki temporarily deployed to sui-typescript-aws-kms-test-env December 6, 2024 19:22 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 6, 2024 19:23 View deployment

tnowacki merged commit 5c06cf6 into MystenLabs:main Dec 6, 2024
48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Linter] Combinable bool conditions #16479

[Linter] Combinable bool conditions #16479

tx-tomcat commented Mar 1, 2024 •

edited by tnowacki

Loading

vercel bot commented Mar 1, 2024 •

edited

Loading

tnowacki Mar 14, 2024

tx-tomcat Apr 3, 2024

tnowacki left a comment

tnowacki Mar 26, 2024

tnowacki Mar 26, 2024

tx-tomcat Apr 3, 2024

tnowacki May 16, 2024

tx-tomcat May 21, 2024

tnowacki May 16, 2024

cgswords May 17, 2024

tnowacki May 17, 2024

tx-tomcat May 21, 2024

tnowacki May 16, 2024

tx-tomcat May 20, 2024

tnowacki May 16, 2024

tx-tomcat May 21, 2024

tnowacki May 16, 2024

tx-tomcat May 21, 2024

tnowacki May 16, 2024

tx-tomcat May 20, 2024

tnowacki May 16, 2024

tnowacki May 16, 2024

tnowacki May 16, 2024

tx-tomcat May 20, 2024

tnowacki May 16, 2024

tx-tomcat May 21, 2024

tnowacki May 16, 2024

tx-tomcat May 20, 2024

vercel bot commented May 20, 2024

	pub const COMBINABLE_BOOL_FILTER_NAME: &str = "combinable_bool_conditions";
	pub const COMBINABLE_COMPARISON_FILTER_NAME: &str = "combinable_comparison";

[Linter] Combinable bool conditions #16479

[Linter] Combinable bool conditions #16479

Conversation

tx-tomcat commented Mar 1, 2024 • edited by tnowacki Loading

Description

Testing

Release notes

vercel bot commented Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnowacki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented May 20, 2024

tx-tomcat commented Mar 1, 2024 •

edited by tnowacki

Loading

vercel bot commented Mar 1, 2024 •

edited

Loading