[move-compiler] Added support for linter warning suppression (#13012)

## Description This implements the ability to add suppression of external warnings, particularly warnings coming from Sui linters. ## Test Plan Must pass all existing tests plus updated linter tests. --- If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process. ### Type of Change (Check all that apply) - [ ] protocol change - [x] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes Developers can now selectively suppress linter warnings. Linter warnings now also feature an additional message describing how they can be suppressed.
MystenLabs · Jul 20, 2023 · e017e8e · e017e8e · github-actions · Jul 20, 2023
1 parent fd69deb
commit e017e8e
Show file tree

Hide file tree

Showing 16 changed files with 575 additions and 169 deletions.
diff --git a/crates/sui-move-build/src/lib.rs b/crates/sui-move-build/src/lib.rs
@@ -53,7 +53,9 @@ use sui_types::{
 };
 use sui_verifier::verifier as sui_bytecode_verifier;
 
-use crate::linters::{self_transfer::SelfTransferVerifier, share_owned::ShareOwnedVerifier};
+use crate::linters::{
+    known_filters, self_transfer::SelfTransferVerifier, share_owned::ShareOwnedVerifier,
+};
 
 #[cfg(test)]
 #[path = "unit_tests/build_tests.rs"]
@@ -134,7 +136,11 @@ impl BuildConfig {
             let (files, units_res) = if lint {
                 let lint_visitors =
                     vec![ShareOwnedVerifier.visitor(), SelfTransferVerifier.visitor()];
-                compiler.add_visitors(lint_visitors).build()?
+                let (filter_attr_name, filters) = known_filters();
+                compiler
+                    .add_visitors(lint_visitors)
+                    .add_custom_known_filters(filters, filter_attr_name)
+                    .build()?
             } else {
                 compiler.build()?
             };

diff --git a/crates/sui-move-build/src/linters/mod.rs b/crates/sui-move-build/src/linters/mod.rs
@@ -1,5 +1,46 @@
 // Copyright (c) Mysten Labs, Inc.
 // SPDX-License-Identifier: Apache-2.0
 
+use move_compiler::{
+    diagnostics::codes::{DiagnosticsID, WarningFilter},
+    expansion::ast as E,
+};
+
 pub mod self_transfer;
 pub mod share_owned;
+
+pub const SHARE_OWNED_DIAG_CATEGORY: u8 = 1;
+pub const SHARE_OWNED_DIAG_CODE: u8 = 1;
+pub const SELF_TRANSFER_DIAG_CATEGORY: u8 = 2;
+pub const SELF_TRANSFER_DIAG_CODE: u8 = 1;
+
+pub const ALLOW_ATTR_NAME: &str = "lint_allow";
+pub const LINT_WARNING_PREFIX: &str = "Lint ";
+
+pub const SHARE_OWNED_FILTER_NAME: &str = "share_owned";
+pub const SELF_TRANSFER_FILTER_NAME: &str = "self_transfer";
+
+pub fn known_filters() -> (E::AttributeName_, Vec<WarningFilter>) {
+    (
+        E::AttributeName_::Unknown(ALLOW_ATTR_NAME.into()),
+        vec![
+            WarningFilter::All(Some(LINT_WARNING_PREFIX)),
+            WarningFilter::Code(
+                DiagnosticsID::new(
+                    SHARE_OWNED_DIAG_CATEGORY,
+                    SHARE_OWNED_DIAG_CODE,
+                    Some(LINT_WARNING_PREFIX),
+                ),
+                Some(SHARE_OWNED_FILTER_NAME),
+            ),
+            WarningFilter::Code(
+                DiagnosticsID::new(
+                    SELF_TRANSFER_DIAG_CATEGORY,
+                    SELF_TRANSFER_DIAG_CODE,
+                    Some(LINT_WARNING_PREFIX),
+                ),
+                Some(SELF_TRANSFER_FILTER_NAME),
+            ),
+        ],
+    )
+}
diff --git a/crates/sui-move-build/src/linters/self_transfer.rs b/crates/sui-move-build/src/linters/self_transfer.rs
@@ -26,6 +26,8 @@ use move_compiler::{
 use move_symbol_pool::Symbol;
 use std::collections::BTreeMap;
 
+use super::{SELF_TRANSFER_DIAG_CATEGORY, SELF_TRANSFER_DIAG_CODE};
+
 const TRANSFER_FUNCTIONS: &[(&str, &str, &str)] = &[
     ("sui", "transfer", "public_transfer"),
     ("sui", "transfer", "transfer"),
@@ -36,8 +38,8 @@ const INVALID_LOC: Loc = Loc::invalid();
 const SELF_TRANSFER_DIAG: DiagnosticInfo = custom(
     "Lint ",
     Severity::Warning,
-    /* category */ 1,
-    /* code */ 1,
+    SELF_TRANSFER_DIAG_CATEGORY,
+    SELF_TRANSFER_DIAG_CODE,
     "non-composable transfer to sender",
 );
 

diff --git a/crates/sui-move-build/src/linters/share_owned.rs b/crates/sui-move-build/src/linters/share_owned.rs
@@ -30,6 +30,8 @@ use move_compiler::{
 };
 use std::collections::BTreeMap;
 
+use super::{SHARE_OWNED_DIAG_CATEGORY, SHARE_OWNED_DIAG_CODE};
+
 const SHARE_FUNCTIONS: &[(&str, &str, &str)] = &[
     ("sui", "transfer", "public_share_object"),
     ("sui", "transfer", "share_object"),
@@ -38,8 +40,8 @@ const SHARE_FUNCTIONS: &[(&str, &str, &str)] = &[
 const SHARE_OWNED_DIAG: DiagnosticInfo = custom(
     "Lint ",
     Severity::Warning,
-    /* category */ 1,
-    /* code */ 1,
+    SHARE_OWNED_DIAG_CATEGORY,
+    SHARE_OWNED_DIAG_CODE,
     "possible owned object share",
 );
 

diff --git a/crates/sui-move-build/tests/linter/self_transfer.exp b/crates/sui-move-build/tests/linter/self_transfer.exp
@@ -1,4 +1,4 @@
-warning[Lint W01001]: non-composable transfer to sender
+warning[Lint W02001]: non-composable transfer to sender
    ┌─ tests/linter/self_transfer.move:23:9
    │
 22 │     public fun public_transfer_bad(ctx: &mut TxContext) {
@@ -8,8 +8,10 @@ warning[Lint W01001]: non-composable transfer to sender
    │         │                                                       │
    │         │                                                       Transaction sender address coming from here
    │         Transfer of an object to transaction sender address in function public_transfer_bad
+   │
+   = This warning can be suppressed with '#[lint_allow(self_transfer)]' applied to the 'module' or module member ('const', 'fun', or 'struct')
 
-warning[Lint W01001]: non-composable transfer to sender
+warning[Lint W02001]: non-composable transfer to sender
    ┌─ tests/linter/self_transfer.move:27:9
    │
 26 │     public fun private_transfer_bad(ctx: &mut TxContext) {
@@ -19,8 +21,10 @@ warning[Lint W01001]: non-composable transfer to sender
    │         │                                                │
    │         │                                                Transaction sender address coming from here
    │         Transfer of an object to transaction sender address in function private_transfer_bad
+   │
+   = This warning can be suppressed with '#[lint_allow(self_transfer)]' applied to the 'module' or module member ('const', 'fun', or 'struct')
 
-warning[Lint W01001]: non-composable transfer to sender
+warning[Lint W02001]: non-composable transfer to sender
    ┌─ tests/linter/self_transfer.move:31:9
    │
 30 │     public fun private_transfer_no_store_bad(ctx: &mut TxContext) {
@@ -30,15 +34,19 @@ warning[Lint W01001]: non-composable transfer to sender
    │         │                                                │
    │         │                                                Transaction sender address coming from here
    │         Transfer of an object to transaction sender address in function private_transfer_no_store_bad
+   │
+   = This warning can be suppressed with '#[lint_allow(self_transfer)]' applied to the 'module' or module member ('const', 'fun', or 'struct')
 
-warning[Lint W01001]: non-composable transfer to sender
-   ┌─ tests/linter/self_transfer.move:37:9
+warning[Lint W02001]: non-composable transfer to sender
+   ┌─ tests/linter/self_transfer.move:39:9
    │
-34 │     public fun transfer_through_assigns_bad(ctx: &mut TxContext) {
+36 │     public fun transfer_through_assigns_bad(ctx: &mut TxContext) {
    │                ---------------------------- Returning an object from a function, allows a caller to use the object and enables composability via programmable transactions.
-35 │         let sender = tx_context::sender(ctx);
+37 │         let sender = tx_context::sender(ctx);
    │                      ----------------------- Transaction sender address coming from here
-36 │         let another_sender = sender;
-37 │         transfer::public_transfer(S1 { id: object::new(ctx), }, another_sender)
+38 │         let another_sender = sender;
+39 │         transfer::public_transfer(S1 { id: object::new(ctx), }, another_sender)
    │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Transfer of an object to transaction sender address in function transfer_through_assigns_bad
+   │
+   = This warning can be suppressed with '#[lint_allow(self_transfer)]' applied to the 'module' or module member ('const', 'fun', or 'struct')
 
diff --git a/crates/sui-move-build/tests/linter/self_transfer.move b/crates/sui-move-build/tests/linter/self_transfer.move
@@ -31,6 +31,8 @@ module 0x42::test {
         transfer::transfer(S2 { id: object::new(ctx), }, tx_context::sender(ctx))
     }
 
+    // non-linter suppression annotation should not suppress linter warnings
+    #[allow(all)]
     public fun transfer_through_assigns_bad(ctx: &mut TxContext) {
         let sender = tx_context::sender(ctx);
         let another_sender = sender;
@@ -47,4 +49,9 @@ module 0x42::test {
         transfer::public_transfer(S1 { id: object::new(ctx), }, xfer_address);
         transfer::transfer(S1 { id: object::new(ctx), }, xfer_address);
     }
+
+    #[lint_allow(self_transfer)]
+    public fun public_transfer_bad_suppressed(ctx: &mut TxContext) {
+        transfer::public_transfer(S1 { id: object::new(ctx), }, tx_context::sender(ctx))
+    }
 }
diff --git a/crates/sui-move-build/tests/linter/share_owned.exp b/crates/sui-move-build/tests/linter/share_owned.exp
@@ -9,6 +9,8 @@ warning[Lint W01001]: possible owned object share
    │         │                             │
    │         │                             Creating a fresh object and sharing it within the same function will ensure this does not abort.
    │         Potential abort from a (potentially) owned object created by a different transaction.
+   │
+   = This warning can be suppressed with '#[lint_allow(share_owned)]' applied to the 'module' or module member ('const', 'fun', or 'struct')
 
 warning[Lint W01001]: possible owned object share
    ┌─ tests/linter/share_owned.move:35:9
@@ -20,4 +22,22 @@ warning[Lint W01001]: possible owned object share
    │         │                             │
    │         │                             Creating a fresh object and sharing it within the same function will ensure this does not abort.
    │         Potential abort from a (potentially) owned object created by a different transaction.
+   │
+   = This warning can be suppressed with '#[lint_allow(share_owned)]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[W09008]: unused function
+   ┌─ tests/linter/share_owned.move:48:9
+   │
+48 │     fun private_fun_should_not_be_suppressed() {}
+   │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ The non-'public', non-'entry' function 'private_fun_should_not_be_suppressed' is never called. Consider removing it.
+   │
+   = This warning can be suppressed with '#[allow(unused_function)]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+
+warning[W09008]: unused function
+   ┌─ tests/linter/share_owned.move:52:9
+   │
+52 │     fun another_private_fun_should_not_be_suppressed() {}
+   │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ The non-'public', non-'entry' function 'another_private_fun_should_not_be_suppressed' is never called. Consider removing it.
+   │
+   = This warning can be suppressed with '#[allow(unused_function)]' applied to the 'module' or module member ('const', 'fun', or 'struct')
 
diff --git a/crates/sui-move-build/tests/linter/share_owned.move b/crates/sui-move-build/tests/linter/share_owned.move
@@ -35,4 +35,21 @@ module 0x42::test2 {
         transfer::public_share_object(o);
         object::delete(id);
     }
+
+    #[lint_allow(share_owned)]
+    public entry fun unpack_obj_suppressed(w: Wrapper) {
+        let Wrapper { id, i: _, o } = w;
+        transfer::public_share_object(o);
+        object::delete(id);
+    }
+
+    // a linter suppression should not work for regular compiler warnings
+    #[linter_allow(code_suppression_should_not_work)]
+    fun private_fun_should_not_be_suppressed() {}
+
+    // a linter suppression should not work for regular compiler warnings
+    #[linter_allow(category_suppression_should_not_work)]
+    fun another_private_fun_should_not_be_suppressed() {}
+
+
 }
diff --git a/crates/sui-move-build/tests/linter_tests.rs b/crates/sui-move-build/tests/linter_tests.rs
@@ -10,13 +10,16 @@ use move_command_line_common::{
 use move_compiler::{
     cfgir::visitor::AbstractInterpreterVisitor,
     command_line::compiler::move_check_for_errors,
+    diagnostics::codes::{self, CategoryID, DiagnosticsID, WarningFilter},
     editions::Flavor,
+    expansion::ast as E,
     shared::{NumericalAddress, PackageConfig},
     Compiler, PASS_PARSER,
 };
 
 use sui_move_build::linters::{
-    self_transfer::SelfTransferVerifier, share_owned::ShareOwnedVerifier,
+    known_filters, self_transfer::SelfTransferVerifier, share_owned::ShareOwnedVerifier,
+    LINT_WARNING_PREFIX,
 };
 
 const SUI_FRAMEWORK_PATH: &str = "../sui-framework/packages/sui-framework";
@@ -35,11 +38,32 @@ fn linter_tests(path: &Path) -> datatest_stable::Result<()> {
     Ok(())
 }
 
+pub fn known_filters_for_test() -> (E::AttributeName_, Vec<WarningFilter>) {
+    let (filter_attr_name, mut filters) = known_filters();
+
+    let unused_function_code_filter = WarningFilter::Code(
+        DiagnosticsID::new(
+            codes::Category::UnusedItem as u8,
+            codes::UnusedItem::Function as u8,
+            Some(LINT_WARNING_PREFIX),
+        ),
+        Some("code_suppression_should_not_work"),
+    );
+    let unused_function_category_filter = WarningFilter::Category(
+        CategoryID::new(codes::Category::UnusedItem as u8, Some(LINT_WARNING_PREFIX)),
+        Some("category_suppression_should_not_work"),
+    );
+    filters.push(unused_function_code_filter);
+    filters.push(unused_function_category_filter);
+    (filter_attr_name, filters)
+}
+
 fn run_tests(path: &Path) -> anyhow::Result<()> {
     let exp_path = path.with_extension(EXP_EXT);
 
     let targets: Vec<String> = vec![path.to_str().unwrap().to_owned()];
     let lint_visitors = vec![ShareOwnedVerifier.visitor(), SelfTransferVerifier.visitor()];
+    let (filter_attr_name, filters) = known_filters_for_test();
     let (files, comments_and_compiler_res) = Compiler::from_files(
         targets,
         vec![MOVE_STDLIB_PATH.to_string(), SUI_FRAMEWORK_PATH.to_string()],
@@ -50,6 +74,7 @@ fn run_tests(path: &Path) -> anyhow::Result<()> {
         flavor: Flavor::Sui,
         ..PackageConfig::default()
     })
+    .add_custom_known_filters(filters, filter_attr_name)
     .run::<PASS_PARSER>()?;
 
     let diags = move_check_for_errors(comments_and_compiler_res);

diff --git a/external-crates/move/move-compiler/src/cfgir/translate.rs b/external-crates/move/move-compiler/src/cfgir/translate.rs
@@ -720,13 +720,21 @@ fn visit_module(
     mident: ModuleIdent,
     mdef: &G::ModuleDefinition,
 ) {
+    context
+        .env
+        .add_warning_filter_scope(mdef.warning_filter.clone());
     for (name, fdef) in mdef.functions.key_cloned_iter() {
         visit_function(context, prog, Some(mident), name, fdef)
     }
+    context.env.pop_warning_filter_scope();
 }
 
 fn visit_script(context: &mut Context, prog: &G::Program, script: &G::Script) {
-    visit_function(context, prog, None, script.function_name, &script.function)
+    context
+        .env
+        .add_warning_filter_scope(script.warning_filter.clone());
+    visit_function(context, prog, None, script.function_name, &script.function);
+    context.env.pop_warning_filter_scope();
 }
 
 fn visit_function(
@@ -737,7 +745,7 @@ fn visit_function(
     fdef: &G::Function,
 ) {
     let G::Function {
-        warning_filter: _,
+        warning_filter,
         index: _,
         attributes: _,
         visibility: _,
@@ -749,6 +757,7 @@ fn visit_function(
     let G::FunctionBody_::Defined { locals, start, blocks, block_info } = &body.value else {
         return
     };
+    context.env.add_warning_filter_scope(warning_filter.clone());
     let (cfg, infinite_loop_starts) = ImmForwardCFG::new(*start, blocks, block_info.iter());
     let function_context = super::CFGContext {
         module: mident,
@@ -764,5 +773,6 @@ fn visit_function(
         let mut v = visitor.borrow_mut();
         ds.extend(v.verify(&context.env, prog, &function_context, &cfg));
     }
-    context.env.add_diags(ds)
+    context.env.add_diags(ds);
+    context.env.pop_warning_filter_scope();
 }