Attributes to suppress lint warnings (aptos-labs#14362)

third_party/move/move-compiler-v2/Cargo.toml

@@ -37,6 +37,8 @@ num = { workspace = true }
 once_cell = { workspace = true }
 #paste = "1.0.5"
 petgraph = { workspace = true }
+strum = { workspace = true }
+strum_macros = { workspace = true }
 
 [dev-dependencies]
 anyhow = { workspace = true }
@@ -54,3 +56,6 @@ doctest = false
 name = "testsuite"
 harness = false
 doctest = false
+
+[package.metadata.cargo-machete]
+ignored = ["strum"]

third_party/move/move-compiler-v2/src/env_pipeline/model_ast_lints.rs

@@ -8,20 +8,24 @@ mod unnecessary_boolean_identity_comparison;
 mod unnecessary_numerical_extreme_comparison;
 mod while_true;
 
+use crate::lint_common::{lint_skips_from_attributes, LintChecker};
+use move_compiler::shared::known_attributes::LintAttribute;
 use move_model::{
     ast::ExpData,
-    model::{FunctionEnv, GlobalEnv},
+    model::{FunctionEnv, GlobalEnv, Loc},
 };
+use std::collections::BTreeSet;
 
 /// Perform various lint checks on the model AST.
 pub fn checker(env: &mut GlobalEnv) {
     for module in env.get_modules() {
         if module.is_primary_target() {
+            let module_lint_skips = lint_skips_from_attributes(env, module.get_attributes());
             for function in module.get_functions() {
                 if function.is_native() {
                     continue;
                 }
-                check_function(&function);
+                check_function(&function, &module_lint_skips);
             }
         }
     }
@@ -31,24 +35,38 @@ pub fn checker(env: &mut GlobalEnv) {
 /// expression as we traverse the model AST.
 /// Implement at least one of the `visit` methods to be a useful lint.
 trait ExpressionLinter {
-    /// The name of the lint.
-    fn get_name(&self) -> &'static str;
+    /// The corresponding lint checker enumerated value.
+    fn get_lint_checker(&self) -> LintChecker;
 
     /// Examine `expr` before any of its children have been visited.
-    /// Potentially emit lint warnings using `env.lint_diag()`.
+    /// Potentially emit lint warnings using `self.warning()`.
     fn visit_expr_pre(&mut self, _env: &GlobalEnv, _expr: &ExpData) {}
 
     /// Examine `expr` after all its children have been visited.
-    /// Potentially emit lint warnings using `env.lint_diag()`.
+    /// Potentially emit lint warnings using `self.warning()`.
     fn visit_expr_post(&mut self, _env: &GlobalEnv, _expr: &ExpData) {}
+
+    /// Emit a lint warning with the `msg` highlighting the `loc`.
+    fn warning(&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_lint_checker()
+            ),
+        ]);
+    }
 }
 
 /// Perform the lint checks on the code in `function`.
