[Linter] Reduce perceived false positives and have links to linter do…

…c in warnings (#15241)
aptos-labs · Nov 13, 2024 · 0153ee0 · 0153ee0
1 parent d035d74
commit 0153ee0
Show file tree

Hide file tree

Showing 14 changed files with 155 additions and 87 deletions.
diff --git a/third_party/move/move-compiler-v2/src/external_checks.rs b/third_party/move/move-compiler-v2/src/external_checks.rs
@@ -12,6 +12,9 @@ use move_model::{
 use move_stackless_bytecode::function_target::FunctionTarget;
 use std::{collections::BTreeSet, fmt, sync::Arc};
 
+/// Base URL for the linter documentation.
+const LINTER_URL_BASE: &str = "https://aptos.dev/en/build/smart-contracts/linter";
+
 /// Implement this trait to provide a collection of external checks.
 pub trait ExternalChecks {
     /// Get all the expression checkers.
@@ -60,13 +63,7 @@ pub trait ExpChecker {
 
     /// Report the `msg` highlighting the `loc`.
     fn report(&self, env: &GlobalEnv, loc: &Loc, msg: &str) {
-        env.lint_diag_with_notes(loc, msg, vec![
-            format!(
-                "To suppress this warning, annotate the function/module with the attribute `#[{}({})]`.",
-                LintAttribute::SKIP,
-                self.get_name()
-            ),
-        ]);
+        report(env, loc, msg, self.get_name().as_str());
     }
 }
 
@@ -80,13 +77,7 @@ pub trait StacklessBytecodeChecker {
 
     /// Report the `msg` highlighting the `loc`.
     fn report(&self, env: &GlobalEnv, loc: &Loc, msg: &str) {
-        env.lint_diag_with_notes(loc, msg, vec![
-            format!(
-                "To suppress this warning, annotate the function/module with the attribute `#[{}({})]`.",
-                LintAttribute::SKIP,
-                self.get_name()
-            ),
-        ]);
+        report(env, loc, msg, self.get_name().as_str());
     }
 }
 
@@ -103,3 +94,18 @@ pub fn known_checker_names(external_checkers: &Vec<Arc<dyn ExternalChecks>>) ->
     }
     names
 }
+
+/// Report the `msg` highlighting the `loc` for the `checker_name`.
+fn report(env: &GlobalEnv, loc: &Loc, msg: &str, checker_name: &str) {
+    env.lint_diag_with_notes(loc, msg, vec![
+        format!(
+        "To suppress this warning, annotate the function/module with the attribute `#[{}({})]`.",
+        LintAttribute::SKIP,
+        checker_name
+    ),
+        format!(
+            "For more information, see {}#{}.",
+            LINTER_URL_BASE, checker_name
+        ),
+    ]);
+}
diff --git a/...ty/move/tools/move-linter/src/model_ast_lints/unnecessary_numerical_extreme_comparison.rs b/...ty/move/tools/move-linter/src/model_ast_lints/unnecessary_numerical_extreme_comparison.rs
@@ -43,7 +43,11 @@ impl ExpChecker for UnnecessaryNumericalExtremeComparison {
             // get the type of the left-hand side.
             let ty = env.get_node_type(lhs.node_id());
             if let Some(result) = Self::check_comparisons_with_extremes(lhs, cmp, rhs, &ty) {
-                self.report(env, &env.get_node_loc(*id), &result.to_string());
+                // TODO: we could report `UseEqInstead` and `UseNeqInstead` as well, but in a strict or pedantic
+                // mode. For now, we skip reporting them as suggestions to reduce perceived false positives.
+                if let ComparisonResult::AlwaysTrue | ComparisonResult::AlwaysFalse = result {
+                    self.report(env, &env.get_node_loc(*id), &result.to_string());
+                }
             }
         }
     }

diff --git a/third_party/move/tools/move-linter/tests/model_ast_lints/blocks_in_conditions_warn.exp b/third_party/move/tools/move-linter/tests/model_ast_lints/blocks_in_conditions_warn.exp
@@ -7,6 +7,7 @@ warning: [lint] Having blocks in conditions make code harder to read. Consider r
    │             ^^^^^^^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(blocks_in_conditions)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#blocks_in_conditions.
 
 warning: [lint] Having blocks in conditions make code harder to read. Consider rewriting this code.
    ┌─ tests/model_ast_lints/blocks_in_conditions_warn.move:26:13
