Skip to content

Commit

Permalink
[move] Turn off some lints for tests (MystenLabs#15424)
Browse files Browse the repository at this point in the history
## Description 

- Disabled most Sui lints for tests and test only functions 
  - coin_field
  - freeze_wrapped
  - self transfer
  - share owned
  - freeze wrapped
- Disabled a lint for entry functions (since you cannot return objects)
  - self transfer 

Turned on the lints for sui-framework-test
  
## Test Plan 

- 👀 ran sui-framework-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

The Sui specific lints from Move have been silenced in nearly all cases for test or test only functions. Additionally, the self transfer lint has been silenced for `entry` functions.
  • Loading branch information
tnowacki authored Dec 19, 2023
1 parent 59d7f5b commit 69104e3
Show file tree
Hide file tree
Showing 16 changed files with 161 additions and 79 deletions.
8 changes: 2 additions & 6 deletions crates/sui-framework-tests/src/unit_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ fn check_package_builds(path: PathBuf) {
config.print_diags_to_stderr = true;
config.config.warnings_are_errors = true;
config.config.silence_warnings = false;

config.config.no_lint = false;
config
.build(path.clone())
.unwrap_or_else(|e| panic!("Building package {}.\nWith error {e}", path.display()));
Expand All @@ -106,15 +106,11 @@ fn check_move_unit_tests(path: PathBuf) {
config.print_diags_to_stderr = true;
config.config.warnings_are_errors = true;
config.config.silence_warnings = false;
config.config.no_lint = false;
let move_config = config.config.clone();
let mut testing_config = UnitTestingConfig::default_with_bound(Some(3_000_000));
testing_config.filter = std::env::var(FILTER_ENV).ok().map(|s| s.to_string());

// build tests first to enable Sui-specific test code verification
config
.build(path.clone())
.unwrap_or_else(|e| panic!("Building tests at {}.\nWith error {e}", path.display()));

assert_eq!(
run_move_unit_tests(path, move_config, Some(testing_config), false).unwrap(),
UnitTestResult::Success
Expand Down
22 changes: 12 additions & 10 deletions crates/sui-framework/packages/deepbook/sources/clob_v2.move
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,8 @@ module deepbook::clob_v2 {

/// Capability granting permission to access an entry in `Pool.quote_asset_trading_fees`.
/// The pool objects created for older pools do not have a PoolOwnerCap because they were created
/// prior to the addition of this feature. Here is a list of 11 pools on mainnet that
/// do not have this capability:
/// prior to the addition of this feature. Here is a list of 11 pools on mainnet that
/// do not have this capability:
/// 0x31d1790e617eef7f516555124155b28d663e5c600317c769a75ee6336a54c07f
/// 0x6e417ee1c12ad5f2600a66bc80c7bd52ff3cb7c072d508700d17cf1325324527
/// 0x17625f1a241d34d2da0dc113086f67a2b832e3e8cd8006887c195cd24d3598a3
Expand All @@ -272,21 +272,21 @@ module deepbook::clob_v2 {

/// Accessor functions
public fun usr_open_orders_exist<BaseAsset, QuoteAsset>(
pool: &Pool<BaseAsset, QuoteAsset>,
pool: &Pool<BaseAsset, QuoteAsset>,
owner: address
): bool {
table::contains(&pool.usr_open_orders, owner)
}

public fun usr_open_orders_for_address<BaseAsset, QuoteAsset>(
pool: &Pool<BaseAsset, QuoteAsset>,
pool: &Pool<BaseAsset, QuoteAsset>,
owner: address
): &LinkedTable<u64, u64> {
table::borrow(&pool.usr_open_orders, owner)
}

public fun usr_open_orders<BaseAsset, QuoteAsset>(
pool: &Pool<BaseAsset, QuoteAsset>,
pool: &Pool<BaseAsset, QuoteAsset>,
): &Table<address, LinkedTable<u64, u64>> {
&pool.usr_open_orders
}
Expand Down Expand Up @@ -321,6 +321,7 @@ module deepbook::clob_v2 {
mint_account_cap(ctx)
}

#[lint_allow(self_transfer, share_owned)]
fun create_pool_<BaseAsset, QuoteAsset>(
taker_fee_rate: u64,
maker_rebate_rate: u64,
Expand Down Expand Up @@ -403,8 +404,8 @@ module deepbook::clob_v2 {
// Creates the capability to mark a pool owner.
let id = object::new(ctx);
let owner = object::uid_to_address(&id);
let pool_owner_cap = PoolOwnerCap {
id,
let pool_owner_cap = PoolOwnerCap {
id,
owner
};

Expand Down Expand Up @@ -453,6 +454,7 @@ module deepbook::clob_v2 {
)
}

#[lint_allow(self_transfer)]
/// Function for creating pool with customized taker fee rate and maker rebate rate.
/// The taker_fee_rate should be greater than or equal to the maker_rebate_rate, and both should have a scaling of 10^9.
/// Taker_fee_rate of 0.25% should be 2_500_000 for example
Expand All @@ -476,8 +478,8 @@ module deepbook::clob_v2 {
pool
}

/// A V2 function for creating customized pools for better PTB friendliness/compostability.
/// If a user wants to create a pool and then destroy/lock the pool_owner_cap one can do
/// A V2 function for creating customized pools for better PTB friendliness/compostability.
/// If a user wants to create a pool and then destroy/lock the pool_owner_cap one can do
/// so with this function.
public fun create_customized_pool_v2<BaseAsset, QuoteAsset>(
tick_size: u64,
Expand Down Expand Up @@ -1005,7 +1007,7 @@ module deepbook::clob_v2 {
&mut pool.quote_custodian,
maker_order.owner,
1
);
);
balance::join(&mut pool.quote_asset_trading_fees, rounded_down_quantity);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1053,6 +1053,7 @@ module sui_system::sui_system_state_inner {
validator_set::active_validator_addresses(validator_set)
}

#[lint_allow(self_transfer)]
/// Extract required Balance from vector of Coin<SUI>, transfer the remainder back to sender.
fun extract_coin_balance(coins: vector<Coin<SUI>>, amount: option::Option<u64>, ctx: &mut TxContext): Balance<SUI> {
let merged_coin = vector::pop_back(&mut coins);
Expand Down
5 changes: 4 additions & 1 deletion external-crates/move/crates/move-compiler/src/cfgir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,22 @@ pub mod visitor;
mod optimize;

use crate::{
expansion::ast::{AbilitySet, ModuleIdent},
expansion::ast::{AbilitySet, Attributes, ModuleIdent},
hlir::ast::{FunctionSignature, Label, SingleType, Var, Visibility},
parser::ast::StructName,
shared::{unique_map::UniqueMap, CompilationEnv, Name},
};
use cfg::*;
use move_ir_types::location::Loc;
use optimize::optimize;
use std::collections::BTreeSet;

pub struct CFGContext<'a> {
pub module: ModuleIdent,
pub member: MemberName,
pub struct_declared_abilities: &'a UniqueMap<ModuleIdent, UniqueMap<StructName, AbilitySet>>,
pub attributes: &'a Attributes,
pub entry: Option<Loc>,
pub visibility: Visibility,
pub signature: &'a FunctionSignature,
pub locals: &'a UniqueMap<Var, SingleType>,
Expand Down
27 changes: 23 additions & 4 deletions external-crates/move/crates/move-compiler/src/cfgir/translate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use crate::{
},
diag,
diagnostics::Diagnostics,
expansion::ast::{AbilitySet, ModuleIdent},
expansion::ast::{AbilitySet, Attributes, ModuleIdent},
hlir::ast::{self as H, BlockLabel, Label, Value, Value_, Var},
parser::ast::{ConstantName, FunctionName, StructName},
shared::{unique_map::UniqueMap, CompilationEnv},
Expand Down Expand Up @@ -396,6 +396,7 @@ fn constant(
module,
name,
loc,
&attributes,
signature.clone(),
locals,
block,
Expand Down Expand Up @@ -433,6 +434,7 @@ fn constant_(
module: ModuleIdent,
name: ConstantName,
full_loc: Loc,
attributes: &Attributes,
signature: H::BaseType,
locals: UniqueMap<Var, H::SingleType>,
body: H::Block,
Expand All @@ -459,6 +461,8 @@ fn constant_(
module,
member: cfgir::MemberName::Constant(name.0),
struct_declared_abilities: &context.struct_declared_abilities,
attributes,
entry: None,
visibility: H::Visibility::Internal,
signature: &fake_signature,
locals: &locals,
Expand Down Expand Up @@ -554,7 +558,16 @@ fn function(
body,
} = f;
context.env.add_warning_filter_scope(warning_filter.clone());
let body = function_body(context, module, name, visibility, &signature, body);
let body = function_body(
context,
module,
name,
&attributes,
entry,
visibility,
&signature,
body,
);
context.env.pop_warning_filter_scope();
G::Function {
warning_filter,
Expand All @@ -571,6 +584,8 @@ fn function_body(
context: &mut Context,
module: ModuleIdent,
name: FunctionName,
attributes: &Attributes,
entry: Option<Loc>,
visibility: H::Visibility,
signature: &H::FunctionSignature,
sp!(loc, tb_): H::FunctionBody,
Expand All @@ -595,6 +610,8 @@ fn function_body(
module,
member: cfgir::MemberName::Function(name.0),
struct_declared_abilities: &context.struct_declared_abilities,
attributes,
entry,
visibility,
signature,
locals: &locals,
Expand Down Expand Up @@ -902,9 +919,9 @@ fn visit_function(
let G::Function {
warning_filter,
index: _,
attributes: _,
attributes,
visibility,
entry: _,
entry,
signature,
body,
} = fdef;
Expand All @@ -923,6 +940,8 @@ fn visit_function(
module: mident,
member: cfgir::MemberName::Function(name.0),
struct_declared_abilities: &context.struct_declared_abilities,
attributes,
entry: *entry,
visibility: *visibility,
signature,
locals,
Expand Down
48 changes: 29 additions & 19 deletions external-crates/move/crates/move-compiler/src/expansion/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,31 +87,12 @@ pub enum Attribute_ {
}
pub type Attribute = Spanned<Attribute_>;

impl Attribute_ {
pub fn attribute_name(&self) -> &Name {
match self {
Attribute_::Name(nm)
| Attribute_::Assigned(nm, _)
| Attribute_::Parameterized(nm, _) => nm,
}
}
}

#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
pub enum AttributeName_ {
Unknown(Symbol),
Known(KnownAttribute),
}

impl AttributeName_ {
pub fn name(&self) -> Symbol {
match self {
Self::Unknown(s) => *s,
Self::Known(a) => a.name().into(),
}
}
}

pub type AttributeName = Spanned<AttributeName_>;

pub type Attributes = UniqueMap<AttributeName, Attribute>;
Expand Down Expand Up @@ -602,6 +583,35 @@ impl Hash for Address {
// impls
//**************************************************************************************************

impl Attribute_ {
pub fn attribute_name(&self) -> &Name {
match self {
Attribute_::Name(nm)
| Attribute_::Assigned(nm, _)
| Attribute_::Parameterized(nm, _) => nm,
}
}
}

impl AttributeName_ {
pub fn name(&self) -> Symbol {
match self {
Self::Unknown(s) => *s,
Self::Known(a) => a.name().into(),
}
}
}

impl Attributes {
pub fn is_test_or_test_only(&self) -> bool {
self.contains_key_(&AttributeName_::Known(KnownAttribute::Testing(
known_attributes::TestingAttribute::TestOnly,
))) || self.contains_key_(&AttributeName_::Known(KnownAttribute::Testing(
known_attributes::TestingAttribute::Test,
)))
}
}

impl UseFuns {
pub fn new() -> Self {
Self {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,13 @@ impl TypingVisitor for CoinFieldVisitor {
program: &mut T::Program_,
) {
for (_, _, mdef) in program.modules.iter() {
if mdef.attributes.is_test_or_test_only() {
continue;
}
env.add_warning_filter_scope(mdef.warning_filter.clone());
mdef.structs
.iter()
.filter(|(_, _, sdef)| !sdef.attributes.is_test_or_test_only())
.for_each(|(sloc, sname, sdef)| struct_def(env, *sname, sdef, sloc));
env.pop_warning_filter_scope();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ impl SimpleAbsIntConstructor for CustomStateChangeVerifier {
_env: &CompilationEnv,
_program: &'a Program,
context: &'a CFGContext<'a>,
_init_state: &mut <Self::AI<'a> as SimpleAbsInt>::State,
_init_state: &mut State,
) -> Option<Self::AI<'a>> {
let MemberName::Function(fn_name) = context.member else {
return None;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,25 @@ impl TypingVisitorConstructor for FreezeWrappedVisitor {
}

impl<'a> TypingVisitorContext for Context<'a> {
fn visit_module_custom(
&mut self,
_ident: E::ModuleIdent,
mdef: &mut T::ModuleDefinition,
) -> bool {
// skips if true
mdef.attributes.is_test_or_test_only()
}

fn visit_function_custom(
&mut self,
_module: E::ModuleIdent,
_function_name: P::FunctionName,
fdef: &mut T::Function,
) -> bool {
// skips if true
fdef.attributes.is_test_or_test_only()
}

fn visit_exp_custom(&mut self, exp: &mut T::Exp) -> bool {
use T::UnannotatedExp_ as E;
if let E::ModuleCall(fun) = &exp.exp.value {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,28 @@ impl SimpleAbsIntConstructor for SelfTransferVerifier {

fn new<'a>(
_env: &CompilationEnv,
_program: &'a Program,
program: &'a Program,
context: &'a CFGContext<'a>,
_init_state: &mut <Self::AI<'a> as SimpleAbsInt>::State,
) -> Option<Self::AI<'a>> {
let MemberName::Function(name) = context.member else {
return None;
};

if context.entry.is_some()
|| context.attributes.is_test_or_test_only()
|| program
.modules
.get(&context.module)
.unwrap()
.attributes
.is_test_or_test_only()
{
// Cannot return objects from entry
// No need to check test functions
return None;
}

if name.value.as_str() == "init" {
// do not lint module initializers, since they do not have the option of returning
// values, and the entire purpose of this linter is to encourage folks to return
Expand Down
Loading

0 comments on commit 69104e3

Please sign in to comment.