[move-compiler] Added unused constants warning (#13422)

## Description 

Added a warning about unused constants.

## Test Plan 

Added appropriate 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

When building Move code, new compiler warnings that point to unused
constants may appear.

external-crates/move/move-compiler/src/diagnostics/codes.rs

@@ -289,6 +289,7 @@ codes!(
         Function: { msg: "unused function", severity: Warning },
         StructField: { msg: "unused struct field", severity: Warning },
         FunTypeParam: { msg: "unused function type parameter", severity: Warning },
+        Constant: { msg: "unused constant", severity: Warning },
     ],
     Attributes: [
         Duplicate: { msg: "invalid duplicate attribute", severity: NonblockingError },

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

@@ -11,8 +11,8 @@ use crate::{
         WellKnownFilterName,
     },
     shared::{
-        ast_debug::AstDebug, FILTER_UNUSED_FUNCTION, FILTER_UNUSED_STRUCT_FIELD,
-        FILTER_UNUSED_TYPE_PARAMETER,
+        ast_debug::AstDebug, FILTER_UNUSED_CONST, FILTER_UNUSED_FUNCTION,
+        FILTER_UNUSED_STRUCT_FIELD, FILTER_UNUSED_TYPE_PARAMETER,
     },
 };
 use codespan_reporting::{
@@ -507,6 +507,7 @@ impl UnprefixedWarningFilters {
         let unused_fun_info = UnusedItem::Function.into_info();
         let unused_field_info = UnusedItem::StructField.into_info();
         let unused_fn_tparam_info = UnusedItem::FunTypeParam.into_info();
+        let unused_const_info = UnusedItem::Constant.into_info();
         let filtered_codes = BTreeMap::from([
             (
                 (unused_fun_info.category(), unused_fun_info.code()),
@@ -523,6 +524,10 @@ impl UnprefixedWarningFilters {
                 ),
                 Some(FILTER_UNUSED_TYPE_PARAMETER),
             ),
+            (
+                (unused_const_info.category(), unused_const_info.code()),
+                Some(FILTER_UNUSED_CONST),
+            ),
         ]);
         Self::Specified {
             categories: BTreeMap::new(),

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

@@ -151,6 +151,7 @@ pub const FILTER_UNUSED_ATTRIBUTE: &str = "unused_attribute";
 pub const FILTER_UNUSED_TYPE_PARAMETER: &str = "unused_type_parameter";
 pub const FILTER_UNUSED_FUNCTION: &str = "unused_function";
 pub const FILTER_UNUSED_STRUCT_FIELD: &str = "unused_field";
+pub const FILTER_UNUSED_CONST: &str = "unused_const";
 pub const FILTER_DEAD_CODE: &str = "dead_code";
 
 pub type NamedAddressMap = BTreeMap<Symbol, NumericalAddress>;
@@ -326,6 +327,7 @@ impl CompilationEnv {
                     },
                 ]),
             ),
+            known_code_filter!(FILTER_UNUSED_CONST, UnusedItem::Constant, filter_attr_name),
             known_code_filter!(FILTER_DEAD_CODE, UnusedItem::DeadCode, filter_attr_name),
         ]);
 

external-crates/move/move-compiler/src/typing/core.rs