@@ -15,6 +16,7 @@ warning: [lint] Having blocks in conditions make code harder to read. Consider r
    │             ^^^^^^^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(blocks_in_conditions)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#blocks_in_conditions.
 
 warning: [lint] Having blocks in conditions make code harder to read. Consider rewriting this code.
    ┌─ tests/model_ast_lints/blocks_in_conditions_warn.move:32:16
@@ -23,6 +25,7 @@ warning: [lint] Having blocks in conditions make code harder to read. Consider r
    │                ^^^^^^^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(blocks_in_conditions)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#blocks_in_conditions.
 
 warning: [lint] Having blocks in conditions make code harder to read. Consider rewriting this code.
    ┌─ tests/model_ast_lints/blocks_in_conditions_warn.move:39:13
@@ -31,6 +34,7 @@ warning: [lint] Having blocks in conditions make code harder to read. Consider r
    │             ^^^^^^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(blocks_in_conditions)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#blocks_in_conditions.
 
 warning: [lint] Having blocks in conditions make code harder to read. Consider rewriting this code.
    ┌─ tests/model_ast_lints/blocks_in_conditions_warn.move:40:17
@@ -39,6 +43,7 @@ warning: [lint] Having blocks in conditions make code harder to read. Consider r
    │                 ^^^^^^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(blocks_in_conditions)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#blocks_in_conditions.
 
 warning: [lint] Having blocks in conditions make code harder to read. Consider rewriting this code.
    ┌─ tests/model_ast_lints/blocks_in_conditions_warn.move:44:17
@@ -47,6 +52,7 @@ warning: [lint] Having blocks in conditions make code harder to read. Consider r
    │                 ^^^^^^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(blocks_in_conditions)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#blocks_in_conditions.
 
 warning: [lint] Having blocks in conditions make code harder to read. Consider rewriting this code.
    ┌─ tests/model_ast_lints/blocks_in_conditions_warn.move:52:13
@@ -55,6 +61,7 @@ warning: [lint] Having blocks in conditions make code harder to read. Consider r
    │             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(blocks_in_conditions)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#blocks_in_conditions.
 
 warning: [lint] Having blocks in conditions make code harder to read. Consider rewriting this code.
    ┌─ tests/model_ast_lints/blocks_in_conditions_warn.move:58:16
@@ -63,6 +70,7 @@ warning: [lint] Having blocks in conditions make code harder to read. Consider r
    │                ^^^^^^^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(blocks_in_conditions)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#blocks_in_conditions.
 
 warning: [lint] Having blocks in conditions make code harder to read. Consider rewriting this code.
    ┌─ tests/model_ast_lints/blocks_in_conditions_warn.move:64:13
@@ -71,3 +79,4 @@ warning: [lint] Having blocks in conditions make code harder to read. Consider r
    │             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(blocks_in_conditions)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#blocks_in_conditions.
diff --git a/third_party/move/tools/move-linter/tests/model_ast_lints/multi_attributes_01.exp b/third_party/move/tools/move-linter/tests/model_ast_lints/multi_attributes_01.exp
@@ -7,6 +7,7 @@ warning: [lint] Having blocks in conditions make code harder to read. Consider r
    │             ^^^^^^^^^^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(blocks_in_conditions)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#blocks_in_conditions.
 
 warning: [lint] Use the more explicit `loop` instead.
    ┌─ tests/model_ast_lints/multi_attributes_01.move:16:9
@@ -17,3 +18,4 @@ warning: [lint] Use the more explicit `loop` instead.
    │ ╰─────────^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(while_true)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#while_true.
