Skip to content

Commit 3a5e20c

Browse files
mohammadfawazMohammad Fawaz
andauthored
fix(typeck): reject non-path receivers for Vector/Mapping storage ops (#29432)
`Vector::{len,get,set,push,pop,swap_remove,clear}` and `Mapping::{get,get_or_use,contains,set,remove}` assumed an `Expression::Path` receiver downstream: storage_lowering for vectors, codegen for mappings. Type-checking only verified the receiver's type. - Read ops (`len`/`get`/`get_or_use`/`contains`) had no path check at all, so a ternary of two storage vectors ICEd in storage_lowering (#29428) and a ternary of two mappings produced invalid bytecode that failed late at disassembly. - Write ops already had an `is_local_path` check, but emitted the "external vectors/mappings" diagnostic for a local ternary receiver, which is misleading. Add an `Expression::Path` pre-check to all eleven arms and emit a new `ETYC0372191` for non-path receivers. The existing "external vectors/mappings" diagnostic now only fires when the receiver is an actual external path, which is what its wording describes. Fixes #29428. Co-authored-by: Mohammad Fawaz <mohammadfawaz@gmail.com>
1 parent 3a6eba3 commit 3a5e20c

8 files changed

Lines changed: 285 additions & 4 deletions

File tree

crates/passes/src/errors/type_checker.rs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1063,6 +1063,21 @@ pub(crate) fn multi_identifier_definition_requires_tuple(type_: impl Display, sp
10631063
.with_help("Use a tuple expression, e.g. `let (a, b) = (x, y);`.")
10641064
}
10651065

1066+
pub(crate) fn storage_op_requires_path_receiver(
1067+
module: impl Display,
1068+
operation: impl Display,
1069+
kind: impl Display,
1070+
span: Span,
1071+
) -> Formatted {
1072+
Formatted::error(
1073+
CODE_PREFIX,
1074+
CODE_MASK + 191,
1075+
format!("the receiver of `{module}::{operation}` must be a {kind}"),
1076+
span,
1077+
)
1078+
.with_help(format!("Call `{operation}` directly on a declared {kind}, not on a temporary expression."))
1079+
}
1080+
10661081
// TypeCheckerWarning builder functions
10671082

10681083
pub(crate) fn caller_as_record_owner(record_name: impl Display, span: Span) -> Formatted {

crates/passes/src/type_checking/visitor.rs

Lines changed: 69 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,23 @@ impl TypeCheckingVisitor<'_> {
8484
self.state.handler.emit_err(err);
8585
}
8686

87+
/// Returns `true` if `expr` is a path receiver; otherwise emits a diagnostic and returns `false`.
88+
/// Storage `Vector::*` and `Mapping::*` operations require a path receiver because downstream
89+
/// passes look up the backing mappings by name (storage_lowering for vectors, codegen for
90+
/// mappings).
91+
fn check_path_receiver(&self, module: &str, operation: &str, kind: &str, expr: &Expression) -> bool {
92+
if matches!(expr, Expression::Path(_)) {
93+
return true;
94+
}
95+
self.emit_err(crate::errors::type_checker::storage_op_requires_path_receiver(
96+
module,
97+
operation,
98+
kind,
99+
expr.span(),
100+
));
101+
false
102+
}
103+
87104
/// Emits an error if the two given types are not equal.
88105
pub fn check_eq_types(&self, t1: &Option<Type>, t2: &Option<Type>, span: Span) {
89106
match (t1, t2) {
@@ -966,6 +983,10 @@ impl TypeCheckingVisitor<'_> {
966983
// Check that the operation is invoked in a `finalize` or `async` block.
967984
self.check_access_allowed("Mapping::get", true, function_span);
968985

986+
if !self.check_path_receiver("Mapping", "get", "mapping", map_expr) {
987+
return Type::Err;
988+
}
989+
969990
*value.clone()
970991
}
971992
Intrinsic::MappingSet => {
@@ -979,6 +1000,10 @@ impl TypeCheckingVisitor<'_> {
9791000
// Check that the operation is invoked in a `finalize` or `async` block.
9801001
self.check_access_allowed("Mapping::set", true, function_span);
9811002

1003+
if !self.check_path_receiver("Mapping", "set", "mapping", map_expr) {
1004+
return Type::Err;
1005+
}
1006+
9821007
// Argument 0 must be a local path (cannot modify external mappings).
9831008
if !is_local_path(map_expr) {
9841009
self.state.handler.emit_err(crate::errors::type_checker::cannot_modify_external_container(
@@ -1005,6 +1030,10 @@ impl TypeCheckingVisitor<'_> {
10051030
return Type::Err;
10061031
};
10071032

1033+
if !self.check_path_receiver("Mapping", "get_or_use", "mapping", map_expr) {
1034+
return Type::Err;
1035+
}
1036+
10081037
value.deref().clone()
10091038
}
10101039
Intrinsic::MappingRemove => {
@@ -1021,6 +1050,10 @@ impl TypeCheckingVisitor<'_> {
10211050
return Type::Err;
10221051
};
10231052

1053+
if !self.check_path_receiver("Mapping", "remove", "mapping", map_expr) {
1054+
return Type::Err;
1055+
}
1056+
10241057
// Argument 0 must be a local path (cannot modify external mappings).
10251058
if !is_local_path(map_expr) {
10261059
self.state.handler.emit_err(crate::errors::type_checker::cannot_modify_external_container(
@@ -1047,6 +1080,10 @@ impl TypeCheckingVisitor<'_> {
10471080
return Type::Err;
10481081
};
10491082

1083+
if !self.check_path_receiver("Mapping", "contains", "mapping", map_expr) {
1084+
return Type::Err;
1085+
}
1086+
10501087
Type::Boolean
10511088
}
10521089
Intrinsic::OptionalUnwrap => {
@@ -1082,6 +1119,10 @@ impl TypeCheckingVisitor<'_> {
10821119
// Check that the operation is invoked in a `finalize` or `async` block.
10831120
self.check_access_allowed("Vector::get", true, function_span);
10841121

1122+
if !self.check_path_receiver("Vector", "get", "storage vector", vec_expr) {
1123+
return Type::Err;
1124+
}
1125+
10851126
Type::Optional(OptionalType { inner: Box::new(*element_type.clone()) })
10861127
}
10871128
Intrinsic::VectorSet => {
@@ -1095,6 +1136,10 @@ impl TypeCheckingVisitor<'_> {
10951136
// Check that the operation is invoked in a `finalize` or `async` block.
10961137
self.check_access_allowed("Vector::set", true, function_span);
10971138

1139+
if !self.check_path_receiver("Vector", "set", "storage vector", vec_expr) {
1140+
return Type::Err;
1141+
}
1142+
10981143
// Argument 0 must be a local path (cannot modify external vectors).
10991144
if !is_local_path(vec_expr) {
11001145
self.state.handler.emit_err(crate::errors::type_checker::cannot_modify_external_container(
@@ -1123,6 +1168,10 @@ impl TypeCheckingVisitor<'_> {
11231168
// Check that the operation is invoked in a `finalize` or `async` block.
11241169
self.check_access_allowed("Vector::push", true, function_span);
11251170

1171+
if !self.check_path_receiver("Vector", "push", "storage vector", vec_expr) {
1172+
return Type::Err;
1173+
}
1174+
11261175
// Argument 0 must be a local path (cannot modify external vectors).
11271176
if !is_local_path(vec_expr) {
11281177
self.state.handler.emit_err(crate::errors::type_checker::cannot_modify_external_container(
@@ -1141,12 +1190,16 @@ impl TypeCheckingVisitor<'_> {
11411190
// Check that the operation is invoked in a `finalize` or `async` block.
11421191
self.check_access_allowed("Vector::len", true, function_span);
11431192

1144-
if vec_ty.is_vector() {
1145-
Type::Integer(IntegerType::U32)
1146-
} else {
1193+
if !vec_ty.is_vector() {
11471194
self.assert_vector_type(vec_ty, vec_expr.span());
1148-
Type::Err
1195+
return Type::Err;
1196+
}
1197+
1198+
if !self.check_path_receiver("Vector", "len", "storage vector", vec_expr) {
1199+
return Type::Err;
11491200
}
1201+
1202+
Type::Integer(IntegerType::U32)
11501203
}
11511204
Intrinsic::VectorPop => {
11521205
let (vec_ty, vec_expr) = &arguments[0];
@@ -1159,6 +1212,10 @@ impl TypeCheckingVisitor<'_> {
11591212
return Type::Err;
11601213
};
11611214

1215+
if !self.check_path_receiver("Vector", "pop", "storage vector", vec_expr) {
1216+
return Type::Err;
1217+
}
1218+
11621219
// Argument 0 must be a local path (cannot modify external vectors).
11631220
if !is_local_path(vec_expr) {
11641221
self.state.handler.emit_err(crate::errors::type_checker::cannot_modify_external_container(
@@ -1182,6 +1239,10 @@ impl TypeCheckingVisitor<'_> {
11821239
return Type::Err;
11831240
};
11841241

1242+
if !self.check_path_receiver("Vector", "swap_remove", "storage vector", vec_expr) {
1243+
return Type::Err;
1244+
}
1245+
11851246
// Argument 0 must be a local path (cannot modify external vectors).
11861247
if !is_local_path(vec_expr) {
11871248
self.state.handler.emit_err(crate::errors::type_checker::cannot_modify_external_container(
@@ -1202,6 +1263,10 @@ impl TypeCheckingVisitor<'_> {
12021263
return Type::Err;
12031264
}
12041265

1266+
if !self.check_path_receiver("Vector", "clear", "storage vector", vec_expr) {
1267+
return Type::Err;
1268+
}
1269+
12051270
// Argument 0 must be a local path (cannot modify external vectors).
12061271
if !is_local_path(vec_expr) {
12071272
self.state.handler.emit_err(crate::errors::type_checker::cannot_modify_external_container(
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
[ETYC0372191] Error: the receiver of `Mapping::get` must be a mapping
2+
╭─[ compiler-test:12:27 ]
3+
4+
12 │ let v: u32 = (c ? m1 : m2).get(0u32); // ❌ non-path receiver
5+
6+
│ Help: Call `get` directly on a declared mapping, not on a temporary expression.
7+
────╯
8+
[ETYC0372191] Error: the receiver of `Mapping::contains` must be a mapping
9+
╭─[ compiler-test:19:28 ]
10+
11+
19 │ let b: bool = (c ? m1 : m2).contains(0u32); // ❌ non-path receiver
12+
13+
│ Help: Call `contains` directly on a declared mapping, not on a temporary expression.
14+
────╯
15+
[ETYC0372191] Error: the receiver of `Mapping::get_or_use` must be a mapping
16+
╭─[ compiler-test:26:27 ]
17+
18+
26 │ let v: u32 = (c ? m1 : m2).get_or_use(0u32, 0u32); // ❌ non-path receiver
19+
20+
│ Help: Call `get_or_use` directly on a declared mapping, not on a temporary expression.
21+
────╯
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
[ETYC0372191] Error: the receiver of `Vector::len` must be a storage vector
2+
╭─[ compiler-test:15:27 ]
3+
4+
15 │ let n: u32 = (c ? h1 : h2).len(); // ❌ non-path receiver
5+
6+
│ Help: Call `len` directly on a declared storage vector, not on a temporary expression.
7+
────╯
8+
[ETYC0372191] Error: the receiver of `Vector::get` must be a storage vector
9+
╭─[ compiler-test:22:28 ]
10+
11+
22 │ let v: u64? = (c ? h1 : h2).get(0u32); // ❌ non-path receiver
12+
13+
│ Help: Call `get` directly on a declared storage vector, not on a temporary expression.
14+
────╯
15+
[ETYC0372191] Error: the receiver of `Vector::len` must be a storage vector
16+
╭─[ compiler-test:29:39 ]
17+
18+
29 │ let n: u32 = Vector::len((c ? h1 : h2)); // ❌ non-path receiver
19+
20+
│ Help: Call `len` directly on a declared storage vector, not on a temporary expression.
21+
────╯
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
[ETYC0372191] Error: the receiver of `Vector::set` must be a storage vector
2+
╭─[ compiler-test:15:25 ]
3+
4+
15 │ return final { (c ? h1 : h2).set(0u32, 1u64); }; // ❌ non-path receiver
5+
6+
│ Help: Call `set` directly on a declared storage vector, not on a temporary expression.
7+
────╯
8+
[ETYC0372191] Error: the receiver of `Vector::push` must be a storage vector
9+
╭─[ compiler-test:19:25 ]
10+
11+
19 │ return final { (c ? h1 : h2).push(1u64); }; // ❌ non-path receiver
12+
13+
│ Help: Call `push` directly on a declared storage vector, not on a temporary expression.
14+
────╯
15+
[ETYC0372191] Error: the receiver of `Vector::pop` must be a storage vector
16+
╭─[ compiler-test:24:28 ]
17+
18+
24 │ let x: u64? = (c ? h1 : h2).pop(); // ❌ non-path receiver
19+
20+
│ Help: Call `pop` directly on a declared storage vector, not on a temporary expression.
21+
────╯
22+
[ETYC0372191] Error: the receiver of `Vector::swap_remove` must be a storage vector
23+
╭─[ compiler-test:31:27 ]
24+
25+
31 │ let x: u64 = (c ? h1 : h2).swap_remove(0u32); // ❌ non-path receiver
26+
27+
│ Help: Call `swap_remove` directly on a declared storage vector, not on a temporary expression.
28+
────╯
29+
[ETYC0372191] Error: the receiver of `Vector::clear` must be a storage vector
30+
╭─[ compiler-test:37:25 ]
31+
32+
37 │ return final { (c ? h1 : h2).clear(); }; // ❌ non-path receiver
33+
34+
│ Help: Call `clear` directly on a declared storage vector, not on a temporary expression.
35+
────╯
36+
[ETYC0372191] Error: the receiver of `Mapping::set` must be a mapping
37+
╭─[ compiler-test:41:25 ]
38+
39+
41 │ return final { (c ? m1 : m2).set(0u32, 1u32); }; // ❌ non-path receiver
40+
41+
│ Help: Call `set` directly on a declared mapping, not on a temporary expression.
42+
────╯
43+
[ETYC0372191] Error: the receiver of `Mapping::remove` must be a mapping
44+
╭─[ compiler-test:45:25 ]
45+
46+
45 │ return final { (c ? m1 : m2).remove(0u32); }; // ❌ non-path receiver
47+
48+
│ Help: Call `remove` directly on a declared mapping, not on a temporary expression.
49+
────╯
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
// Companion to #29428: ensure non-path receivers for `Mapping::get` /
2+
// `Mapping::contains` / `Mapping::get_or_use` are also caught cleanly.
3+
program bugpoc_map.aleo {
4+
@noupgrade
5+
constructor() {}
6+
7+
mapping m1: u32 => u32;
8+
mapping m2: u32 => u32;
9+
10+
fn method_get(c: bool) -> Final {
11+
return final {
12+
let v: u32 = (c ? m1 : m2).get(0u32); // ❌ non-path receiver
13+
assert(v >= 0u32);
14+
};
15+
}
16+
17+
fn method_contains(c: bool) -> Final {
18+
return final {
19+
let b: bool = (c ? m1 : m2).contains(0u32); // ❌ non-path receiver
20+
assert(b == b);
21+
};
22+
}
23+
24+
fn method_get_or_use(c: bool) -> Final {
25+
return final {
26+
let v: u32 = (c ? m1 : m2).get_or_use(0u32, 0u32); // ❌ non-path receiver
27+
assert(v >= 0u32);
28+
};
29+
}
30+
}
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
// Regression test for https://github.com/ProvableHQ/leo/issues/29428
2+
// `Vector::len` / `Vector::get` on a non-path receiver (e.g. a ternary over
3+
// storage vectors) used to ICE in storage_lowering. Now caught cleanly by
4+
// type-checking.
5+
program bugpoc.aleo {
6+
@noupgrade
7+
constructor() {}
8+
9+
storage h1: [u64];
10+
storage h2: [u64];
11+
mapping seen: u32 => u32;
12+
13+
fn method_len(c: bool) -> Final {
14+
return final {
15+
let n: u32 = (c ? h1 : h2).len(); // ❌ non-path receiver
16+
Mapping::set(seen, 0u32, n);
17+
};
18+
}
19+
20+
fn method_get(c: bool) -> Final {
21+
return final {
22+
let v: u64? = (c ? h1 : h2).get(0u32); // ❌ non-path receiver
23+
Mapping::set(seen, 0u32, v.unwrap_or(0u64) as u32);
24+
};
25+
}
26+
27+
fn explicit_len(c: bool) -> Final {
28+
return final {
29+
let n: u32 = Vector::len((c ? h1 : h2)); // ❌ non-path receiver
30+
Mapping::set(seen, 0u32, n);
31+
};
32+
}
33+
}
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
// Companion to #29428: mutating storage Vector/Mapping ops on a non-path
2+
// receiver (e.g. a ternary over local storage) used to be reported as
3+
// "on external vectors/mappings", which is misleading when the receiver
4+
// is local. Now reported with the dedicated non-path diagnostic.
5+
program bugpoc_write.aleo {
6+
@noupgrade
7+
constructor() {}
8+
9+
storage h1: [u64];
10+
storage h2: [u64];
11+
mapping m1: u32 => u32;
12+
mapping m2: u32 => u32;
13+
14+
fn vec_set(c: bool) -> Final {
15+
return final { (c ? h1 : h2).set(0u32, 1u64); }; // ❌ non-path receiver
16+
}
17+
18+
fn vec_push(c: bool) -> Final {
19+
return final { (c ? h1 : h2).push(1u64); }; // ❌ non-path receiver
20+
}
21+
22+
fn vec_pop(c: bool) -> Final {
23+
return final {
24+
let x: u64? = (c ? h1 : h2).pop(); // ❌ non-path receiver
25+
assert(x != none);
26+
};
27+
}
28+
29+
fn vec_swap_remove(c: bool) -> Final {
30+
return final {
31+
let x: u64 = (c ? h1 : h2).swap_remove(0u32); // ❌ non-path receiver
32+
assert(x >= 0u64);
33+
};
34+
}
35+
36+
fn vec_clear(c: bool) -> Final {
37+
return final { (c ? h1 : h2).clear(); }; // ❌ non-path receiver
38+
}
39+
40+
fn map_set(c: bool) -> Final {
41+
return final { (c ? m1 : m2).set(0u32, 1u32); }; // ❌ non-path receiver
42+
}
43+
44+
fn map_remove(c: bool) -> Final {
45+
return final { (c ? m1 : m2).remove(0u32); }; // ❌ non-path receiver
46+
}
47+
}

0 commit comments

Comments
 (0)