Skip to content

Commit

Permalink
Allow constants as standalone value paths (MystenLabs#16831)
Browse files Browse the repository at this point in the history
## Description 

This allows constants to be used in some positions that were previously
errors.

## Test Plan 

New and updated tests

---
If your changes are not user-facing and do not break anything, you can
skip the following section. Otherwise, please briefly describe what has
changed under the Release Notes section.

### Type of Change (Check all that apply)

- [ ] protocol change
- [ ] 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
  • Loading branch information
cgswords authored Mar 26, 2024
1 parent 08c643b commit fc7ef79
Show file tree
Hide file tree
Showing 7 changed files with 173 additions and 64 deletions.
70 changes: 54 additions & 16 deletions external-crates/move/crates/move-compiler/src/typing/translate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2295,6 +2295,10 @@ struct ExpDotted {
base: T::Exp,
base_type: N::Type,
accessors: Vec<ExpDottedAccess>,
// This should only be used in the functions grouped here, nowhere else. This is for tracking
// if a constant appears plainly in a `use`/`copy` position, and suppresses constant usage
// warning if so.
warn_on_constant: bool,
}

impl ExpDotted {
Expand Down Expand Up @@ -2341,6 +2345,7 @@ fn process_exp_dotted(
base_kind,
base_type,
accessors,
warn_on_constant: true,
}
}
N::ExpDotted_::Dot(ndot, field) => {
Expand Down Expand Up @@ -2543,7 +2548,6 @@ fn resolve_exp_dotted(
copy_exp
}
DottedUsage::Use => {
warn_on_constant_borrow(context, edotted.base.exp.loc, &edotted.base);
if edotted.accessors.is_empty() {
Box::new(edotted.base)
} else {
Expand Down Expand Up @@ -2612,10 +2616,22 @@ fn borrow_exp_dotted(context: &mut Context, mut_: bool, ed: ExpDotted) -> Box<T:
base_type,
base_kind,
accessors,
mut warn_on_constant,
} = ed;

// If we have accessors, we are definitely going to actually borrow and that means we'll copy
// a base constant, so we should warn if we do.
warn_on_constant = warn_on_constant || !accessors.is_empty();

let mut exp = match base_kind {
BaseRefKind::Owned => exp_to_borrow(context, loc, mut_, Box::new(base), base_type),
BaseRefKind::Owned => exp_to_borrow_(
context,
loc,
mut_,
Box::new(base),
base_type,
warn_on_constant,
),
BaseRefKind::ImmRef | BaseRefKind::MutRef => Box::new(base),
};

Expand Down Expand Up @@ -2711,21 +2727,18 @@ fn exp_dotted_to_owned(context: &mut Context, usage: DottedUsage, ed: ExpDotted)
)));
return make_error_exp(context, ed.loc);
};
let borrow_exp = borrow_exp_dotted(context, false, ed);
let case = match usage {
DottedUsage::Move(_) => {
context.env.add_diag(ice!((
borrow_exp.exp.loc,
"Invalid dotted usage 'move' in to_owned"
)));
return make_error_exp(context, borrow_exp.exp.loc);
context
.env
.add_diag(ice!((ed.loc, "Invalid dotted usage 'move' in to_owned")));
return make_error_exp(context, ed.loc);
}
DottedUsage::Borrow(_) => {
context.env.add_diag(ice!((
borrow_exp.exp.loc,
"Invalid dotted usage 'borrow' in to_owned"
)));
return make_error_exp(context, borrow_exp.exp.loc);
context
.env
.add_diag(ice!((ed.loc, "Invalid dotted usage 'borrow' in to_owned")));
return make_error_exp(context, ed.loc);
}
DottedUsage::Use => "implicit copy",
DottedUsage::Copy(loc) => {
Expand All @@ -2735,6 +2748,13 @@ fn exp_dotted_to_owned(context: &mut Context, usage: DottedUsage, ed: ExpDotted)
"'copy'"
}
};
// If we are going to an owned value and we have a constant with no accessors, we're fine to
// not warn about its usage.
let mut edotted = ed;
if edotted.accessors.is_empty() {
edotted.warn_on_constant = false;
}
let borrow_exp = borrow_exp_dotted(context, false, edotted);
let eloc = borrow_exp.exp.loc;
context.add_ability_constraint(
eloc,
Expand Down Expand Up @@ -2763,12 +2783,28 @@ fn exp_to_borrow(
mut_: bool,
eb: Box<T::Exp>,
base_type: Type,
) -> Box<T::Exp> {
exp_to_borrow_(
context, loc, mut_, eb, base_type, /* warn_on_constant */ true,
)
}

fn exp_to_borrow_(
context: &mut Context,
loc: Loc,
mut_: bool,
eb: Box<T::Exp>,
base_type: Type,
warn_on_constant: bool,
) -> Box<T::Exp> {
use Type_::*;
use T::UnannotatedExp_ as TE;
warn_on_constant_borrow(context, eb.exp.loc, &eb);
if warn_on_constant {
warn_on_constant_borrow(context, eb.exp.loc, &eb)
};
let eb_ty = eb.ty;
let sp!(ebloc, eb_) = eb.exp;
let ref_ty = Ref(mut_, Box::new(base_type));
let e_ = match eb_ {
TE::Use(v) => TE::BorrowLocal(mut_, v),
eb_ => {
Expand All @@ -2781,7 +2817,7 @@ fn exp_to_borrow(
TE::TempBorrow(mut_, Box::new(T::exp(eb_ty, sp(ebloc, eb_))))
}
};
let ty = sp(loc, Ref(mut_, Box::new(base_type)));
let ty = sp(loc, ref_ty);
Box::new(T::exp(ty, sp(loc, e_)))
}

Expand Down Expand Up @@ -2820,13 +2856,15 @@ fn method_call(
fn method_call_resolve(
context: &mut Context,
loc: Loc,
mut edotted: ExpDotted,
edotted: ExpDotted,
method: Name,
ty_args_opt: Option<Vec<Type>>,
) -> Option<(ModuleIdent, FunctionName, ResolvedFunctionType, T::Exp)> {
use TypeName_ as TN;
use Type_ as Ty;

let mut edotted = edotted;

edotted.loc = loc;
let edotted_ty = core::unfold_type(&context.subst, edotted.last_type());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,144 +8,160 @@ warning[W09001]: unused alias
= This warning can be suppressed with '#[allow(unused_use)]' applied to the 'module' or module member ('const', 'fun', or 'struct')

error[E04012]: invalid type for constant
┌─ tests/move_2024/typing/all_index_paths.move:44:16
┌─ tests/move_2024/typing/all_index_paths.move:46:16
44 │ const VEC: vector<X> = vector[];
46 │ const VEC: vector<X> = vector[];
│ ^^^^^^^^^
│ │ │
│ │ Found: 'a::m::X'. But expected one of: 'u8', 'u16', 'u32', 'u64', 'u128', 'u256', 'bool', 'address', 'vector<_>'
│ Unpermitted constant type

error[E01013]: invalid 'move' or 'copy'
┌─ tests/move_2024/typing/all_index_paths.move:61:9
┌─ tests/move_2024/typing/all_index_paths.move:63:9
61 │ copy t.u.vs[n].w.xs[m+1].deref();
63 │ copy t.u.vs[n].w.xs[m+1].deref();
│ ^^^^ --------------------------- Expected a name or path access, e.g. 'x' or 'e.f'
│ │
│ Invalid 'copy' of expression

error[E01013]: invalid 'move' or 'copy'
┌─ tests/move_2024/typing/all_index_paths.move:62:9
┌─ tests/move_2024/typing/all_index_paths.move:64:9
62 │ copy t.u.vs[n].w.xs[m+1].deref().id();
64 │ copy t.u.vs[n].w.xs[m+1].deref().id();
│ ^^^^ -------------------------------- Expected a name or path access, e.g. 'x' or 'e.f'
│ │
│ Invalid 'copy' of expression

warning[W04028]: implicit copy of a constant
┌─ tests/move_2024/typing/all_index_paths.move:63:14
┌─ tests/move_2024/typing/all_index_paths.move:65:14
63 │ copy VEC[n+1];
65 │ copy VEC[n+1];
│ ^^^ This access will make a new copy of the constant. Consider binding the value to a variable first to make this copy explicit
= This warning can be suppressed with '#[allow(implicit_const_copy)]' applied to the 'module' or module member ('const', 'fun', or 'struct')

error[E01013]: invalid 'move' or 'copy'
┌─ tests/move_2024/typing/all_index_paths.move:64:9
┌─ tests/move_2024/typing/all_index_paths.move:66:9
64 │ copy VEC[n+1].id_u64();
66 │ copy VEC[n+1].id_u64();
│ ^^^^ ----------------- Expected a name or path access, e.g. 'x' or 'e.f'
│ │
│ Invalid 'copy' of expression

error[E01013]: invalid 'move' or 'copy'
┌─ tests/move_2024/typing/all_index_paths.move:67:9
67 │ copy VEC.id();
│ ^^^^ -------- Expected a name or path access, e.g. 'x' or 'e.f'
│ │
│ Invalid 'copy' of expression

error[E04027]: invalid 'move' usage
┌─ tests/move_2024/typing/all_index_paths.move:81:9
┌─ tests/move_2024/typing/all_index_paths.move:84:9
81 │ move t2.u;
84 │ move t2.u;
│ ^^^^ Invalid 'move'. 'move' works only with variables, e.g. 'move x'. 'move' on a path access is not supported

error[E04027]: invalid 'move' usage
┌─ tests/move_2024/typing/all_index_paths.move:82:10
┌─ tests/move_2024/typing/all_index_paths.move:85:10
82 │ (move t2.u).vs[2];
85 │ (move t2.u).vs[2];
│ ^^^^ Invalid 'move'. 'move' works only with variables, e.g. 'move x'. 'move' on a path access is not supported

error[E04027]: invalid 'move' usage
┌─ tests/move_2024/typing/all_index_paths.move:83:10
┌─ tests/move_2024/typing/all_index_paths.move:86:10
83 │ (move t2.u).vs[2].w;
86 │ (move t2.u).vs[2].w;
│ ^^^^ Invalid 'move'. 'move' works only with variables, e.g. 'move x'. 'move' on a path access is not supported

error[E04027]: invalid 'move' usage
┌─ tests/move_2024/typing/all_index_paths.move:84:10
┌─ tests/move_2024/typing/all_index_paths.move:87:10
84 │ (move t2.u).vs[2].w.xs[m+1];
87 │ (move t2.u).vs[2].w.xs[m+1];
│ ^^^^ Invalid 'move'. 'move' works only with variables, e.g. 'move x'. 'move' on a path access is not supported

error[E04027]: invalid 'move' usage
┌─ tests/move_2024/typing/all_index_paths.move:85:10
┌─ tests/move_2024/typing/all_index_paths.move:88:10
85 │ (move t2.u).vs[2].w.xs[m+1].y;
88 │ (move t2.u).vs[2].w.xs[m+1].y;
│ ^^^^ Invalid 'move'. 'move' works only with variables, e.g. 'move x'. 'move' on a path access is not supported

error[E04027]: invalid 'move' usage
┌─ tests/move_2024/typing/all_index_paths.move:86:10
┌─ tests/move_2024/typing/all_index_paths.move:89:10
86 │ (move t2.u).vs[2].w.xs[m+1].y.z;
89 │ (move t2.u).vs[2].w.xs[m+1].y.z;
│ ^^^^ Invalid 'move'. 'move' works only with variables, e.g. 'move x'. 'move' on a path access is not supported

error[E01013]: invalid 'move' or 'copy'
┌─ tests/move_2024/typing/all_index_paths.move:90:9
90 │ move VEC.id();
│ ^^^^ -------- Expected a name or path access, e.g. 'x' or 'e.f'
│ │
│ Invalid 'move' of expression

error[E04004]: expected a single non-reference type
┌─ tests/move_2024/typing/all_index_paths.move:100:9
┌─ tests/move_2024/typing/all_index_paths.move:104:9
16 │ fun ref_unused(_x: &X) { }
18 │ fun ref_unused(_x: &X) { }
│ ---------- Expected a single non-reference type, but found: '()'
·
100 │ &t2.u.vs[2].w.xs[m+1].ref_unused(); // invalid -- trying to borrow `()`
104 │ &t2.u.vs[2].w.xs[m+1].ref_unused(); // invalid -- trying to borrow `()`
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Invalid borrow

warning[W04028]: implicit copy of a constant
┌─ tests/move_2024/typing/all_index_paths.move:104:10
┌─ tests/move_2024/typing/all_index_paths.move:108:10
104 │ &VEC[n+1];
108 │ &VEC[n+1];
│ ^^^ This access will make a new copy of the constant. Consider binding the value to a variable first to make this copy explicit
= This warning can be suppressed with '#[allow(implicit_const_copy)]' applied to the 'module' or module member ('const', 'fun', or 'struct')

warning[W04028]: implicit copy of a constant
┌─ tests/move_2024/typing/all_index_paths.move:105:10
┌─ tests/move_2024/typing/all_index_paths.move:109:10
105 │ &VEC[n+1].id();
109 │ &VEC[n+1].id();
│ ^^^ This access will make a new copy of the constant. Consider binding the value to a variable first to make this copy explicit
= This warning can be suppressed with '#[allow(implicit_const_copy)]' applied to the 'module' or module member ('const', 'fun', or 'struct')

error[E04004]: expected a single non-reference type
┌─ tests/move_2024/typing/all_index_paths.move:119:9
┌─ tests/move_2024/typing/all_index_paths.move:124:9
16 │ fun ref_unused(_x: &X) { }
18 │ fun ref_unused(_x: &X) { }
│ ---------- Expected a single non-reference type, but found: '()'
·
119 │ &mut t2.u.vs[2].w.xs[m+1].ref_unused(); // invalid -- trying to borrow `()`
124 │ &mut t2.u.vs[2].w.xs[m+1].ref_unused(); // invalid -- trying to borrow `()`
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Invalid borrow

warning[W04028]: implicit copy of a constant
┌─ tests/move_2024/typing/all_index_paths.move:123:14
┌─ tests/move_2024/typing/all_index_paths.move:128:14
123 │ &mut VEC[n+1];
128 │ &mut VEC[n+1];
│ ^^^ This access will make a new copy of the constant. Consider binding the value to a variable first to make this copy explicit
= This warning can be suppressed with '#[allow(implicit_const_copy)]' applied to the 'module' or module member ('const', 'fun', or 'struct')

warning[W04028]: implicit copy of a constant
┌─ tests/move_2024/typing/all_index_paths.move:124:14
┌─ tests/move_2024/typing/all_index_paths.move:129:14
124 │ &mut VEC[n+1].id();
129 │ &mut VEC[n+1].id();
│ ^^^ This access will make a new copy of the constant. Consider binding the value to a variable first to make this copy explicit
= This warning can be suppressed with '#[allow(implicit_const_copy)]' applied to the 'module' or module member ('const', 'fun', or 'struct')

warning[W04028]: implicit copy of a constant
┌─ tests/move_2024/typing/all_index_paths.move:139:9
┌─ tests/move_2024/typing/all_index_paths.move:145:9
139 │ VEC[n+1];
145 │ VEC[n+1];
│ ^^^ This access will make a new copy of the constant. Consider binding the value to a variable first to make this copy explicit
= This warning can be suppressed with '#[allow(implicit_const_copy)]' applied to the 'module' or module member ('const', 'fun', or 'struct')

warning[W04028]: implicit copy of a constant
┌─ tests/move_2024/typing/all_index_paths.move:140:9
┌─ tests/move_2024/typing/all_index_paths.move:146:9
140 │ VEC[n+1].id();
146 │ VEC[n+1].id();
│ ^^^ This access will make a new copy of the constant. Consider binding the value to a variable first to make this copy explicit
= This warning can be suppressed with '#[allow(implicit_const_copy)]' applied to the 'module' or module member ('const', 'fun', or 'struct')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ module a::m {

fun id_w(w: W): W { w }
fun id(x: X): X { x }
fun vec_id<T>(v: vector<T>): vector<T> { v }
use fun vec_id as vector.id;

fun ref_unused(_x: &X) { }

Expand Down Expand Up @@ -62,6 +64,7 @@ module a::m {
copy t.u.vs[n].w.xs[m+1].deref().id();
copy VEC[n+1];
copy VEC[n+1].id_u64();
copy VEC.id();
}

fun all_index_move(t: T, t2: T, n: u64, m: u64) {
Expand All @@ -84,6 +87,7 @@ module a::m {
(move t2.u).vs[2].w.xs[m+1];
(move t2.u).vs[2].w.xs[m+1].y;
(move t2.u).vs[2].w.xs[m+1].y.z;
move VEC.id();
}

fun all_index_borrow(t: T, t2: T, n: u64, m: u64) {
Expand All @@ -103,6 +107,7 @@ module a::m {
&(&t2.u.vs[2].w.xs[m+1]).deref();
&VEC[n+1];
&VEC[n+1].id();
&VEC.id();
}

fun all_index_borrow_mut(mut t: T, mut t2: T, n: u64, m: u64) {
Expand All @@ -122,6 +127,7 @@ module a::m {
(&mut t2.u.vs[2].w).xs[m+1].deref();
&mut VEC[n+1];
&mut VEC[n+1].id();
&mut VEC.id();
}

fun all_index_use(t: T, t2: T, n: u64, m: u64) {
Expand All @@ -138,5 +144,6 @@ module a::m {
t2.u.vs[2].w.xs[m+1].id();
VEC[n+1];
VEC[n+1].id();
VEC.id();
}
}
Loading

0 comments on commit fc7ef79

Please sign in to comment.