diff --git a/third_party/move/tools/move-linter/tests/model_ast_lints/needless_bool_warn.exp b/third_party/move/tools/move-linter/tests/model_ast_lints/needless_bool_warn.exp
@@ -7,6 +7,7 @@ warning: [lint] This if-else can be replaced with just the condition
   │         ^^^^^^^^^^^^^^^^^^^^^^^^^^
   │
   = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_bool)]`.
+  = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_bool.
 
 warning: [lint] This if-else can be replaced with just the negation of the condition
    ┌─ tests/model_ast_lints/needless_bool_warn.move:11:9
@@ -19,6 +20,7 @@ warning: [lint] This if-else can be replaced with just the negation of the condi
    │ ╰─────────^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_bool)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_bool.
 
 warning: [lint] This if-else can be replaced with just returning the condition
    ┌─ tests/model_ast_lints/needless_bool_warn.move:19:9
@@ -31,6 +33,7 @@ warning: [lint] This if-else can be replaced with just returning the condition
    │ ╰─────────^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_bool)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_bool.
 
 warning: [lint] This if-else can be replaced with just returning the negation of the condition
    ┌─ tests/model_ast_lints/needless_bool_warn.move:28:13
@@ -43,6 +46,7 @@ warning: [lint] This if-else can be replaced with just returning the negation of
    │ ╰─────────────^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_bool)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_bool.
 
 warning: [lint] This if-else has the same bool expression in both branches, consider rewriting the code to remove this redundancy
    ┌─ tests/model_ast_lints/needless_bool_warn.move:38:9
@@ -51,3 +55,4 @@ warning: [lint] This if-else has the same bool expression in both branches, cons
    │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_bool)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_bool.
diff --git a/third_party/move/tools/move-linter/tests/model_ast_lints/needless_deref_ref_warn.exp b/third_party/move/tools/move-linter/tests/model_ast_lints/needless_deref_ref_warn.exp
@@ -7,6 +7,7 @@ warning: [lint] Needless pair of `*` and `&mut` operators: consider removing the
    │         ^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_deref_ref)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_deref_ref.
 
 warning: [lint] Needless pair of `*` and `&` operators: consider removing them
    ┌─ tests/model_ast_lints/needless_deref_ref_warn.move:27:9
@@ -15,6 +16,7 @@ warning: [lint] Needless pair of `*` and `&` operators: consider removing them
    │         ^^^^^^^^^^^^^^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_deref_ref)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_deref_ref.
 
 warning: [lint] Needless pair of `*` and `&mut` operators: consider removing them
    ┌─ tests/model_ast_lints/needless_deref_ref_warn.move:35:9
@@ -23,6 +25,7 @@ warning: [lint] Needless pair of `*` and `&mut` operators: consider removing the
    │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_deref_ref)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_deref_ref.
 
 warning: [lint] Needless pair of `*` and `&mut` operators: consider removing them
    ┌─ tests/model_ast_lints/needless_deref_ref_warn.move:60:9
@@ -31,6 +34,7 @@ warning: [lint] Needless pair of `*` and `&mut` operators: consider removing the
    │         ^^^^^^^^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_deref_ref)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_deref_ref.
 
 warning: [lint] Needless pair of `*` and `&mut` operators: consider removing them
    ┌─ tests/model_ast_lints/needless_deref_ref_warn.move:68:9
@@ -39,6 +43,7 @@ warning: [lint] Needless pair of `*` and `&mut` operators: consider removing the
    │         ^^^^^^^^^^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_deref_ref)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_deref_ref.
 
 warning: [lint] Needless pair of `*` and `&mut` operators: consider removing them
    ┌─ tests/model_ast_lints/needless_deref_ref_warn.move:82:9
@@ -47,6 +52,7 @@ warning: [lint] Needless pair of `*` and `&mut` operators: consider removing the
    │         ^^^^^^^^^^^^^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_deref_ref)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_deref_ref.
 
 warning: [lint] Needless pair of `*` and `&` operators: consider removing them
    ┌─ tests/model_ast_lints/needless_deref_ref_warn.move:99:9
@@ -55,6 +61,7 @@ warning: [lint] Needless pair of `*` and `&` operators: consider removing them
    │         ^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_deref_ref)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_deref_ref.
 
 warning: [lint] Needless pair of `*` and `&mut` operators: consider removing them
     ┌─ tests/model_ast_lints/needless_deref_ref_warn.move:109:9
@@ -63,6 +70,7 @@ warning: [lint] Needless pair of `*` and `&mut` operators: consider removing the
     │         ^^^^^^^
     │
     = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_deref_ref)]`.
+    = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_deref_ref.
 
 warning: [lint] Needless pair of `*` and `&mut` operators: consider removing them
     ┌─ tests/model_ast_lints/needless_deref_ref_warn.move:114:9