@@ -5,7 +5,7 @@
 use crate::{
     diag,
     diagnostics::{codes::NameResolution, Diagnostic},
-    expansion::ast::{AbilitySet, ModuleIdent, Visibility},
+    expansion::ast::{AbilitySet, ModuleIdent, ModuleIdent_, Visibility},
     naming::ast::{
         self as N, BuiltinTypeName_, FunctionSignature, StructDefinition, StructTypeParameter,
         TParam, TParamID, TVar, Type, TypeName, TypeName_, Type_, Var,
@@ -88,14 +88,14 @@ pub struct Context<'env> {
 
     loop_info: LoopInfo,
 
-    /// collects all called functions in the current module
-    pub called_fns: BTreeSet<Symbol>,
-
     /// collects all friends that should be added over the course of 'public(package)' calls
     /// structured as (defining module, new friend, location) where `new friend` is usually the
     /// context's current module. Note there may be more than one location in practice, but
     /// tracking a single one is sufficient for error reporting.
     pub new_friends: BTreeSet<(ModuleIdent, Loc)>,
+    /// collects all used module members (functions and constants) but it's a superset of these in
+    /// that it may contain other identifiers that do not in fact represent a function or a constant
+    pub used_module_members: BTreeMap<ModuleIdent_, BTreeSet<Symbol>>,
 }
 
 macro_rules! program_info {
@@ -218,8 +218,8 @@ impl<'env> Context<'env> {
             loop_info: LoopInfo(LoopInfo_::NotInLoop),
             modules,
             env,
-            called_fns: BTreeSet::new(),
             new_friends: BTreeSet::new(),
+            used_module_members: BTreeMap::new(),
         }
     }
 

external-crates/move/move-compiler/src/typing/translate.rs

@@ -10,7 +10,10 @@ use crate::{
     diag,
     diagnostics::{codes::*, Diagnostic},
     editions::Flavor,
-    expansion::ast::{AttributeName_, Fields, Friend, ModuleIdent, Value_, Visibility},
+    expansion::ast::{
+        AttributeName_, AttributeValue_, Attribute_, Attributes, Fields, Friend, ModuleAccess_,
+        ModuleIdent, ModuleIdent_, Value_, Visibility,
+    },
     naming::ast::{self as N, TParam, TParamID, Type, TypeName_, Type_},
     parser::ast::{Ability_, BinOp_, ConstantName, Field, FunctionName, StructName, UnaryOp_},
     shared::{
@@ -86,6 +89,10 @@ fn modules(
             .expect("ICE compiler added duplicate friends to public(package) friend list");
     }
 
+    for (_, mident, mdef) in &typed_modules {
+        gen_unused_warnings(context, mident, mdef);
+    }
+
     typed_modules
 }
 
@@ -95,8 +102,6 @@ fn module(
     mdef: N::ModuleDefinition,
 ) -> (T::ModuleDefinition, BTreeSet<(ModuleIdent, Loc)>) {
     assert!(context.current_script_constants.is_none());
-    assert!(context.called_fns.is_empty());
-    assert!(context.new_friends.is_empty());
 
     context.current_module = Some(ident);
     let N::ModuleDefinition {
@@ -114,6 +119,7 @@ fn module(
     structs
         .iter_mut()
         .for_each(|(_, _, s)| struct_def(context, s));
+    process_attributes(context, &attributes);
     let constants = nconstants.map(|name, c| constant(context, name, c));
     let functions = nfunctions.map(|name, f| function(context, name, f, false));
     assert!(context.constraints.is_empty());
@@ -129,12 +135,8 @@ fn module(
         constants,
         functions,
     };
-    gen_unused_warnings(context, &typed_module);
     // get the list of new friends and reset the list.
     let new_friends = std::mem::take(&mut context.new_friends);
-    // reset called functions set so that it's ready to be be populated with values from
-    // a single module only
-    context.called_fns.clear();
     (typed_module, new_friends)
 }
 
@@ -202,6 +204,7 @@ fn function(
     assert!(context.constraints.is_empty());
     context.reset_for_module_item();
     context.current_function = Some(name);
+    process_attributes(context, &attributes);
     function_signature(context, &signature);
     if is_script {
         let mk_msg = || {
@@ -301,6 +304,8 @@ fn constant(context: &mut Context, _name: ConstantName, nconstant: N::Constant)
     } = nconstant;
     context.env.add_warning_filter_scope(warning_filter.clone());
 
+    process_attributes(context, &attributes);
+
     // Don't need to add base type constraint, as it is checked in `check_valid_constant::signature`
     let mut signature = core::instantiate(context, signature);
     check_valid_constant::signature(
@@ -1241,6 +1246,13 @@ fn exp_inner(context: &mut Context, sp!(eloc, ne_): N::Exp) -> T::Exp {
 
         NE::Constant(m, c) => {
             let ty = core::make_constant_type(context, eloc, &m, &c);
+            if let Some(mident) = m {
+                context
+                    .used_module_members
+                    .entry(mident.value)
+                    .or_insert_with(BTreeSet::new)
+                    .insert(c.value());
+            }
             (ty, TE::Constant(m, c))
         }
 
@@ -2102,9 +2114,11 @@ fn module_call(
         parameter_types: params_ty_list,
         acquires,
     };
-    if context.is_current_module(&m) {
-        context.called_fns.insert(f.value());
-    }
+    context
+        .used_module_members
+        .entry(m.value)
+        .or_insert_with(BTreeSet::new)
+        .insert(f.value());
     (ret_ty, T::UnannotatedExp_::ModuleCall(Box::new(call)))
 }
 
@@ -2318,12 +2332,38 @@ fn make_arg_types<S: std::fmt::Display, F: Fn() -> S>(
     given
 }
 
+//**************************************************************************************************
+// Utils
+//**************************************************************************************************
+
+fn process_attributes(context: &mut Context, all_attributes: &Attributes) {
+    for (_, _, attr) in all_attributes {
+        match &attr.value {
+            Attribute_::Name(_) => (),
+            Attribute_::Parameterized(_, attrs) => process_attributes(context, attrs),
+            Attribute_::Assigned(_, val) => {
+                let AttributeValue_::ModuleAccess(mod_access) = &val.value else {
+                    continue;
+                };
+                if let ModuleAccess_::ModuleAccess(mident, name) = mod_access.value {
+                    // conservatively assume that each `ModuleAccess` refers to a constant name
+                    context
+                        .used_module_members
+                        .entry(mident.value)
+                        .or_insert_with(BTreeSet::new)
+                        .insert(name.value);
+                }
+            }
+        }
+    }
+}
+
 //**************************************************************************************************
 // Module-wide warnings
 //**************************************************************************************************
 
-/// Generates warnings for unused (private) functions.
-fn gen_unused_warnings(context: &mut Context, mdef: &T::ModuleDefinition) {
+/// Generates warnings for unused (private) functions and unused constants.
+fn gen_unused_warnings(context: &mut Context, mident: &ModuleIdent_, mdef: &T::ModuleDefinition) {
     if !mdef.is_source_module {
         // generate warnings only for modules compiled in this pass rather than for all modules
         // including pre-compiled libraries for which we do not have source code available and
@@ -2336,6 +2376,22 @@ fn gen_unused_warnings(context: &mut Context, mdef: &T::ModuleDefinition) {
         .env
         .add_warning_filter_scope(mdef.warning_filter.clone());
 
+    for (loc, name, c) in &mdef.constants {
+        context
+            .env
+            .add_warning_filter_scope(c.warning_filter.clone());
+
+        let members = context.used_module_members.get(mident);
+        if members.is_none() || !members.unwrap().contains(name) {
+            let msg = format!("The constant '{name}' is never used. Consider removing it.");
+            context
+                .env
+                .add_diag(diag!(UnusedItem::Constant, (loc, msg)))
+        }
+
+        context.env.pop_warning_filter_scope();
+    }
+
     for (loc, name, fun) in &mdef.functions {
         if fun.attributes.iter().any(|(_, n, _)| {
             n == &AttributeName_::Known(KnownAttribute::Testing(TestingAttribute::Test))
@@ -2350,9 +2406,11 @@ fn gen_unused_warnings(context: &mut Context, mdef: &T::ModuleDefinition) {
         context
             .env
             .add_warning_filter_scope(fun.warning_filter.clone());
-        if !context.called_fns.contains(name)
-            && fun.entry.is_none()
+
+        let members = context.used_module_members.get(mident);
+        if fun.entry.is_none()
             && matches!(fun.visibility, Visibility::Internal)
+            && (members.is_none() || !members.unwrap().contains(name))
         {
             // TODO: postponing handling of friend functions until we decide what to do with them
             // vis-a-vis ideas around package-private

external-crates/move/move-compiler/tests/move_check/typing/unused_const.move

@@ -0,0 +1,3 @@
+module 0x42::unused_consts {
+    const UNUSED: u64 = 42;
+}

external-crates/move/move-compiler/tests/move_check/typing/unused_const.unused.exp

@@ -0,0 +1,8 @@
+warning[W09011]: unused constant
+  ┌─ tests/move_check/typing/unused_const.move:2:11
+  │
+2 │     const UNUSED: u64 = 42;
+  │           ^^^^^^ The constant 'UNUSED' is never used. Consider removing it.
+  │
+  = This warning can be suppressed with '#[allow(unused_const)]' applied to the 'module' or module member ('const', 'fun', or 'struct')
+

external-crates/move/move-compiler/tests/move_check/typing/used_const.move

@@ -0,0 +1,20 @@
+module 0x42::used_consts {
+
+    // at this point consts can only be used in functions and annotations (and not, for example, to
+    // define other constants) so we can only test for that
+
+    const USED_IN_FUN: u64 = 42;
+    const USED_IN_ANNOTATION: u64 = 42;
+
+    public fun foo(): u64 {
+        USED_IN_FUN
+    }
+
+    #[test(p = @42)]
+    #[expected_failure(abort_code = USED_IN_ANNOTATION)]
+    public fun baz(p: u64) {
+        assert!(p > 7, 42);
+    }
+
+
+}

external-crates/move/move-compiler/tests/move_check_testsuite.rs

@@ -135,7 +135,7 @@ fn testsuite(path: &Path, mut config: PackageConfig) -> datatest_stable::Result<
             path,
             Path::new(&unused_exp_path),
             Path::new(&unused_out_path),
-            Flags::empty(),
+            Flags::testing(),
             config.clone(),
         )?;
     }

external-crates/move/move-stdlib/docs/bit_vector.md

@@ -88,15 +88,6 @@ The maximum allowed bitvector size
 
 
 
-<a name="0x1_bit_vector_WORD_SIZE"></a>
-
-
-
-<pre><code><b>const</b> <a href="bit_vector.md#0x1_bit_vector_WORD_SIZE">WORD_SIZE</a>: u64 = 1;
-</code></pre>
-
-
-
 <a name="0x1_bit_vector_new"></a>
 
 ## Function `new`

external-crates/move/move-stdlib/sources/bit_vector.move

@@ -6,6 +6,7 @@ module std::bit_vector {
     /// An invalid length of bitvector was given
     const ELENGTH: u64 = 0x20001;
 
+    #[test_only]
     const WORD_SIZE: u64 = 1;
     /// The maximum allowed bitvector size
     const MAX_SIZE: u64 = 1024;

external-crates/move/move-stdlib/sources/error.move

@@ -48,6 +48,7 @@ module std::error {
   /// Out of gas or other forms of quota (http: 429)
   const RESOURCE_EXHAUSTED: u64 = 0x9;
 
+  #[allow(unused_const)]
   /// Request cancelled by the client (http: 499)
   const CANCELLED: u64 = 0xA;
 

external-crates/move/tools/move-cli/tests/sandbox_tests/print_values/sources/M.move

@@ -131,7 +131,9 @@ module M {
         print(&sender);
     }
 
+    #[allow(unused_const)]
     const MSG_1 : vector<u8> = b"abcdef";
+    #[allow(unused_const)]
     const MSG_2 : vector<u8> = b"123456";
 
     #[test_only]