Skip to content

Commit

Permalink
[move-compiler] Added a warning for unused function type params (Myst…
Browse files Browse the repository at this point in the history
…enLabs#13112)

## Description 

Added a warning for unused function type params. Kept the same
suppression annotation as for struct type params which required a small
change to how known warning filters are stored.

## Test Plan 

Added new 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 may appear pointing
towards unused function type parameters
  • Loading branch information
awelc committed Aug 2, 2023
1 parent 8e67b9c commit 8c56af4
Show file tree
Hide file tree
Showing 16 changed files with 202 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ codes!(
Attribute: { msg: "unused attribute", severity: Warning },
Function: { msg: "unused function", severity: Warning },
StructField: { msg: "unused struct field", severity: Warning },
FunTypeParam: { msg: "unused function type parameter", severity: Warning },
],
Attributes: [
Duplicate: { msg: "invalid duplicate attribute", severity: NonblockingError },
Expand Down
14 changes: 13 additions & 1 deletion external-crates/move/move-compiler/src/diagnostics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ use crate::{
diagnostics::codes::{
CategoryID, DiagnosticCode, DiagnosticInfo, DiagnosticsID, Severity, WarningFilter,
},
shared::{ast_debug::AstDebug, FILTER_UNUSED_FUNCTION, FILTER_UNUSED_STRUCT_FIELD},
shared::{
ast_debug::AstDebug, FILTER_UNUSED_FUNCTION, FILTER_UNUSED_STRUCT_FIELD,
FILTER_UNUSED_TYPE_PARAMETER,
},
};
use codespan_reporting::{
self as csr,
Expand Down Expand Up @@ -446,6 +449,7 @@ impl WarningFilters {
pub fn unused_warnings_filter_for_test() -> Self {
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 filtered_codes = BTreeMap::from([
(
DiagnosticsID::new(
Expand All @@ -463,6 +467,14 @@ impl WarningFilters {
),
Some(FILTER_UNUSED_STRUCT_FIELD),
),
(
DiagnosticsID::new(
unused_fn_tparam_info.category() as u8,
unused_fn_tparam_info.code(),
None,
),
Some(FILTER_UNUSED_TYPE_PARAMETER),
),
]);
WarningFilters::Specified {
category: BTreeMap::new(),
Expand Down
11 changes: 8 additions & 3 deletions external-crates/move/move-compiler/src/expansion/translate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -770,14 +770,19 @@ fn warning_filter(
n
}
};
let Some(filter) = context.env.filter_from_str(name_.to_string(), allow.clone()) else {
let filters = context
.env
.filter_from_str(name_.to_string(), allow.clone());
if filters.is_empty() {
let msg = format!("Unknown warning filter '{name_}'");
context
.env
.add_diag(diag!(Attributes::InvalidValue, (attr.loc, msg)));
continue
continue;
};
warning_filters.add(filter);
for f in filters {
warning_filters.add(f);
}
}
}
warning_filters
Expand Down
30 changes: 29 additions & 1 deletion external-crates/move/move-compiler/src/naming/translate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use move_ir_types::location::*;
use move_symbol_pool::Symbol;
use std::collections::{BTreeMap, BTreeSet};

use super::fake_natives;
use super::{ast::TParamID, fake_natives};

//**************************************************************************************************
// Context
Expand Down Expand Up @@ -53,6 +53,11 @@ struct Context<'env> {
local_scopes: Vec<BTreeMap<Symbol, u16>>,
local_count: BTreeMap<Symbol, u16>,
used_locals: BTreeSet<N::Var_>,
/// Type parameters used in a function (they have to be cleared after processing each function).
used_fun_tparams: BTreeSet<TParamID>,
/// Indicates if the compiler is currently translating a function (set to true before starting
/// to translate a function and to false after translation is over).
translating_fun: bool,
}

impl<'env> Context<'env> {
Expand Down Expand Up @@ -123,6 +128,8 @@ impl<'env> Context<'env> {
local_scopes: vec![],
local_count: BTreeMap::new(),
used_locals: BTreeSet::new(),
used_fun_tparams: BTreeSet::new(),
translating_fun: false,
}
}

Expand Down Expand Up @@ -556,12 +563,28 @@ fn function(
assert!(context.local_scopes.is_empty());
assert!(context.local_count.is_empty());
assert!(context.used_locals.is_empty());
assert!(context.used_fun_tparams.is_empty());
assert!(!context.translating_fun);
context.env.add_warning_filter_scope(warning_filter.clone());
context.local_scopes = vec![BTreeMap::new()];
context.local_count = BTreeMap::new();
context.translating_fun = true;
let signature = function_signature(context, signature);
let acquires = function_acquires(context, acquires);
let body = function_body(context, body);

if !matches!(body.value, N::FunctionBody_::Native) {
for tparam in &signature.type_parameters {
if !context.used_fun_tparams.contains(&tparam.id) {
let sp!(loc, n) = tparam.user_specified_name;
let msg = format!("Unused type parameter '{}'.", n);
context
.env
.add_diag(diag!(UnusedItem::FunTypeParam, (loc, msg)))
}
}
}

let mut f = N::Function {
warning_filter,
index,
Expand All @@ -578,7 +601,9 @@ fn function(
context.local_scopes = vec![];
context.local_count = BTreeMap::new();
context.used_locals = BTreeSet::new();
context.used_fun_tparams = BTreeSet::new();
context.env.pop_warning_filter_scope();
context.translating_fun = false;
f
}

Expand Down Expand Up @@ -888,6 +913,9 @@ fn type_(context: &mut Context, sp!(loc, ety_): E::Type) -> N::Type {
));
NT::UnresolvedError
} else {
if context.translating_fun {
context.used_fun_tparams.insert(tp.id);
}
NT::Param(tp)
}
}
Expand Down
68 changes: 47 additions & 21 deletions external-crates/move/move-compiler/src/shared/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ pub struct CompilationEnv {
/// Config for any package not found in `package_configs`, or for inputs without a package.
default_config: PackageConfig,
/// Maps warning filter key (filter name and filter attribute name) to the filter itself.
known_filters: BTreeMap<KnownFilterInfo, WarningFilter>,
known_filters: BTreeMap<KnownFilterInfo, BTreeSet<WarningFilter>>,
/// Maps a diagnostics ID to a known filter name.
known_filter_names: BTreeMap<DiagnosticsID, KnownFilterInfo>,
/// Attribute names (including externally provided ones) identifying known warning filters.
Expand All @@ -236,10 +236,10 @@ macro_rules! known_code_filter {
($name:ident, $category:ident::$code:ident, $attr_name:ident) => {
(
KnownFilterInfo::new($name, $attr_name),
WarningFilter::Code(
BTreeSet::from([WarningFilter::Code(
DiagnosticsID::new(Category::$category as u8, $category::$code as u8, None),
Some($name),
),
)]),
)
};
}
Expand All @@ -263,14 +263,14 @@ impl CompilationEnv {
let known_filters = BTreeMap::from([
(
KnownFilterInfo::new(FILTER_ALL, filter_attr_name),
WarningFilter::All(None),
BTreeSet::from([WarningFilter::All(None)]),
),
(
KnownFilterInfo::new(FILTER_UNUSED, filter_attr_name),
WarningFilter::Category(
BTreeSet::from([WarningFilter::Category(
CategoryID::new(Category::UnusedItem as u8, None),
Some(FILTER_UNUSED),
),
)]),
),
known_code_filter!(
FILTER_MISSING_PHANTOM,
Expand Down Expand Up @@ -298,11 +298,6 @@ impl CompilationEnv {
UnusedItem::Attribute,
filter_attr_name
),
known_code_filter!(
FILTER_UNUSED_TYPE_PARAMETER,
UnusedItem::StructTypeParam,
filter_attr_name
),
known_code_filter!(
FILTER_UNUSED_FUNCTION,
UnusedItem::Function,
Expand All @@ -313,17 +308,40 @@ impl CompilationEnv {
UnusedItem::StructField,
filter_attr_name
),
(
KnownFilterInfo::new(FILTER_UNUSED_TYPE_PARAMETER, filter_attr_name),
BTreeSet::from([
WarningFilter::Code(
DiagnosticsID::new(
Category::UnusedItem as u8,
UnusedItem::StructTypeParam as u8,
None,
),
Some(FILTER_UNUSED_TYPE_PARAMETER),
),
WarningFilter::Code(
DiagnosticsID::new(
Category::UnusedItem as u8,
UnusedItem::FunTypeParam as u8,
None,
),
Some(FILTER_UNUSED_TYPE_PARAMETER),
),
]),
),
known_code_filter!(FILTER_DEAD_CODE, UnusedItem::DeadCode, filter_attr_name),
]);

let known_filter_names: BTreeMap<DiagnosticsID, KnownFilterInfo> = known_filters
.iter()
.filter_map(|(known_filter_info, v)| {
if let WarningFilter::Code(diag_id, _) = v {
Some((*diag_id, known_filter_info.clone()))
} else {
None
}
.flat_map(|(known_filter_info, filters)| {
filters.iter().filter_map(|v| {
if let WarningFilter::Code(diag_id, _) = v {
Some((*diag_id, known_filter_info.clone()))
} else {
None
}
})
})
.collect();

Expand Down Expand Up @@ -435,10 +453,11 @@ impl CompilationEnv {
&self,
name: String,
attribute_name: E::AttributeName_,
) -> Option<WarningFilter> {
) -> BTreeSet<WarningFilter> {
self.known_filters
.get(&KnownFilterInfo::new(name.as_str(), attribute_name))
.cloned()
.unwrap_or_else(BTreeSet::new)
}

pub fn filter_attributes(&self) -> &BTreeSet<E::AttributeName_> {
Expand All @@ -455,21 +474,28 @@ impl CompilationEnv {
match filter {
WarningFilter::All(_) => {
self.known_filters
.insert(KnownFilterInfo::new(FILTER_ALL, filter_attr_name), filter);
.entry(KnownFilterInfo::new(FILTER_ALL, filter_attr_name))
.or_insert_with(BTreeSet::new)
.insert(filter);
}
WarningFilter::Category(_, name) => {
let Some(n) = name else {
anyhow::bail!("A known Category warning filter must have a name specified");
};
self.known_filters
.insert(KnownFilterInfo::new(n, filter_attr_name), filter);
.entry(KnownFilterInfo::new(n, filter_attr_name))
.or_insert_with(BTreeSet::new)
.insert(filter);
}
WarningFilter::Code(diag_id, name) => {
let Some(n) = name else {
anyhow::bail!("A known Code warning filter must have a name specified");
};
let known_filter_info = KnownFilterInfo::new(n, filter_attr_name);
self.known_filters.insert(known_filter_info.clone(), filter);
self.known_filters
.entry(known_filter_info.clone())
.or_insert_with(BTreeSet::new)
.insert(filter);
self.known_filter_names.insert(diag_id, known_filter_info);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
module 0x42::unused_fun_tparam {

public fun unused<T>(): u64 {
42
}

public fun one_unused<T1, T2>(v: T1): T1 {
v
}

public fun all_unused<T1, T2>(): u64 {
42
}

}
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
warning[W09010]: unused function type parameter
┌─ tests/move_check/naming/unused_fun_tparam.move:3:23
3 │ public fun unused<T>(): u64 {
│ ^ Unused type parameter 'T'.
= This warning can be suppressed with '#[allow(unused_type_parameter)]' applied to the 'module' or module member ('const', 'fun', or 'struct')

warning[W09010]: unused function type parameter
┌─ tests/move_check/naming/unused_fun_tparam.move:7:31
7 │ public fun one_unused<T1, T2>(v: T1): T1 {
│ ^^ Unused type parameter 'T2'.
= This warning can be suppressed with '#[allow(unused_type_parameter)]' applied to the 'module' or module member ('const', 'fun', or 'struct')

warning[W09010]: unused function type parameter
┌─ tests/move_check/naming/unused_fun_tparam.move:11:27
11 │ public fun all_unused<T1, T2>(): u64 {
│ ^^ Unused type parameter 'T1'.
= This warning can be suppressed with '#[allow(unused_type_parameter)]' applied to the 'module' or module member ('const', 'fun', or 'struct')

warning[W09010]: unused function type parameter
┌─ tests/move_check/naming/unused_fun_tparam.move:11:31
11 │ public fun all_unused<T1, T2>(): u64 {
│ ^^ Unused type parameter 'T2'.
= This warning can be suppressed with '#[allow(unused_type_parameter)]' applied to the 'module' or module member ('const', 'fun', or 'struct')

Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
module 0x42::used_fun_tparam {
// no warnings related to unused function type params should be generated for functions in this
// module


struct S<phantom T: key + drop> has key, drop {
}

public fun foo<T>(): T {
abort 0
}

public fun bar<T>(_: T) {
abort 0
}


public fun no_warn_sig_direct<T>(v: T): T {
v
}

public fun no_warn_sig_indirect<T: key + drop>(v: S<T>): S<T> {
v
}

public fun no_warn_pack<T: key + drop>() {
let _ = S<T> {};
}

public fun no_warn_pack_unpack<T: key + drop>() {
let x = S {};
let S<T> {} = x;
}

public fun no_warn_bind<T: key + drop>() {
let _: T = foo();
}

public fun no_warn_call<T: key + drop>() {
let _ = foo<T>();
}

public fun no_warn_annotation<T: key + drop>() {
let x = foo();
bar((x: T));
}




}
Empty file.
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
script {
#[allow(unused_type_parameter)]
/// This script does really nothing but just aborts.
fun some_script<Token>(_account: signer, code: u64) {
abort code
Expand Down
Loading

0 comments on commit 8c56af4

Please sign in to comment.