-fn check_function(function: &FunctionEnv) {
-    let mut expression_linters = get_expression_linter_pipeline();
+fn check_function(function: &FunctionEnv, module_lint_skips: &[LintChecker]) {
+    let env = function.module_env.env;
+    let function_lint_skips = lint_skips_from_attributes(env, function.get_attributes());
+    let mut lint_skips = BTreeSet::from_iter(function_lint_skips);
+    lint_skips.extend(module_lint_skips);
+    let mut expression_linters = get_applicable_lints(lint_skips);
     if let Some(def) = function.get_def() {
         let mut visitor = |post: bool, e: &ExpData| {
-            let env = function.module_env.env;
             if !post {
                 for exp_lint in expression_linters.iter_mut() {
                     exp_lint.visit_expr_pre(env, e);
@@ -64,8 +82,16 @@ fn check_function(function: &FunctionEnv) {
     }
 }
 
-/// Returns a pipeline of "expression linters" to run.
-fn get_expression_linter_pipeline() -> Vec<Box<dyn ExpressionLinter>> {
+/// Returns a pipeline of "expression linters" to run, skipping the ones in `lint_skips`.
+fn get_applicable_lints(lint_skips: BTreeSet<LintChecker>) -> Vec<Box<dyn ExpressionLinter>> {
+    get_default_expression_linter_pipeline()
+        .into_iter()
+        .filter(|lint| !lint_skips.contains(&lint.get_lint_checker()))
+        .collect()
+}
+
+/// Returns a default pipeline of "expression linters" to run.
+fn get_default_expression_linter_pipeline() -> Vec<Box<dyn ExpressionLinter>> {
     vec![
         Box::<blocks_in_conditions::BlocksInConditions>::default(),
         Box::<unnecessary_boolean_identity_comparison::UnnecessaryBooleanIdentityComparison>::default(),

third_party/move/move-compiler-v2/src/env_pipeline/model_ast_lints/blocks_in_conditions.rs

@@ -9,7 +9,7 @@
 //!
 //! We also only report on the outermost condition with blocks.
 
-use crate::env_pipeline::model_ast_lints::ExpressionLinter;
+use crate::{env_pipeline::model_ast_lints::ExpressionLinter, lint_common::LintChecker};
 use move_model::{
     ast::ExpData,
     model::{GlobalEnv, NodeId},
@@ -38,8 +38,8 @@ enum CondExprState {
 }
 
 impl ExpressionLinter for BlocksInConditions {
-    fn get_name(&self) -> &'static str {
-        "blocks_in_conditions"
+    fn get_lint_checker(&self) -> LintChecker {
+        LintChecker::BlocksInConditions
     }
 
     fn visit_expr_pre(&mut self, _env: &GlobalEnv, expr: &ExpData) {
@@ -88,10 +88,10 @@ impl ExpressionLinter for BlocksInConditions {
                 // We are done with traversing the condition of interest.
                 self.state = None;
                 if has_any_block && !has_spec_block {
-                    env.lint_diag(
+                    self.warning(
+                        env,
                         &env.get_node_loc(id),
-                        self.get_name(),
-                        "Having blocks in conditions make code harder to read. Consider rewriting this code.",
+                        "Having blocks in conditions make code harder to read. Consider rewriting this code."
                     );
                 }
             },

third_party/move/move-compiler-v2/src/env_pipeline/model_ast_lints/unnecessary_boolean_identity_comparison.rs

@@ -9,7 +9,7 @@
 //!   `x == true` ==> `x`
 //!   `false != foo(x)` ==> `!foo(x)`
 
-use crate::env_pipeline::model_ast_lints::ExpressionLinter;
+use crate::{env_pipeline::model_ast_lints::ExpressionLinter, lint_common::LintChecker};
 use move_model::{
     ast::{ExpData, Operation, Value},
     model::GlobalEnv,
@@ -19,8 +19,8 @@ use move_model::{
 pub struct UnnecessaryBooleanIdentityComparison;
 
 impl ExpressionLinter for UnnecessaryBooleanIdentityComparison {
-    fn get_name(&self) -> &'static str {
-        "unnecessary_boolean_identity_comparison"
+    fn get_lint_checker(&self) -> LintChecker {
+        LintChecker::UnnecessaryBooleanIdentityComparison
     }
 
     fn visit_expr_pre(&mut self, env: &GlobalEnv, expr: &ExpData) {
@@ -45,7 +45,7 @@ impl ExpressionLinter for UnnecessaryBooleanIdentityComparison {
                         },
                         if *b { "true" } else { "false" }
                     );
-                    env.lint_diag(&env.get_node_loc(e.node_id()), self.get_name(), &msg);
+                    self.warning(env, &env.get_node_loc(e.node_id()), &msg);
                 },
                 _ => {},
             }

third_party/move/move-compiler-v2/src/env_pipeline/model_ast_lints/unnecessary_numerical_extreme_comparison.rs

@@ -12,7 +12,7 @@
 //!   `x > 0` ==> can be clarified to `x != 0`
 //! and similarly for comparing `x` with u64::MAX.
 
-use crate::env_pipeline::model_ast_lints::ExpressionLinter;
+use crate::{env_pipeline::model_ast_lints::ExpressionLinter, lint_common::LintChecker};
 use move_model::{
     ast::{ExpData, Operation, Value},
     model::GlobalEnv,
@@ -25,8 +25,8 @@ use std::fmt;
 pub struct UnnecessaryNumericalExtremeComparison;
 
 impl ExpressionLinter for UnnecessaryNumericalExtremeComparison {
-    fn get_name(&self) -> &'static str {
-        "unnecessary_numerical_extreme_comparison"
+    fn get_lint_checker(&self) -> LintChecker {
+        LintChecker::UnnecessaryNumericalExtremeComparison
     }
 
     fn visit_expr_pre(&mut self, env: &GlobalEnv, expr: &ExpData) {
@@ -43,7 +43,7 @@ impl ExpressionLinter 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) {
-                env.lint_diag(&env.get_node_loc(*id), self.get_name(), &result.to_string());
+                self.warning(env, &env.get_node_loc(*id), &result.to_string());
             }
         }
     }

third_party/move/move-compiler-v2/src/env_pipeline/model_ast_lints/while_true.rs

@@ -4,7 +4,7 @@
 //! This module implements an expression linter that checks code of the form:
 //! `while (true) { ... }` and suggests to use `loop { ... }` instead.
 
-use crate::env_pipeline::model_ast_lints::ExpressionLinter;
+use crate::{env_pipeline::model_ast_lints::ExpressionLinter, lint_common::LintChecker};
 use move_compiler::parser::syntax::FOR_LOOP_UPDATE_ITER_FLAG;
 use move_model::{
     ast::{Exp, ExpData, Value},
@@ -15,8 +15,8 @@ use move_model::{
 pub struct WhileTrue;
 
 impl ExpressionLinter for WhileTrue {
-    fn get_name(&self) -> &'static str {
-        "while_true"
+    fn get_lint_checker(&self) -> LintChecker {
+        LintChecker::WhileTrue
     }
 
     fn visit_expr_pre(&mut self, env: &GlobalEnv, expr: &ExpData) {
@@ -37,9 +37,9 @@ impl ExpressionLinter for WhileTrue {
             return;
         }
         // If we are here, it is `while (true) {...}`.
-        env.lint_diag(
+        self.warning(
+            env,
             &env.get_node_loc(*id),
-            self.get_name(),
             "Use the more explicit `loop` instead.",
         );
     }

third_party/move/move-compiler-v2/src/lib.rs

@@ -6,6 +6,7 @@ mod bytecode_generator;
 pub mod env_pipeline;
 mod experiments;
 mod file_format_generator;
+pub mod lint_common;
 pub mod logging;
 pub mod options;
 pub mod pipeline;

third_party/move/move-compiler-v2/src/lint_common.rs

@@ -0,0 +1,89 @@
+// Copyright (c) Aptos Foundation
+// SPDX-License-Identifier: Apache-2.0
+
+//! This module contains common code useful for lint checkers at various stages
+//! of the compilation pipeline.
+
+use move_compiler::shared::known_attributes::LintAttribute;
+use move_model::{ast::Attribute, model::GlobalEnv};
+use std::str::FromStr;
+use strum_macros::{Display, EnumString};
+
+/// Enumeration of all the lint checks that can be performed.
+///
+/// With the use of `strum_macros::EnumString` and `strum_macros::Display`,
+/// each of the variants can be serialized to and deserialized from a string.
+/// The serialization follows "snake_case".
+#[derive(Copy, Clone, Ord, Eq, PartialEq, PartialOrd, EnumString, Display)]
+#[strum(serialize_all = "snake_case")]
+pub enum LintChecker {
+    BlocksInConditions,
+    UnnecessaryBooleanIdentityComparison,
+    UnnecessaryNumericalExtremeComparison,
+    WhileTrue,
+}
+
+/// Extract all the lint checks to skip from the given attributes.
+/// Also performs error-checking on any `LintAttribute::SKIP` attributes.
+pub fn lint_skips_from_attributes(env: &GlobalEnv, attrs: &[Attribute]) -> Vec<LintChecker> {
+    let lint_skip = env.symbol_pool().make(LintAttribute::SKIP);
+    let skip_attr = attrs.iter().find(|attr| attr.name() == lint_skip);
+    if let Some(skip_attr) = skip_attr {
+        parse_lint_skip_attribute(env, skip_attr)
+    } else {
+        vec![]
+    }
+}
+
+/// Extract all the lint checks to skip from `attr`.
+/// Also performs error-checking on the LintAttribute::SKIP `attr`.
+fn parse_lint_skip_attribute(env: &GlobalEnv, attr: &Attribute) -> Vec<LintChecker> {
+    match attr {
+        Attribute::Assign(id, ..) => {
+            env.error(
+                &env.get_node_loc(*id),
+                &format!(
+                    "expected `#[{}(...)]`, not an assigned value",
+                    LintAttribute::SKIP
+                ),
+            );
+            vec![]
+        },
+        Attribute::Apply(id, _, attrs) => {
+            if attrs.is_empty() {
+                env.error(
+                    &env.get_node_loc(*id),
+                    "no lint checks are specified to be skipped",
+                );
+            }
+            attrs
+            .iter()
+            .filter_map(|lint_check| match lint_check {
+                Attribute::Assign(id, ..) => {
+                    env.error(
+                        &env.get_node_loc(*id),
+                        "did not expect an assigned value, expected only the names of the lint checks to be skipped",
+                    );
+                    None
+                },
+                Attribute::Apply(id, name, sub_attrs) => {
+                    if !sub_attrs.is_empty() {
+                        env.error(&env.get_node_loc(*id), "unexpected nested attributes");
+                        None
+                    } else {
+                        let name = name.display(env.symbol_pool()).to_string();
+                        let checker = LintChecker::from_str(&name).ok();
+                        if checker.is_none() {
+                            env.error(
+                                &env.get_node_loc(*id),
+                                &format!("unknown lint check: `{}`", name),
+                            );
+                        }
+                        checker
+                    }
+                },
+            })
+            .collect()
+        },
+    }
+}

third_party/move/move-compiler-v2/tests/checking/attributes/aptos_stdlib_attributes.exp

@@ -4,13 +4,13 @@ warning: unknown attribute
   ┌─ tests/checking/attributes/aptos_stdlib_attributes.move:4:7
   │
 4 │     #[a, a(x = 0)]
-  │       ^ Attribute name 'a' is unknown (use --skip-attribute-checks CLI option to ignore); known attributes are '{"bytecode_instruction", "deprecated", "expected_failure", "native_interface", "test", "test_only", "verify_only"}'.
+  │       ^ Attribute name 'a' is unknown (use --skip-attribute-checks CLI option to ignore); known attributes are '{"bytecode_instruction", "deprecated", "expected_failure", "lint::skip", "native_interface", "test", "test_only", "verify_only"}'.
 
 warning: unknown attribute
   ┌─ tests/checking/attributes/aptos_stdlib_attributes.move:4:10
   │
 4 │     #[a, a(x = 0)]
-  │          ^ Attribute name 'a' is unknown (use --skip-attribute-checks CLI option to ignore); known attributes are '{"bytecode_instruction", "deprecated", "expected_failure", "native_interface", "test", "test_only", "verify_only"}'.
+  │          ^ Attribute name 'a' is unknown (use --skip-attribute-checks CLI option to ignore); known attributes are '{"bytecode_instruction", "deprecated", "expected_failure", "lint::skip", "native_interface", "test", "test_only", "verify_only"}'.
 
 error: duplicate declaration, item, or annotation
   ┌─ tests/checking/attributes/aptos_stdlib_attributes.move:4:10
@@ -24,13 +24,13 @@ warning: unknown attribute
   ┌─ tests/checking/attributes/aptos_stdlib_attributes.move:7:7
   │
 7 │     #[testonly]
-  │       ^^^^^^^^ Attribute name 'testonly' is unknown (use --skip-attribute-checks CLI option to ignore); known attributes are '{"bytecode_instruction", "deprecated", "expected_failure", "native_interface", "test", "test_only", "verify_only"}'.
+  │       ^^^^^^^^ Attribute name 'testonly' is unknown (use --skip-attribute-checks CLI option to ignore); known attributes are '{"bytecode_instruction", "deprecated", "expected_failure", "lint::skip", "native_interface", "test", "test_only", "verify_only"}'.
 
 warning: unknown attribute
   ┌─ tests/checking/attributes/aptos_stdlib_attributes.move:8:7
   │
 8 │     #[b(a, a = 0, a(x = 1))]
-  │       ^ Attribute name 'b' is unknown (use --skip-attribute-checks CLI option to ignore); known attributes are '{"bytecode_instruction", "deprecated", "expected_failure", "native_interface", "test", "test_only", "verify_only"}'.
+  │       ^ Attribute name 'b' is unknown (use --skip-attribute-checks CLI option to ignore); known attributes are '{"bytecode_instruction", "deprecated", "expected_failure", "lint::skip", "native_interface", "test", "test_only", "verify_only"}'.
 
 error: duplicate declaration, item, or annotation
   ┌─ tests/checking/attributes/aptos_stdlib_attributes.move:8:12

third_party/move/move-compiler-v2/tests/checking/attributes/aptos_stdlib_attributes2.exp

@@ -4,7 +4,7 @@ warning: unknown attribute
   ┌─ tests/checking/attributes/aptos_stdlib_attributes2.move:4:7
   │
 4 │     #[testonly]
-  │       ^^^^^^^^ Attribute name 'testonly' is unknown (use --skip-attribute-checks CLI option to ignore); known attributes are '{"bytecode_instruction", "deprecated", "expected_failure", "native_interface", "test", "test_only", "verify_only"}'.
+  │       ^^^^^^^^ Attribute name 'testonly' is unknown (use --skip-attribute-checks CLI option to ignore); known attributes are '{"bytecode_instruction", "deprecated", "expected_failure", "lint::skip", "native_interface", "test", "test_only", "verify_only"}'.
 
 // -- Model dump before bytecode pipeline
 module 0x1::M {