@@ -71,6 +79,7 @@ warning: [lint] Needless pair of `*` and `&mut` operators: consider removing the
     │         ^^^^^^^^^
     │
     = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_deref_ref)]`.
+    = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_deref_ref.
 
 warning: [lint] Needless pair of `*` and `&` operators: consider removing them
     ┌─ tests/model_ast_lints/needless_deref_ref_warn.move:119:9
@@ -79,6 +88,7 @@ warning: [lint] Needless pair of `*` and `&` operators: consider removing them
     │         ^^^^^^^^
     │
     = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_deref_ref)]`.
+    = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_deref_ref.
 
 warning: [lint] Needless pair of `*` and `&` operators: consider removing them
     ┌─ tests/model_ast_lints/needless_deref_ref_warn.move:129:9
@@ -87,6 +97,7 @@ warning: [lint] Needless pair of `*` and `&` operators: consider removing them
     │         ^^^^^^
     │
     = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_deref_ref)]`.
+    = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_deref_ref.
 
 warning: [lint] Needless pair of `*` and `&mut` operators: consider removing them
     ┌─ tests/model_ast_lints/needless_deref_ref_warn.move:139:9
@@ -95,6 +106,7 @@ warning: [lint] Needless pair of `*` and `&mut` operators: consider removing the
     │         ^^^^^^^^^
     │
     = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_deref_ref)]`.
+    = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_deref_ref.
 
 warning: [lint] Needless pair of `*` and `&mut` operators: consider removing them
     ┌─ tests/model_ast_lints/needless_deref_ref_warn.move:144:9
@@ -103,6 +115,7 @@ warning: [lint] Needless pair of `*` and `&mut` operators: consider removing the
     │         ^^^^^^^^^^^^^
     │
     = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_deref_ref)]`.
+    = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_deref_ref.
 
 warning: [lint] Needless pair of `*` and `&mut` operators: consider removing them
     ┌─ tests/model_ast_lints/needless_deref_ref_warn.move:148:9
@@ -111,6 +124,7 @@ warning: [lint] Needless pair of `*` and `&mut` operators: consider removing the
     │         ^^^^^^^^^^^^
     │
     = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_deref_ref)]`.
+    = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_deref_ref.
 
 warning: [lint] Needless pair of `*` and `&mut` operators: consider removing them
     ┌─ tests/model_ast_lints/needless_deref_ref_warn.move:153:9
@@ -119,6 +133,7 @@ warning: [lint] Needless pair of `*` and `&mut` operators: consider removing the
     │         ^^^^^^^^^^^
     │
     = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_deref_ref)]`.
+    = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_deref_ref.
 
 warning: [lint] Needless pair of `*` and `&` operators: consider removing them
     ┌─ tests/model_ast_lints/needless_deref_ref_warn.move:170:26
@@ -127,6 +142,7 @@ warning: [lint] Needless pair of `*` and `&` operators: consider removing them
     │                          ^^^^^^^^^
     │
     = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_deref_ref)]`.
+    = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_deref_ref.
 
 warning: [lint] Needless mutable reference or borrow: consider using immutable reference or borrow instead
    ┌─ tests/model_ast_lints/needless_deref_ref_warn.move:35:15
@@ -135,3 +151,4 @@ warning: [lint] Needless mutable reference or borrow: consider using immutable r
    │               ^^^^^^^^^^^^^^^^^^^^^^^^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_mutable_reference)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_mutable_reference.
diff --git a/third_party/move/tools/move-linter/tests/model_ast_lints/needless_ref_deref_warn.exp b/third_party/move/tools/move-linter/tests/model_ast_lints/needless_ref_deref_warn.exp
@@ -7,6 +7,7 @@ warning: [lint] Needless pair of `&` and `*` operators: consider removing them
   │                                              ^^^
   │
   = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_ref_deref)]`.
+  = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_ref_deref.
 
 warning: [lint] Needless pair of `&` and `*` operators: consider removing them
    ┌─ tests/model_ast_lints/needless_ref_deref_warn.move:35:9
@@ -15,3 +16,4 @@ warning: [lint] Needless pair of `&` and `*` operators: consider removing them
    │         ^^^
    │
    = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_ref_deref)]`.
+   = For more information, see https://aptos.dev/en/build/smart-contracts/linter#needless_ref_deref.