From 755938ca5cc0cef7cdf3f1d6b4c1a33c5b67ffc6 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Mon, 25 Sep 2023 16:18:58 +0700 Subject: [PATCH 01/17] feat(lint+test): add skeleton of storage_never_freed (#1431) --- linting/Cargo.toml | 6 ++ linting/src/lib.rs | 3 +- linting/src/storage_never_freed.rs | 77 ++++++++++++++++++++++++++ linting/ui/fail/storage_never_freed.rs | 37 +++++++++++++ linting/ui/pass/storage_never_freed.rs | 47 ++++++++++++++++ 5 files changed, 169 insertions(+), 1 deletion(-) create mode 100644 linting/src/storage_never_freed.rs create mode 100644 linting/ui/fail/storage_never_freed.rs create mode 100644 linting/ui/pass/storage_never_freed.rs diff --git a/linting/Cargo.toml b/linting/Cargo.toml index 3f91dc017ae..6da4f19c306 100644 --- a/linting/Cargo.toml +++ b/linting/Cargo.toml @@ -51,6 +51,12 @@ path = "ui/pass/primitive_topic.rs" [[example]] name = "primitive_topic_fail" path = "ui/fail/primitive_topic.rs" +[[example]] +name = "storage_never_freed_pass" +path = "ui/pass/storage_never_freed.rs" +[[example]] +name = "storage_never_freed_fail" +path = "ui/fail/storage_never_freed.rs" [package.metadata.rust-analyzer] rustc_private = true diff --git a/linting/src/lib.rs b/linting/src/lib.rs index 5b1dc50b4b6..247e1d21b0c 100644 --- a/linting/src/lib.rs +++ b/linting/src/lib.rs @@ -36,8 +36,9 @@ pub fn register_lints( _sess: &rustc_session::Session, lint_store: &mut rustc_lint::LintStore, ) { - lint_store.register_lints(&[primitive_topic::PRIMITIVE_TOPIC]); + lint_store.register_lints(&[primitive_topic::PRIMITIVE_TOPIC, storage_never_freed::STORAGE_NEVER_FREED]); lint_store.register_late_pass(|_| Box::new(primitive_topic::PrimitiveTopic)); + lint_store.register_late_pass(|_| Box::new(storage_never_freed::StorageNeverFreed)); } #[test] diff --git a/linting/src/storage_never_freed.rs b/linting/src/storage_never_freed.rs new file mode 100644 index 00000000000..ccd8665976e --- /dev/null +++ b/linting/src/storage_never_freed.rs @@ -0,0 +1,77 @@ +// Copyright (C) Parity Technologies (UK) Ltd. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +use rustc_hir as hir; +use rustc_lint::{ + LateContext, + LateLintPass, +}; +use rustc_session::{ + declare_lint, + declare_lint_pass, +}; + +declare_lint! { + /// **What it does:** + /// This lint ensures that for every storage field with a collection type that supports adding + /// new elements, there's also an operation for removing elements. + /// + /// **Why is this bad?** + /// When a user executes a contract function that writes to storage, the user has to put a + /// deposit down for the amount of storage space used. Whoever frees up that storage at some + /// later point gets the deposit back. Therefore, it is always a good idea to make it possible + /// for users to free up their storage space. + /// + /// **Example:** + /// + /// In the following example there is a storage field with the `Mapping` type that has an + /// function that inserts new elements: + /// ```rust + /// #[ink(storage)] + /// pub struct Transaction { + /// values: Mapping, + /// } + /// + /// fn add_value(&mut self, k: &AccountId, v: &AccountId) { + /// // ... + /// self.values.insert(k, v); + /// // ... + /// } + /// ``` + /// + /// But, ideally, there also should be a function that allows the user to remove elements from + /// the Mapping freeing storage space: + /// + /// ```rust + /// fn del_value(&mut self, k: &AccountId) { + /// // ... + /// self.values.remove(k); + /// // ... + /// } + /// ``` + pub STORAGE_NEVER_FREED, + Allow, + "storage never freed" +} + +declare_lint_pass!(StorageNeverFreed => [STORAGE_NEVER_FREED]); + +impl<'tcx> LateLintPass<'tcx> for StorageNeverFreed { + fn check_mod( + &mut self, + cx: &LateContext<'tcx>, + m: &'tcx hir::Mod<'tcx>, + _: hir::HirId, + ) { + } +} diff --git a/linting/ui/fail/storage_never_freed.rs b/linting/ui/fail/storage_never_freed.rs new file mode 100644 index 00000000000..c647577d9cb --- /dev/null +++ b/linting/ui/fail/storage_never_freed.rs @@ -0,0 +1,37 @@ +#![cfg_attr(dylint_lib = "ink_linting", deny(storage_never_freed))] +pub type MapAlias = ink::storage::Mapping; + +#[ink::contract] +pub mod storage_never_freed { + use crate::MapAlias; + use ink::storage::Mapping; + + #[ink(storage)] + pub struct StorageNeverFreed { + // All the fields generate warnings, since there are `insert` operations for + // them, but there are no `remove` operations. + vec_field: Vec, + map_field: Mapping, + map_field2: MapAlias, + } + + impl StorageNeverFreed { + #[ink(constructor)] + pub fn new() -> Self { + Self { + vec_field: Vec::new(), + map_field: Mapping::new(), + map_field2: Mapping::new(), + } + } + + #[ink(message)] + pub fn add_to_fields(&mut self, v: AccountId) { + self.vec_field.push(v); + self.map_field.insert(v, &v); + self.map_field2.insert(v, &v); + } + } +} + +fn main() {} diff --git a/linting/ui/pass/storage_never_freed.rs b/linting/ui/pass/storage_never_freed.rs new file mode 100644 index 00000000000..156f3a09d40 --- /dev/null +++ b/linting/ui/pass/storage_never_freed.rs @@ -0,0 +1,47 @@ +#![cfg_attr(dylint_lib = "ink_linting", deny(storage_never_freed))] +pub type MapAlias = ink::storage::Mapping; + +#[ink::contract] +pub mod storage_never_freed { + use crate::MapAlias; + use ink::storage::Mapping; + + #[ink(storage)] + pub struct StorageNeverFreed { + vec_field: Vec, + map_field: Mapping, + map_field2: MapAlias, + #[cfg_attr(dylint_lib = "ink_linting", allow(storage_never_freed))] + map_field_suppressed: Mapping, + } + + impl StorageNeverFreed { + #[ink(constructor)] + pub fn new() -> Self { + Self { + vec_field: Vec::new(), + map_field: Mapping::new(), + map_field2: Mapping::new(), + map_field_suppressed: Mapping::new(), + } + } + + #[ink(message)] + pub fn add_to_fields(&mut self, v: AccountId) { + self.vec_field.push(v); + self.map_field.insert(v, &v); + self.map_field2.insert(v, &v); + self.map_field_suppressed.insert(v, &v); + } + + #[ink(message)] + pub fn remove_from_fields(&mut self, v: AccountId) { + self.vec_field.pop(); + self.map_field.remove(v); + self.map_field2.remove(v); + } + } +} + +fn main() {} + From c05262e4c4bf6529b56739fcb4f0e362fdeaba8d Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Wed, 27 Sep 2023 11:48:29 +0700 Subject: [PATCH 02/17] feat(lint): add utils and logic to collect `Vec`/`Mapping` types It reuses functions implemented in #1914, so we need to rebase and modify it when it is merged. --- linting/src/ink_utils.rs | 148 ++++++++++++++++++++++ linting/src/lib.rs | 2 + linting/src/storage_never_freed.rs | 162 ++++++++++++++++++++++++- linting/ui/fail/storage_never_freed.rs | 7 +- 4 files changed, 315 insertions(+), 4 deletions(-) create mode 100644 linting/src/ink_utils.rs diff --git a/linting/src/ink_utils.rs b/linting/src/ink_utils.rs new file mode 100644 index 00000000000..d1987c17f91 --- /dev/null +++ b/linting/src/ink_utils.rs @@ -0,0 +1,148 @@ +// Copyright (C) Parity Technologies (UK) Ltd. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +use clippy_utils::match_def_path; +use if_chain::if_chain; +use rustc_hir::{ + ExprKind, + HirId, + ItemId, + ItemKind, + QPath, + StmtKind, + Ty, + TyKind, +}; +use rustc_lint::LateContext; + +/// Returns `true` iff the ink storage attribute is defined for the given HIR +fn has_storage_attr(cx: &LateContext, hir: HirId) -> bool { + const INK_STORAGE: &str = "__ink_dylint_Storage"; + let attrs = format!("{:?}", cx.tcx.hir().attrs(hir)); + attrs.contains(INK_STORAGE) +} + +/// Returns `ItemId` of the structure annotated with `#[ink(storage)]` +pub(crate) fn find_storage_struct( + cx: &LateContext, + item_ids: &[ItemId], +) -> Option { + item_ids + .iter() + .find(|&item_id| { + let item = cx.tcx.hir().item(*item_id); + if_chain! { + if has_storage_attr(cx, item.hir_id()); + if let ItemKind::Struct(..) = item.kind; + then { true } else { false } + + } + }) + .copied() +} + +// TODO: Extracted from #1914; reuse this in #1914 when it is merged +/// Returns `ItemId`s defined inside the code block of `const _: () = {}`. +/// +/// The Rust code expanded after ink! code generation used these to define different +/// implementations of a contract. +fn items_in_unnamed_const(cx: &LateContext<'_>, id: &ItemId) -> Vec { + if_chain! { + if let ItemKind::Const(ty, body_id) = cx.tcx.hir().item(*id).kind; + if let TyKind::Tup([]) = ty.kind; + let body = cx.tcx.hir().body(body_id); + if let ExprKind::Block(block, _) = body.value.kind; + then { + block.stmts.iter().fold(Vec::new(), |mut acc, stmt| { + if let StmtKind::Item(id) = stmt.kind { + // We don't call `items_in_unnamed_const` here recursively, because the source + // code generated by ink! doesn't have nested `const _: () = {}` expressions. + acc.push(id) + } + acc + }) + } else { + vec![] + } + } +} + +// TODO: Extracted from #1914; reuse this in #1914 when it is merged +/// Collect all the `ItemId`s in nested `const _: () = {}` +pub(crate) fn expand_unnamed_consts( + cx: &LateContext<'_>, + item_ids: &[ItemId], +) -> Vec { + item_ids.iter().fold(Vec::new(), |mut acc, item_id| { + acc.push(*item_id); + acc.append(&mut items_in_unnamed_const(cx, item_id)); + acc + }) +} + +// TODO: Extracted from #1914; reuse this in #1914 when it is merged +/// Finds type of the struct that implements a contract with user-defined code +fn find_contract_ty_hir<'tcx>( + cx: &LateContext<'tcx>, + item_ids: &[ItemId], +) -> Option<&'tcx Ty<'tcx>> { + item_ids + .iter() + .find_map(|item_id| { + if_chain! { + let item = cx.tcx.hir().item(*item_id); + if let ItemKind::Impl(item_impl) = &item.kind; + if let Some(trait_ref) = cx.tcx.impl_trait_ref(item.owner_id); + if match_def_path( + cx, + trait_ref.skip_binder().def_id, + &["ink_env", "contract", "ContractEnv"], + ); + then { Some(&item_impl.self_ty) } else { None } + } + }) + .copied() +} + +// TODO: Extracted from #1914; reuse this in #1914 when it is merged +/// Compares types of two user-defined structs +fn eq_hir_struct_tys(lhs: &Ty<'_>, rhs: &Ty<'_>) -> bool { + match (lhs.kind, rhs.kind) { + ( + TyKind::Path(QPath::Resolved(_, lhs_path)), + TyKind::Path(QPath::Resolved(_, rhs_path)), + ) => lhs_path.res.eq(&rhs_path.res), + _ => false, + } +} + +// TODO: Extracted from #1914; reuse this in #1914 when it is merged +/// Finds an ID of the implementation of the contract struct containing user-defined code +pub(crate) fn find_contract_impl_id( + cx: &LateContext<'_>, + item_ids: Vec, +) -> Option { + let contract_struct_ty = find_contract_ty_hir(cx, &item_ids)?; + item_ids + .iter() + .find(|item_id| { + if_chain! { + let item = cx.tcx.hir().item(**item_id); + if let ItemKind::Impl(item_impl) = &item.kind; + if item_impl.of_trait.is_none(); + if eq_hir_struct_tys(contract_struct_ty, item_impl.self_ty); + then { true } else { false } + } + }) + .copied() +} diff --git a/linting/src/lib.rs b/linting/src/lib.rs index 247e1d21b0c..2004485dd24 100644 --- a/linting/src/lib.rs +++ b/linting/src/lib.rs @@ -29,6 +29,8 @@ extern crate rustc_session; extern crate rustc_span; mod primitive_topic; +mod storage_never_freed; +mod ink_utils; #[doc(hidden)] #[no_mangle] diff --git a/linting/src/storage_never_freed.rs b/linting/src/storage_never_freed.rs index ccd8665976e..12dd7845b2a 100644 --- a/linting/src/storage_never_freed.rs +++ b/linting/src/storage_never_freed.rs @@ -11,7 +11,38 @@ // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. // See the License for the specific language governing permissions and // limitations under the License. -use rustc_hir as hir; +use crate::ink_utils::{ + expand_unnamed_consts, + find_contract_impl_id, + find_storage_struct, +}; +use clippy_utils::{ + diagnostics::span_lint_and_then, + is_lint_allowed, + match_def_path, + match_path, + source::snippet_opt, +}; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::{ + self as hir, + def::{ + DefKind, + Res, + }, + def_id::{ + DefId, + LocalDefId, + }, + AssocItemKind, + ItemId, + ItemKind, + Node, + Path, + QPath, + TyKind, +}; use rustc_lint::{ LateContext, LateLintPass, @@ -66,6 +97,112 @@ declare_lint! { declare_lint_pass!(StorageNeverFreed => [STORAGE_NEVER_FREED]); +enum CollectionTy { + Vec, + Map, +} + +/// Fields with collection types that should have both insert and remove operations +struct FieldInfo { + pub did: LocalDefId, + pub ty: CollectionTy, + pub has_insert: bool, + pub has_remove: bool, +} + +impl FieldInfo { + pub fn new(did: LocalDefId, ty: CollectionTy) -> Self { + Self { + did, + ty, + has_insert: false, + has_remove: false, + } + } +} + +/// Returns `DefId` of a field if it has the `Vec` type +fn find_vec_did(cx: &LateContext, path: &Path) -> Option { + if_chain! { + if let Res::Def(DefKind::Struct, def_id) = path.res; + if match_def_path(cx, def_id, &["alloc", "vec", "Vec"]); + then { Some(def_id) } else { None } + } +} + +/// Returns `DefId` of a field if it has the `Mapping` type +fn find_map_did(cx: &LateContext, path: &Path) -> Option { + if_chain! { + if let Res::Def(DefKind::Struct, def_id) = path.res; + if match_def_path(cx, def_id, &["ink_storage", "lazy", "mapping", "Mapping"]); + then { Some(def_id) } else { None } + } +} + +/// Returns vectors of fields that have collection types +fn find_collection_fields(cx: &LateContext, storage_struct_id: ItemId) -> Vec { + let mut result = Vec::new(); + let item = cx.tcx.hir().item(storage_struct_id); + if let ItemKind::Struct(var_data, _) = item.kind { + var_data.fields().iter().for_each(|field_def| { + if_chain! { + // Collection fields of the storage are expanded like this: + // vec_field: as ::ink::storage::traits::AutoStorableHint< + // ::ink::storage::traits::ManualKey<993959520u32, ()>, + // >>::Type, + if let TyKind::Path(QPath::Resolved(Some(ty), path)) = field_def.ty.kind; + if match_path(path, &["ink", "storage", "traits", "AutoStorableHint", "Type"]); + if let TyKind::Path(QPath::Resolved(None, path)) = ty.kind; + then { + // TODO: Inspect type aliases + if let Some(_did) = find_vec_did(cx, path) { + result.push(FieldInfo::new(field_def.def_id, CollectionTy::Vec)); + return; + } + if let Some(_did) = find_map_did(cx, path) { + result.push(FieldInfo::new(field_def.def_id, CollectionTy::Map)); + } + } + } + }) + }; + result +} + +/// Reports the given field defintion +fn report_field(cx: &LateContext, field_info: &FieldInfo) { + if_chain! { + if let Node::Field(field) = cx.tcx.hir().get_by_def_id(field_info.did); + if !is_lint_allowed(cx, STORAGE_NEVER_FREED, field.hir_id); + then { + span_lint_and_then( + cx, + STORAGE_NEVER_FREED, + field.span, + "storage never freed", + |diag| { + let snippet = snippet_opt(cx, field.span).expect("snippet must exist"); + diag.span_suggestion( + field.span, + "consider adding operations to remove elements available to the user".to_string(), + snippet, + Applicability::Unspecified, + ); + }, + ) + + } + } +} + +/// Collects information about `insert` and `remove` operations in the body of the +/// function +fn collect_insert_remove_ops(fields: &mut Vec) { + todo!() +} + impl<'tcx> LateLintPass<'tcx> for StorageNeverFreed { fn check_mod( &mut self, @@ -73,5 +210,28 @@ impl<'tcx> LateLintPass<'tcx> for StorageNeverFreed { m: &'tcx hir::Mod<'tcx>, _: hir::HirId, ) { + if_chain! { + // Find fields of Vec/Mapping type + if let Some(storage_struct_id) = find_storage_struct(cx, m.item_ids); + let mut fields = find_collection_fields(cx, storage_struct_id); + if !fields.is_empty(); + // Find all the user-defined functions of the contract + let all_item_ids = expand_unnamed_consts(cx, m.item_ids); + if let Some(contract_impl_id) = find_contract_impl_id(cx, all_item_ids); + let contract_impl = cx.tcx.hir().item(contract_impl_id); + if let ItemKind::Impl(contract_impl) = contract_impl.kind; + then { + contract_impl.items.iter().for_each(|impl_item| { + if let AssocItemKind::Fn { .. } = impl_item.kind { + collect_insert_remove_ops(&mut fields); + } + }); + fields.iter().for_each(|field| { + if field.has_insert && !field.has_remove { + report_field(cx, field) + } + }) + } + } } } diff --git a/linting/ui/fail/storage_never_freed.rs b/linting/ui/fail/storage_never_freed.rs index c647577d9cb..63ef3796329 100644 --- a/linting/ui/fail/storage_never_freed.rs +++ b/linting/ui/fail/storage_never_freed.rs @@ -1,9 +1,10 @@ #![cfg_attr(dylint_lib = "ink_linting", deny(storage_never_freed))] -pub type MapAlias = ink::storage::Mapping; +pub type MapAlias1 = ink::storage::Mapping; +pub type MapAlias2 = MapAlias1; #[ink::contract] pub mod storage_never_freed { - use crate::MapAlias; + use crate::MapAlias2; use ink::storage::Mapping; #[ink(storage)] @@ -12,7 +13,7 @@ pub mod storage_never_freed { // them, but there are no `remove` operations. vec_field: Vec, map_field: Mapping, - map_field2: MapAlias, + map_field2: MapAlias2, } impl StorageNeverFreed { From d48fb220d53c1e4569fb868d4e9b5ff577af1610 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Tue, 3 Oct 2023 17:44:06 +0700 Subject: [PATCH 03/17] feat(lint): Make it pass simple tests --- linting/src/storage_never_freed.rs | 104 ++++++++++++++++++--- linting/ui/fail/storage_never_freed.stderr | 29 ++++++ 2 files changed, 121 insertions(+), 12 deletions(-) create mode 100644 linting/ui/fail/storage_never_freed.stderr diff --git a/linting/src/storage_never_freed.rs b/linting/src/storage_never_freed.rs index 12dd7845b2a..2bdf01c3d0f 100644 --- a/linting/src/storage_never_freed.rs +++ b/linting/src/storage_never_freed.rs @@ -35,7 +35,14 @@ use rustc_hir::{ DefId, LocalDefId, }, - AssocItemKind, + intravisit::{ + walk_body, + walk_expr, + Visitor, + }, + Expr, + ExprKind, + ImplItemKind, ItemId, ItemKind, Node, @@ -51,6 +58,7 @@ use rustc_session::{ declare_lint, declare_lint_pass, }; +use std::collections::BTreeMap; declare_lint! { /// **What it does:** @@ -106,9 +114,36 @@ enum CollectionTy { struct FieldInfo { pub did: LocalDefId, pub ty: CollectionTy, + // TODO: replace w/ ids pub has_insert: bool, pub has_remove: bool, } +type FieldName = String; +type FieldsMap = BTreeMap; + +// https://paritytech.github.io/ink/ink_prelude/vec/struct.Vec.html +const VEC_INSERT_OPERATIONS: [&str; 6] = [ + "append", + "extend_from_slice", + "extend_from_within", + "insert", + "push", + "push_with_capacity", +]; +const VEC_REMOVE_OPERATIONS: [&str; 8] = [ + "clear", + "dedup", + "pop", + "remove", + "retain", + "retain_mut", + "swap_remove", + "truncate", +]; + +// https://paritytech.github.io/ink/ink_storage/struct.Mapping.html +const MAP_INSERT_OPERATIONS: [&str; 1] = ["insert"]; +const MAP_REMOVE_OPERATIONS: [&str; 2] = ["remove", "take"]; impl FieldInfo { pub fn new(did: LocalDefId, ty: CollectionTy) -> Self { @@ -140,8 +175,8 @@ fn find_map_did(cx: &LateContext, path: &Path) -> Option { } /// Returns vectors of fields that have collection types -fn find_collection_fields(cx: &LateContext, storage_struct_id: ItemId) -> Vec { - let mut result = Vec::new(); +fn find_collection_fields(cx: &LateContext, storage_struct_id: ItemId) -> FieldsMap { + let mut result = FieldsMap::new(); let item = cx.tcx.hir().item(storage_struct_id); if let ItemKind::Struct(var_data, _) = item.kind { var_data.fields().iter().for_each(|field_def| { @@ -156,13 +191,14 @@ fn find_collection_fields(cx: &LateContext, storage_struct_id: ItemId) -> Vec) { - todo!() +/// Visitor that collects `insert` and `remove` operations +struct InsertRemoveCollector<'a, 'b, 'tcx> { + cx: &'tcx LateContext<'a>, + fields: &'b mut FieldsMap, +} + +impl<'a, 'b, 'tcx> InsertRemoveCollector<'a, 'b, 'tcx> { + fn new(cx: &'tcx LateContext<'a>, fields: &'b mut FieldsMap) -> Self { + Self { cx, fields } + } +} + +impl<'hir> Visitor<'hir> for InsertRemoveCollector<'_, '_, '_> { + fn visit_expr(&mut self, e: &'hir Expr<'hir>) { + if let ExprKind::MethodCall(method_path, receiver, args, _) = &e.kind { + if_chain! { + if let ExprKind::Field(s, field) = &receiver.kind; + if let ExprKind::Path(ref path) = s.kind; + let ty = self.cx.qpath_res(path, s.hir_id); + // TODO: check if self + let field_name = field.name.as_str(); + let method_name = method_path.ident.as_str(); + then { + self.fields.entry(field_name.to_string()).and_modify(|field_info| { + match field_info.ty { + CollectionTy::Vec if VEC_INSERT_OPERATIONS.contains(&method_name) => { + field_info.has_insert = true; + }, + CollectionTy::Vec if VEC_REMOVE_OPERATIONS.contains(&method_name) => { + field_info.has_remove = true; + }, + CollectionTy::Map if MAP_INSERT_OPERATIONS.contains(&method_name) => { + field_info.has_insert = true; + }, + CollectionTy::Map if MAP_REMOVE_OPERATIONS.contains(&method_name) => { + field_info.has_remove = true; + }, + _ => () + } + }); + } + } + args.iter().for_each(|arg| walk_expr(self, arg)); + } + walk_expr(self, e); + } } impl<'tcx> LateLintPass<'tcx> for StorageNeverFreed { @@ -222,11 +300,13 @@ impl<'tcx> LateLintPass<'tcx> for StorageNeverFreed { if let ItemKind::Impl(contract_impl) = contract_impl.kind; then { contract_impl.items.iter().for_each(|impl_item| { - if let AssocItemKind::Fn { .. } = impl_item.kind { - collect_insert_remove_ops(&mut fields); + let impl_item = cx.tcx.hir().impl_item(impl_item.id); + if let ImplItemKind::Fn(_, fn_body_id) = impl_item.kind { + let mut visitor = InsertRemoveCollector::new(cx, &mut fields); + walk_body(&mut visitor, cx.tcx.hir().body(fn_body_id)); } }); - fields.iter().for_each(|field| { + fields.iter().for_each(|(_, field)| { if field.has_insert && !field.has_remove { report_field(cx, field) } diff --git a/linting/ui/fail/storage_never_freed.stderr b/linting/ui/fail/storage_never_freed.stderr new file mode 100644 index 00000000000..72c28886694 --- /dev/null +++ b/linting/ui/fail/storage_never_freed.stderr @@ -0,0 +1,29 @@ +error: storage never freed + --> $DIR/storage_never_freed.rs:15:9 + | +LL | map_field: Mapping, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: the lint level is defined here + --> $DIR/storage_never_freed.rs:1:46 + | +LL | #![cfg_attr(dylint_lib = "ink_linting", deny(storage_never_freed))] + | ^^^^^^^^^^^^^^^^^^^ +help: consider adding operations to remove elements available to the user + | +LL | map_field: Mapping, + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: storage never freed + --> $DIR/storage_never_freed.rs:14:9 + | +LL | vec_field: Vec, + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider adding operations to remove elements available to the user + | +LL | vec_field: Vec, + | ~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: aborting due to 2 previous errors + From 1de2f21b602000bee7b7d217582c80078bebecf3 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Tue, 3 Oct 2023 18:32:23 +0700 Subject: [PATCH 04/17] feat(lint): Support index operations (`vec[]`) --- linting/src/storage_never_freed.rs | 83 ++++++++++++++++++-------- linting/ui/fail/storage_never_freed.rs | 9 ++- 2 files changed, 63 insertions(+), 29 deletions(-) diff --git a/linting/src/storage_never_freed.rs b/linting/src/storage_never_freed.rs index 2bdf01c3d0f..8ae953b517e 100644 --- a/linting/src/storage_never_freed.rs +++ b/linting/src/storage_never_freed.rs @@ -243,39 +243,70 @@ impl<'a, 'b, 'tcx> InsertRemoveCollector<'a, 'b, 'tcx> { fn new(cx: &'tcx LateContext<'a>, fields: &'b mut FieldsMap) -> Self { Self { cx, fields } } + + /// Finds a field of the supported type in the given expression present with the form + /// `self.field_name` + fn find_field_name(&self, e: &Expr) -> Option { + if_chain! { + if let ExprKind::Field(s, field) = &e.kind; + if let ExprKind::Path(ref path) = s.kind; + let ty = self.cx.qpath_res(path, s.hir_id); + // TODO: check if ty is `self` + then { Some(field.name.as_str().to_string()) } else { None } + } + } } impl<'hir> Visitor<'hir> for InsertRemoveCollector<'_, '_, '_> { fn visit_expr(&mut self, e: &'hir Expr<'hir>) { - if let ExprKind::MethodCall(method_path, receiver, args, _) = &e.kind { - if_chain! { - if let ExprKind::Field(s, field) = &receiver.kind; - if let ExprKind::Path(ref path) = s.kind; - let ty = self.cx.qpath_res(path, s.hir_id); - // TODO: check if self - let field_name = field.name.as_str(); - let method_name = method_path.ident.as_str(); - then { - self.fields.entry(field_name.to_string()).and_modify(|field_info| { - match field_info.ty { - CollectionTy::Vec if VEC_INSERT_OPERATIONS.contains(&method_name) => { - field_info.has_insert = true; - }, - CollectionTy::Vec if VEC_REMOVE_OPERATIONS.contains(&method_name) => { - field_info.has_remove = true; - }, - CollectionTy::Map if MAP_INSERT_OPERATIONS.contains(&method_name) => { + match &e.kind { + ExprKind::Assign(lhs, ..) => { + if_chain! { + if let ExprKind::Index(field, _) = lhs.kind; + if let Some(field_name) = self.find_field_name(field); + then { + self.fields + .entry(field_name.to_string()) + .and_modify(|field_info| { field_info.has_insert = true; - }, - CollectionTy::Map if MAP_REMOVE_OPERATIONS.contains(&method_name) => { - field_info.has_remove = true; - }, - _ => () - } - }); + }); + } + } + } + ExprKind::MethodCall(method_path, receiver, args, _) => { + if let Some(field_name) = self.find_field_name(receiver) { + let method_name = method_path.ident.as_str(); + self.fields + .entry(field_name.to_string()) + .and_modify(|field_info| { + match field_info.ty { + CollectionTy::Vec + if VEC_INSERT_OPERATIONS.contains(&method_name) => + { + field_info.has_insert = true; + } + CollectionTy::Vec + if VEC_REMOVE_OPERATIONS.contains(&method_name) => + { + field_info.has_remove = true; + } + CollectionTy::Map + if MAP_INSERT_OPERATIONS.contains(&method_name) => + { + field_info.has_insert = true; + } + CollectionTy::Map + if MAP_REMOVE_OPERATIONS.contains(&method_name) => + { + field_info.has_remove = true; + } + _ => (), + } + }); } + args.iter().for_each(|arg| walk_expr(self, arg)); } - args.iter().for_each(|arg| walk_expr(self, arg)); + _ => (), } walk_expr(self, e); } diff --git a/linting/ui/fail/storage_never_freed.rs b/linting/ui/fail/storage_never_freed.rs index 63ef3796329..4d8dfdddce3 100644 --- a/linting/ui/fail/storage_never_freed.rs +++ b/linting/ui/fail/storage_never_freed.rs @@ -12,8 +12,9 @@ pub mod storage_never_freed { // All the fields generate warnings, since there are `insert` operations for // them, but there are no `remove` operations. vec_field: Vec, + vec_field_subscription: Vec, map_field: Mapping, - map_field2: MapAlias2, + map_field_alias: MapAlias2, } impl StorageNeverFreed { @@ -21,8 +22,9 @@ pub mod storage_never_freed { pub fn new() -> Self { Self { vec_field: Vec::new(), + vec_field_subscription: Vec::new(), map_field: Mapping::new(), - map_field2: Mapping::new(), + map_field_alias: Mapping::new(), } } @@ -30,7 +32,8 @@ pub mod storage_never_freed { pub fn add_to_fields(&mut self, v: AccountId) { self.vec_field.push(v); self.map_field.insert(v, &v); - self.map_field2.insert(v, &v); + self.vec_field_subscription[0] = v; + self.map_field_alias.insert(v, &v); } } } From b46915f883c924550b392302400b4b3932bd58f9 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Tue, 3 Oct 2023 18:59:24 +0700 Subject: [PATCH 05/17] feat(lint): Ignore fields that has mutable unsafe pointers --- linting/src/storage_never_freed.rs | 46 +++++++++++----------- linting/ui/fail/storage_never_freed.stderr | 15 ++++++- linting/ui/pass/storage_never_freed.rs | 15 ++++++- 3 files changed, 50 insertions(+), 26 deletions(-) diff --git a/linting/src/storage_never_freed.rs b/linting/src/storage_never_freed.rs index 8ae953b517e..e4348137996 100644 --- a/linting/src/storage_never_freed.rs +++ b/linting/src/storage_never_freed.rs @@ -58,7 +58,10 @@ use rustc_session::{ declare_lint, declare_lint_pass, }; -use std::collections::BTreeMap; +use std::collections::{ + btree_map::Entry, + BTreeMap, +}; declare_lint! { /// **What it does:** @@ -140,6 +143,7 @@ const VEC_REMOVE_OPERATIONS: [&str; 8] = [ "swap_remove", "truncate", ]; +const VEC_IGNORE_OPERATIONS: [&str; 2] = ["as_mut_ptr", "as_mut_slice"]; // https://paritytech.github.io/ink/ink_storage/struct.Mapping.html const MAP_INSERT_OPERATIONS: [&str; 1] = ["insert"]; @@ -274,37 +278,33 @@ impl<'hir> Visitor<'hir> for InsertRemoveCollector<'_, '_, '_> { } } ExprKind::MethodCall(method_path, receiver, args, _) => { - if let Some(field_name) = self.find_field_name(receiver) { + args.iter().for_each(|arg| walk_expr(self, arg)); + if_chain! { + if let Some(field_name) = self.find_field_name(receiver); + if let Entry::Occupied(mut e) = self.fields.entry(field_name.to_string()); let method_name = method_path.ident.as_str(); - self.fields - .entry(field_name.to_string()) - .and_modify(|field_info| { - match field_info.ty { - CollectionTy::Vec - if VEC_INSERT_OPERATIONS.contains(&method_name) => - { + then { + let field_info = e.get_mut(); + match field_info.ty { + CollectionTy::Vec => { + if VEC_IGNORE_OPERATIONS.contains(&method_name) { + e.remove(); + } else if VEC_INSERT_OPERATIONS.contains(&method_name) { field_info.has_insert = true; - } - CollectionTy::Vec - if VEC_REMOVE_OPERATIONS.contains(&method_name) => - { + } else if VEC_REMOVE_OPERATIONS.contains(&method_name) { field_info.has_remove = true; } - CollectionTy::Map - if MAP_INSERT_OPERATIONS.contains(&method_name) => - { + }, + CollectionTy::Map => { + if MAP_INSERT_OPERATIONS.contains(&method_name) { field_info.has_insert = true; - } - CollectionTy::Map - if MAP_REMOVE_OPERATIONS.contains(&method_name) => - { + } else if MAP_REMOVE_OPERATIONS.contains(&method_name) { field_info.has_remove = true; } - _ => (), } - }); + } + } } - args.iter().for_each(|arg| walk_expr(self, arg)); } _ => (), } diff --git a/linting/ui/fail/storage_never_freed.stderr b/linting/ui/fail/storage_never_freed.stderr index 72c28886694..983aaa96a65 100644 --- a/linting/ui/fail/storage_never_freed.stderr +++ b/linting/ui/fail/storage_never_freed.stderr @@ -1,5 +1,5 @@ error: storage never freed - --> $DIR/storage_never_freed.rs:15:9 + --> $DIR/storage_never_freed.rs:16:9 | LL | map_field: Mapping, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -25,5 +25,16 @@ help: consider adding operations to remove elements available to the user LL | vec_field: Vec, | ~~~~~~~~~~~~~~~~~~~~~~~~~ -error: aborting due to 2 previous errors +error: storage never freed + --> $DIR/storage_never_freed.rs:15:9 + | +LL | vec_field_subscription: Vec, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider adding operations to remove elements available to the user + | +LL | vec_field_subscription: Vec, + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: aborting due to 3 previous errors diff --git a/linting/ui/pass/storage_never_freed.rs b/linting/ui/pass/storage_never_freed.rs index 156f3a09d40..77d2c274751 100644 --- a/linting/ui/pass/storage_never_freed.rs +++ b/linting/ui/pass/storage_never_freed.rs @@ -13,6 +13,10 @@ pub mod storage_never_freed { map_field2: MapAlias, #[cfg_attr(dylint_lib = "ink_linting", allow(storage_never_freed))] map_field_suppressed: Mapping, + + // Vec which buffer was used unsafe operations with their raw pointers are not + // reported + vec_field_mut_pointer: Vec, } impl StorageNeverFreed { @@ -23,6 +27,7 @@ pub mod storage_never_freed { map_field: Mapping::new(), map_field2: Mapping::new(), map_field_suppressed: Mapping::new(), + vec_field_mut_pointer: Vec::new(), } } @@ -32,6 +37,15 @@ pub mod storage_never_freed { self.map_field.insert(v, &v); self.map_field2.insert(v, &v); self.map_field_suppressed.insert(v, &v); + + // Should not be reported, since elements may be removed using the pointer + self.vec_field_mut_pointer[0] = v; + unsafe { + let ptr = self.vec_field_mut_pointer.as_mut_ptr(); + let new_len = self.vec_field_mut_pointer.len() - 1; + std::ptr::copy(ptr.offset(1), ptr, new_len); + self.vec_field_mut_pointer.set_len(new_len); + } } #[ink(message)] @@ -44,4 +58,3 @@ pub mod storage_never_freed { } fn main() {} - From a9121cb3788e9743bd29a21bd60319df98b0a8ba Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Tue, 3 Oct 2023 19:04:42 +0700 Subject: [PATCH 06/17] chore(lint): Refactor --- linting/src/storage_never_freed.rs | 62 +++++++++++++++--------------- 1 file changed, 32 insertions(+), 30 deletions(-) diff --git a/linting/src/storage_never_freed.rs b/linting/src/storage_never_freed.rs index e4348137996..395a9fa7cd5 100644 --- a/linting/src/storage_never_freed.rs +++ b/linting/src/storage_never_freed.rs @@ -108,6 +108,33 @@ declare_lint! { declare_lint_pass!(StorageNeverFreed => [STORAGE_NEVER_FREED]); +mod methods { + // https://paritytech.github.io/ink/ink_prelude/vec/struct.Vec.html + pub const VEC_INSERT: [&str; 6] = [ + "append", + "extend_from_slice", + "extend_from_within", + "insert", + "push", + "push_with_capacity", + ]; + pub const VEC_REMOVE: [&str; 8] = [ + "clear", + "dedup", + "pop", + "remove", + "retain", + "retain_mut", + "swap_remove", + "truncate", + ]; + pub const VEC_IGNORE: [&str; 2] = ["as_mut_ptr", "as_mut_slice"]; + + // https://paritytech.github.io/ink/ink_storage/struct.Mapping.html + pub const MAP_INSERT: [&str; 1] = ["insert"]; + pub const MAP_REMOVE: [&str; 2] = ["remove", "take"]; +} + enum CollectionTy { Vec, Map, @@ -124,31 +151,6 @@ struct FieldInfo { type FieldName = String; type FieldsMap = BTreeMap; -// https://paritytech.github.io/ink/ink_prelude/vec/struct.Vec.html -const VEC_INSERT_OPERATIONS: [&str; 6] = [ - "append", - "extend_from_slice", - "extend_from_within", - "insert", - "push", - "push_with_capacity", -]; -const VEC_REMOVE_OPERATIONS: [&str; 8] = [ - "clear", - "dedup", - "pop", - "remove", - "retain", - "retain_mut", - "swap_remove", - "truncate", -]; -const VEC_IGNORE_OPERATIONS: [&str; 2] = ["as_mut_ptr", "as_mut_slice"]; - -// https://paritytech.github.io/ink/ink_storage/struct.Mapping.html -const MAP_INSERT_OPERATIONS: [&str; 1] = ["insert"]; -const MAP_REMOVE_OPERATIONS: [&str; 2] = ["remove", "take"]; - impl FieldInfo { pub fn new(did: LocalDefId, ty: CollectionTy) -> Self { Self { @@ -287,18 +289,18 @@ impl<'hir> Visitor<'hir> for InsertRemoveCollector<'_, '_, '_> { let field_info = e.get_mut(); match field_info.ty { CollectionTy::Vec => { - if VEC_IGNORE_OPERATIONS.contains(&method_name) { + if methods::VEC_IGNORE.contains(&method_name) { e.remove(); - } else if VEC_INSERT_OPERATIONS.contains(&method_name) { + } else if methods::VEC_INSERT.contains(&method_name) { field_info.has_insert = true; - } else if VEC_REMOVE_OPERATIONS.contains(&method_name) { + } else if methods::VEC_REMOVE.contains(&method_name) { field_info.has_remove = true; } }, CollectionTy::Map => { - if MAP_INSERT_OPERATIONS.contains(&method_name) { + if methods::MAP_INSERT.contains(&method_name) { field_info.has_insert = true; - } else if MAP_REMOVE_OPERATIONS.contains(&method_name) { + } else if methods::MAP_REMOVE.contains(&method_name) { field_info.has_remove = true; } } From ee3f98bbea64b567a38199ed9cab95822e4a8b45 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Tue, 3 Oct 2023 19:09:45 +0700 Subject: [PATCH 07/17] feat(test): Inserting in method arg --- linting/ui/fail/storage_never_freed.rs | 19 +++++++++----- linting/ui/fail/storage_never_freed.stderr | 29 +++++++++++++++------- 2 files changed, 33 insertions(+), 15 deletions(-) diff --git a/linting/ui/fail/storage_never_freed.rs b/linting/ui/fail/storage_never_freed.rs index 4d8dfdddce3..cc653d70de4 100644 --- a/linting/ui/fail/storage_never_freed.rs +++ b/linting/ui/fail/storage_never_freed.rs @@ -11,9 +11,10 @@ pub mod storage_never_freed { pub struct StorageNeverFreed { // All the fields generate warnings, since there are `insert` operations for // them, but there are no `remove` operations. - vec_field: Vec, + vec_field_1: Vec, vec_field_subscription: Vec, - map_field: Mapping, + map_field_1: Mapping, + map_field_2: Mapping, map_field_alias: MapAlias2, } @@ -21,18 +22,24 @@ pub mod storage_never_freed { #[ink(constructor)] pub fn new() -> Self { Self { - vec_field: Vec::new(), + vec_field_1: Vec::new(), vec_field_subscription: Vec::new(), - map_field: Mapping::new(), + map_field_1: Mapping::new(), + map_field_2: Mapping::new(), map_field_alias: Mapping::new(), } } + fn flip(a: bool) -> bool { + !a + } + #[ink(message)] pub fn add_to_fields(&mut self, v: AccountId) { - self.vec_field.push(v); - self.map_field.insert(v, &v); + self.vec_field_1.push(v); self.vec_field_subscription[0] = v; + self.map_field_1.insert(v, &v); + let _ = Self::flip(self.map_field_2.insert(v, &v).is_some()); self.map_field_alias.insert(v, &v); } } diff --git a/linting/ui/fail/storage_never_freed.stderr b/linting/ui/fail/storage_never_freed.stderr index 983aaa96a65..a142f3fa960 100644 --- a/linting/ui/fail/storage_never_freed.stderr +++ b/linting/ui/fail/storage_never_freed.stderr @@ -1,8 +1,8 @@ error: storage never freed --> $DIR/storage_never_freed.rs:16:9 | -LL | map_field: Mapping, - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | map_field_1: Mapping, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: the lint level is defined here --> $DIR/storage_never_freed.rs:1:46 @@ -11,19 +11,30 @@ LL | #![cfg_attr(dylint_lib = "ink_linting", deny(storage_never_freed))] | ^^^^^^^^^^^^^^^^^^^ help: consider adding operations to remove elements available to the user | -LL | map_field: Mapping, - | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +LL | map_field_1: Mapping, + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: storage never freed + --> $DIR/storage_never_freed.rs:17:9 + | +LL | map_field_2: Mapping, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider adding operations to remove elements available to the user + | +LL | map_field_2: Mapping, + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: storage never freed --> $DIR/storage_never_freed.rs:14:9 | -LL | vec_field: Vec, - | ^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | vec_field_1: Vec, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | help: consider adding operations to remove elements available to the user | -LL | vec_field: Vec, - | ~~~~~~~~~~~~~~~~~~~~~~~~~ +LL | vec_field_1: Vec, + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: storage never freed --> $DIR/storage_never_freed.rs:15:9 @@ -36,5 +47,5 @@ help: consider adding operations to remove elements available to the user LL | vec_field_subscription: Vec, | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -error: aborting due to 3 previous errors +error: aborting due to 4 previous errors From 4e857f4de0e2758212096c9fe35f583d6a46bade Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Tue, 3 Oct 2023 19:10:54 +0700 Subject: [PATCH 08/17] chore(test): Refactor --- linting/ui/fail/storage_never_freed.rs | 30 ++++++++++---------- linting/ui/fail/storage_never_freed.stderr | 32 +++++++++++----------- 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/linting/ui/fail/storage_never_freed.rs b/linting/ui/fail/storage_never_freed.rs index cc653d70de4..5eea900a34e 100644 --- a/linting/ui/fail/storage_never_freed.rs +++ b/linting/ui/fail/storage_never_freed.rs @@ -11,22 +11,22 @@ pub mod storage_never_freed { pub struct StorageNeverFreed { // All the fields generate warnings, since there are `insert` operations for // them, but there are no `remove` operations. - vec_field_1: Vec, - vec_field_subscription: Vec, - map_field_1: Mapping, - map_field_2: Mapping, - map_field_alias: MapAlias2, + vec_1: Vec, + vec_subscription: Vec, + map_1: Mapping, + map_2: Mapping, + map_alias: MapAlias2, } impl StorageNeverFreed { #[ink(constructor)] pub fn new() -> Self { Self { - vec_field_1: Vec::new(), - vec_field_subscription: Vec::new(), - map_field_1: Mapping::new(), - map_field_2: Mapping::new(), - map_field_alias: Mapping::new(), + vec_1: Vec::new(), + vec_subscription: Vec::new(), + map_1: Mapping::new(), + map_2: Mapping::new(), + map_alias: Mapping::new(), } } @@ -36,11 +36,11 @@ pub mod storage_never_freed { #[ink(message)] pub fn add_to_fields(&mut self, v: AccountId) { - self.vec_field_1.push(v); - self.vec_field_subscription[0] = v; - self.map_field_1.insert(v, &v); - let _ = Self::flip(self.map_field_2.insert(v, &v).is_some()); - self.map_field_alias.insert(v, &v); + self.vec_1.push(v); + self.vec_subscription[0] = v; + self.map_1.insert(v, &v); + let _ = Self::flip(self.map_2.insert(v, &v).is_some()); + self.map_alias.insert(v, &v); } } } diff --git a/linting/ui/fail/storage_never_freed.stderr b/linting/ui/fail/storage_never_freed.stderr index a142f3fa960..64703287ff3 100644 --- a/linting/ui/fail/storage_never_freed.stderr +++ b/linting/ui/fail/storage_never_freed.stderr @@ -1,8 +1,8 @@ error: storage never freed --> $DIR/storage_never_freed.rs:16:9 | -LL | map_field_1: Mapping, - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | map_1: Mapping, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: the lint level is defined here --> $DIR/storage_never_freed.rs:1:46 @@ -11,41 +11,41 @@ LL | #![cfg_attr(dylint_lib = "ink_linting", deny(storage_never_freed))] | ^^^^^^^^^^^^^^^^^^^ help: consider adding operations to remove elements available to the user | -LL | map_field_1: Mapping, - | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +LL | map_1: Mapping, + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: storage never freed --> $DIR/storage_never_freed.rs:17:9 | -LL | map_field_2: Mapping, - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | map_2: Mapping, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | help: consider adding operations to remove elements available to the user | -LL | map_field_2: Mapping, - | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +LL | map_2: Mapping, + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: storage never freed --> $DIR/storage_never_freed.rs:14:9 | -LL | vec_field_1: Vec, - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | vec_1: Vec, + | ^^^^^^^^^^^^^^^^^^^^^ | help: consider adding operations to remove elements available to the user | -LL | vec_field_1: Vec, - | ~~~~~~~~~~~~~~~~~~~~~~~~~~~ +LL | vec_1: Vec, + | ~~~~~~~~~~~~~~~~~~~~~ error: storage never freed --> $DIR/storage_never_freed.rs:15:9 | -LL | vec_field_subscription: Vec, - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | vec_subscription: Vec, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | help: consider adding operations to remove elements available to the user | -LL | vec_field_subscription: Vec, - | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +LL | vec_subscription: Vec, + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: aborting due to 4 previous errors From 74af28f9c4123073e4cda65935dd3fbab2cafea3 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Tue, 3 Oct 2023 19:13:12 +0700 Subject: [PATCH 09/17] feat(test): Insert inside insert --- linting/ui/fail/storage_never_freed.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/linting/ui/fail/storage_never_freed.rs b/linting/ui/fail/storage_never_freed.rs index 5eea900a34e..d0fd1cdfa66 100644 --- a/linting/ui/fail/storage_never_freed.rs +++ b/linting/ui/fail/storage_never_freed.rs @@ -12,9 +12,11 @@ pub mod storage_never_freed { // All the fields generate warnings, since there are `insert` operations for // them, but there are no `remove` operations. vec_1: Vec, + vec_2: Vec, vec_subscription: Vec, map_1: Mapping, map_2: Mapping, + map_3: Mapping, map_alias: MapAlias2, } @@ -23,9 +25,11 @@ pub mod storage_never_freed { pub fn new() -> Self { Self { vec_1: Vec::new(), + vec_2: Vec::new(), vec_subscription: Vec::new(), map_1: Mapping::new(), map_2: Mapping::new(), + map_3: Mapping::new(), map_alias: Mapping::new(), } } @@ -41,6 +45,7 @@ pub mod storage_never_freed { self.map_1.insert(v, &v); let _ = Self::flip(self.map_2.insert(v, &v).is_some()); self.map_alias.insert(v, &v); + self.vec_2.push(self.map_3.insert(v, &v).is_some()); } } } From 1c11849b0cda71dd36f208acdb061d945b3aca63 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Tue, 3 Oct 2023 19:20:39 +0700 Subject: [PATCH 10/17] chore(lint): Refactor --- linting/src/storage_never_freed.rs | 42 +++++++++++++++--------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/linting/src/storage_never_freed.rs b/linting/src/storage_never_freed.rs index 395a9fa7cd5..a3ef7194183 100644 --- a/linting/src/storage_never_freed.rs +++ b/linting/src/storage_never_freed.rs @@ -162,21 +162,25 @@ impl FieldInfo { } } -/// Returns `DefId` of a field if it has the `Vec` type -fn find_vec_did(cx: &LateContext, path: &Path) -> Option { - if_chain! { - if let Res::Def(DefKind::Struct, def_id) = path.res; - if match_def_path(cx, def_id, &["alloc", "vec", "Vec"]); - then { Some(def_id) } else { None } - } -} - -/// Returns `DefId` of a field if it has the `Mapping` type -fn find_map_did(cx: &LateContext, path: &Path) -> Option { - if_chain! { - if let Res::Def(DefKind::Struct, def_id) = path.res; - if match_def_path(cx, def_id, &["ink_storage", "lazy", "mapping", "Mapping"]); - then { Some(def_id) } else { None } +/// Finds `DefId` of the path that has the supported collection type +fn find_collection_def_id( + cx: &LateContext, + path: &Path, +) -> Option<(DefId, CollectionTy)> { + if let Res::Def(DefKind::Struct, def_id) = path.res { + if match_def_path(cx, def_id, &["alloc", "vec", "Vec"]) { + Some((def_id, CollectionTy::Vec)) + } else if match_def_path( + cx, + def_id, + &["ink_storage", "lazy", "mapping", "Mapping"], + ) { + Some((def_id, CollectionTy::Map)) + } else { + None + } + } else { + None } } @@ -199,12 +203,8 @@ fn find_collection_fields(cx: &LateContext, storage_struct_id: ItemId) -> Fields then { let field_name = field_def.ident.name.as_str(); // TODO: Inspect type aliases - if let Some(_did) = find_vec_did(cx, path) { - result.insert(field_name.to_string(), FieldInfo::new(field_def.def_id, CollectionTy::Vec)); - return; - } - if let Some(_did) = find_map_did(cx, path) { - result.insert(field_name.to_string(), FieldInfo::new(field_def.def_id, CollectionTy::Map)); + if let Some((_, ty)) = find_collection_def_id(cx, path) { + result.insert(field_name.to_string(), FieldInfo::new(field_def.def_id, ty)); } } } From dea136d0c38c3e20629d012b64b8b994901faf5f Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Wed, 4 Oct 2023 16:51:08 +0700 Subject: [PATCH 11/17] feat(lint): Support local type aliases --- linting/src/storage_never_freed.rs | 26 +++++++-------- linting/ui/fail/storage_never_freed.stderr | 39 ++++++++++++++++++++-- 2 files changed, 49 insertions(+), 16 deletions(-) diff --git a/linting/src/storage_never_freed.rs b/linting/src/storage_never_freed.rs index a3ef7194183..5ee0158b6e3 100644 --- a/linting/src/storage_never_freed.rs +++ b/linting/src/storage_never_freed.rs @@ -167,21 +167,22 @@ fn find_collection_def_id( cx: &LateContext, path: &Path, ) -> Option<(DefId, CollectionTy)> { + if_chain! { + if let Res::Def(DefKind::TyAlias, def_id) = path.res; + if let Some(local_id) = def_id.as_local(); + if let Some(alias_ty) = cx.tcx.hir().get_by_def_id(local_id).alias_ty(); + if let TyKind::Path(QPath::Resolved(_, path)) = alias_ty.kind; + then { return find_collection_def_id(cx, path); } + }; if let Res::Def(DefKind::Struct, def_id) = path.res { if match_def_path(cx, def_id, &["alloc", "vec", "Vec"]) { - Some((def_id, CollectionTy::Vec)) - } else if match_def_path( - cx, - def_id, - &["ink_storage", "lazy", "mapping", "Mapping"], - ) { - Some((def_id, CollectionTy::Map)) - } else { - None + return Some((def_id, CollectionTy::Vec)) } - } else { - None - } + if match_def_path(cx, def_id, &["ink_storage", "lazy", "mapping", "Mapping"]) { + return Some((def_id, CollectionTy::Map)) + } + }; + None } /// Returns vectors of fields that have collection types @@ -202,7 +203,6 @@ fn find_collection_fields(cx: &LateContext, storage_struct_id: ItemId) -> Fields if let TyKind::Path(QPath::Resolved(None, path)) = ty.kind; then { let field_name = field_def.ident.name.as_str(); - // TODO: Inspect type aliases if let Some((_, ty)) = find_collection_def_id(cx, path) { result.insert(field_name.to_string(), FieldInfo::new(field_def.def_id, ty)); } diff --git a/linting/ui/fail/storage_never_freed.stderr b/linting/ui/fail/storage_never_freed.stderr index 64703287ff3..f2c2c0e3e2a 100644 --- a/linting/ui/fail/storage_never_freed.stderr +++ b/linting/ui/fail/storage_never_freed.stderr @@ -1,5 +1,5 @@ error: storage never freed - --> $DIR/storage_never_freed.rs:16:9 + --> $DIR/storage_never_freed.rs:17:9 | LL | map_1: Mapping, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -15,7 +15,7 @@ LL | map_1: Mapping, | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: storage never freed - --> $DIR/storage_never_freed.rs:17:9 + --> $DIR/storage_never_freed.rs:18:9 | LL | map_2: Mapping, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -25,6 +25,28 @@ help: consider adding operations to remove elements available to the user LL | map_2: Mapping, | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +error: storage never freed + --> $DIR/storage_never_freed.rs:19:9 + | +LL | map_3: Mapping, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider adding operations to remove elements available to the user + | +LL | map_3: Mapping, + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: storage never freed + --> $DIR/storage_never_freed.rs:20:9 + | +LL | map_alias: MapAlias2, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider adding operations to remove elements available to the user + | +LL | map_alias: MapAlias2, + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + error: storage never freed --> $DIR/storage_never_freed.rs:14:9 | @@ -39,6 +61,17 @@ LL | vec_1: Vec, error: storage never freed --> $DIR/storage_never_freed.rs:15:9 | +LL | vec_2: Vec, + | ^^^^^^^^^^^^^^^^ + | +help: consider adding operations to remove elements available to the user + | +LL | vec_2: Vec, + | ~~~~~~~~~~~~~~~~ + +error: storage never freed + --> $DIR/storage_never_freed.rs:16:9 + | LL | vec_subscription: Vec, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | @@ -47,5 +80,5 @@ help: consider adding operations to remove elements available to the user LL | vec_subscription: Vec, | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -error: aborting due to 4 previous errors +error: aborting due to 7 previous errors From 2b0e45bf9340b5f3a03a1e7f3d998efbd174ce90 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Wed, 4 Oct 2023 17:15:57 +0700 Subject: [PATCH 12/17] chore(lint): More accurate warning text --- linting/src/storage_never_freed.rs | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/linting/src/storage_never_freed.rs b/linting/src/storage_never_freed.rs index 5ee0158b6e3..6e777267419 100644 --- a/linting/src/storage_never_freed.rs +++ b/linting/src/storage_never_freed.rs @@ -17,14 +17,12 @@ use crate::ink_utils::{ find_storage_struct, }; use clippy_utils::{ - diagnostics::span_lint_and_then, + diagnostics::span_lint_and_help, is_lint_allowed, match_def_path, match_path, - source::snippet_opt, }; use if_chain::if_chain; -use rustc_errors::Applicability; use rustc_hir::{ self as hir, def::{ @@ -219,20 +217,13 @@ fn report_field(cx: &LateContext, field_info: &FieldInfo) { if let Node::Field(field) = cx.tcx.hir().get_by_def_id(field_info.did); if !is_lint_allowed(cx, STORAGE_NEVER_FREED, field.hir_id); then { - span_lint_and_then( + span_lint_and_help( cx, STORAGE_NEVER_FREED, field.span, - "storage never freed", - |diag| { - let snippet = snippet_opt(cx, field.span).expect("snippet must exist"); - diag.span_suggestion( - field.span, - "consider adding operations to remove elements available to the user".to_string(), - snippet, - Applicability::Unspecified, - ); - }, + "field's storage cannot be freed", + None, + "consider adding operations to remove elements available to the user" ) } From cf9c16aa6618c77321f651397d5941b69bade3d0 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Wed, 4 Oct 2023 17:18:18 +0700 Subject: [PATCH 13/17] chore(lint): Refactor --- linting/src/storage_never_freed.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/linting/src/storage_never_freed.rs b/linting/src/storage_never_freed.rs index 6e777267419..443413cfd8f 100644 --- a/linting/src/storage_never_freed.rs +++ b/linting/src/storage_never_freed.rs @@ -142,7 +142,6 @@ enum CollectionTy { struct FieldInfo { pub did: LocalDefId, pub ty: CollectionTy, - // TODO: replace w/ ids pub has_insert: bool, pub has_remove: bool, } @@ -330,7 +329,7 @@ impl<'tcx> LateLintPass<'tcx> for StorageNeverFreed { walk_body(&mut visitor, cx.tcx.hir().body(fn_body_id)); } }); - fields.iter().for_each(|(_, field)| { + fields.values().for_each(|field| { if field.has_insert && !field.has_remove { report_field(cx, field) } From 4af4e209d00a5c7bb2e1546309d0d1e02cd59cde Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Wed, 4 Oct 2023 17:44:34 +0700 Subject: [PATCH 14/17] feat(lint): Additional check for `self` --- linting/src/storage_never_freed.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/linting/src/storage_never_freed.rs b/linting/src/storage_never_freed.rs index 443413cfd8f..4982fb9e748 100644 --- a/linting/src/storage_never_freed.rs +++ b/linting/src/storage_never_freed.rs @@ -245,9 +245,8 @@ impl<'a, 'b, 'tcx> InsertRemoveCollector<'a, 'b, 'tcx> { fn find_field_name(&self, e: &Expr) -> Option { if_chain! { if let ExprKind::Field(s, field) = &e.kind; - if let ExprKind::Path(ref path) = s.kind; - let ty = self.cx.qpath_res(path, s.hir_id); - // TODO: check if ty is `self` + if let ExprKind::Path(QPath::Resolved(_, path)) = s.kind; + if match_path(path, &["self"]); then { Some(field.name.as_str().to_string()) } else { None } } } From 2d5642d1f8611b473267903425186305cfd66616 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Wed, 4 Oct 2023 17:47:22 +0700 Subject: [PATCH 15/17] chore(lint+tests): Refactor; update tests output --- linting/src/storage_never_freed.rs | 22 +++++----- linting/ui/fail/storage_never_freed.stderr | 49 +++++++--------------- linting/ui/pass/storage_never_freed.rs | 24 +++++------ 3 files changed, 37 insertions(+), 58 deletions(-) diff --git a/linting/src/storage_never_freed.rs b/linting/src/storage_never_freed.rs index 4982fb9e748..6d09769b5f1 100644 --- a/linting/src/storage_never_freed.rs +++ b/linting/src/storage_never_freed.rs @@ -63,11 +63,11 @@ use std::collections::{ declare_lint! { /// **What it does:** - /// This lint ensures that for every storage field with a collection type that supports adding - /// new elements, there's also an operation for removing elements. + /// This lint ensures that for every storage field with a collection type, when there is an + /// operation to insert new elements, there's also an operation for removing elements. /// /// **Why is this bad?** - /// When a user executes a contract function that writes to storage, the user has to put a + /// When a user executes a contract function that writes to storage, they have to put a /// deposit down for the amount of storage space used. Whoever frees up that storage at some /// later point gets the deposit back. Therefore, it is always a good idea to make it possible /// for users to free up their storage space. @@ -76,6 +76,7 @@ declare_lint! { /// /// In the following example there is a storage field with the `Mapping` type that has an /// function that inserts new elements: + /// /// ```rust /// #[ink(storage)] /// pub struct Transaction { @@ -230,14 +231,13 @@ fn report_field(cx: &LateContext, field_info: &FieldInfo) { } /// Visitor that collects `insert` and `remove` operations -struct InsertRemoveCollector<'a, 'b, 'tcx> { - cx: &'tcx LateContext<'a>, - fields: &'b mut FieldsMap, +struct InsertRemoveCollector<'a> { + fields: &'a mut FieldsMap, } -impl<'a, 'b, 'tcx> InsertRemoveCollector<'a, 'b, 'tcx> { - fn new(cx: &'tcx LateContext<'a>, fields: &'b mut FieldsMap) -> Self { - Self { cx, fields } +impl<'a> InsertRemoveCollector<'a> { + fn new(fields: &'a mut FieldsMap) -> Self { + Self { fields } } /// Finds a field of the supported type in the given expression present with the form @@ -252,7 +252,7 @@ impl<'a, 'b, 'tcx> InsertRemoveCollector<'a, 'b, 'tcx> { } } -impl<'hir> Visitor<'hir> for InsertRemoveCollector<'_, '_, '_> { +impl<'hir> Visitor<'hir> for InsertRemoveCollector<'_> { fn visit_expr(&mut self, e: &'hir Expr<'hir>) { match &e.kind { ExprKind::Assign(lhs, ..) => { @@ -324,7 +324,7 @@ impl<'tcx> LateLintPass<'tcx> for StorageNeverFreed { contract_impl.items.iter().for_each(|impl_item| { let impl_item = cx.tcx.hir().impl_item(impl_item.id); if let ImplItemKind::Fn(_, fn_body_id) = impl_item.kind { - let mut visitor = InsertRemoveCollector::new(cx, &mut fields); + let mut visitor = InsertRemoveCollector::new(&mut fields); walk_body(&mut visitor, cx.tcx.hir().body(fn_body_id)); } }); diff --git a/linting/ui/fail/storage_never_freed.stderr b/linting/ui/fail/storage_never_freed.stderr index f2c2c0e3e2a..33dc0b438d1 100644 --- a/linting/ui/fail/storage_never_freed.stderr +++ b/linting/ui/fail/storage_never_freed.stderr @@ -1,84 +1,63 @@ -error: storage never freed +error: field's storage cannot be freed --> $DIR/storage_never_freed.rs:17:9 | LL | map_1: Mapping, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | + = help: consider adding operations to remove elements available to the user note: the lint level is defined here --> $DIR/storage_never_freed.rs:1:46 | LL | #![cfg_attr(dylint_lib = "ink_linting", deny(storage_never_freed))] | ^^^^^^^^^^^^^^^^^^^ -help: consider adding operations to remove elements available to the user - | -LL | map_1: Mapping, - | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -error: storage never freed +error: field's storage cannot be freed --> $DIR/storage_never_freed.rs:18:9 | LL | map_2: Mapping, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: consider adding operations to remove elements available to the user - | -LL | map_2: Mapping, - | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + = help: consider adding operations to remove elements available to the user -error: storage never freed +error: field's storage cannot be freed --> $DIR/storage_never_freed.rs:19:9 | LL | map_3: Mapping, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: consider adding operations to remove elements available to the user - | -LL | map_3: Mapping, - | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + = help: consider adding operations to remove elements available to the user -error: storage never freed +error: field's storage cannot be freed --> $DIR/storage_never_freed.rs:20:9 | LL | map_alias: MapAlias2, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: consider adding operations to remove elements available to the user - | -LL | map_alias: MapAlias2, - | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + = help: consider adding operations to remove elements available to the user -error: storage never freed +error: field's storage cannot be freed --> $DIR/storage_never_freed.rs:14:9 | LL | vec_1: Vec, | ^^^^^^^^^^^^^^^^^^^^^ | -help: consider adding operations to remove elements available to the user - | -LL | vec_1: Vec, - | ~~~~~~~~~~~~~~~~~~~~~ + = help: consider adding operations to remove elements available to the user -error: storage never freed +error: field's storage cannot be freed --> $DIR/storage_never_freed.rs:15:9 | LL | vec_2: Vec, | ^^^^^^^^^^^^^^^^ | -help: consider adding operations to remove elements available to the user - | -LL | vec_2: Vec, - | ~~~~~~~~~~~~~~~~ + = help: consider adding operations to remove elements available to the user -error: storage never freed +error: field's storage cannot be freed --> $DIR/storage_never_freed.rs:16:9 | LL | vec_subscription: Vec, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: consider adding operations to remove elements available to the user - | -LL | vec_subscription: Vec, - | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + = help: consider adding operations to remove elements available to the user error: aborting due to 7 previous errors diff --git a/linting/ui/pass/storage_never_freed.rs b/linting/ui/pass/storage_never_freed.rs index 77d2c274751..aa14f2fd873 100644 --- a/linting/ui/pass/storage_never_freed.rs +++ b/linting/ui/pass/storage_never_freed.rs @@ -8,9 +8,9 @@ pub mod storage_never_freed { #[ink(storage)] pub struct StorageNeverFreed { - vec_field: Vec, - map_field: Mapping, - map_field2: MapAlias, + vec_1: Vec, + map_1: Mapping, + map_2: MapAlias, #[cfg_attr(dylint_lib = "ink_linting", allow(storage_never_freed))] map_field_suppressed: Mapping, @@ -23,9 +23,9 @@ pub mod storage_never_freed { #[ink(constructor)] pub fn new() -> Self { Self { - vec_field: Vec::new(), - map_field: Mapping::new(), - map_field2: Mapping::new(), + vec_1: Vec::new(), + map_1: Mapping::new(), + map_2: Mapping::new(), map_field_suppressed: Mapping::new(), vec_field_mut_pointer: Vec::new(), } @@ -33,9 +33,9 @@ pub mod storage_never_freed { #[ink(message)] pub fn add_to_fields(&mut self, v: AccountId) { - self.vec_field.push(v); - self.map_field.insert(v, &v); - self.map_field2.insert(v, &v); + self.vec_1.push(v); + self.map_1.insert(v, &v); + self.map_2.insert(v, &v); self.map_field_suppressed.insert(v, &v); // Should not be reported, since elements may be removed using the pointer @@ -50,9 +50,9 @@ pub mod storage_never_freed { #[ink(message)] pub fn remove_from_fields(&mut self, v: AccountId) { - self.vec_field.pop(); - self.map_field.remove(v); - self.map_field2.remove(v); + self.vec_1.pop(); + self.map_1.remove(v); + self.map_2.remove(v); } } } From 470f2bdc418d025df15235cf17110b00faef5416 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Wed, 4 Oct 2023 18:26:54 +0700 Subject: [PATCH 16/17] chore: Update changelog --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a8b00c9cdbe..ebf96986dc6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,10 +6,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -## Changed +### Changed - Make `set_code_hash` generic - [#1906](https://github.com/paritytech/ink/pull/1906) - Clean E2E configuration parsing - [#1922](https://github.com/paritytech/ink/pull/1922) +### Added +- Linter: `storage_never_freed` lint - [#1932](https://github.com/paritytech/ink/pull/1932) + ## Version 5.0.0-alpha The preview release of the ink! 5.0.0 release. From 1a5a1f80d0fd4e59f0c9bb0495d33a0e22478266 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Wed, 4 Oct 2023 18:40:12 +0700 Subject: [PATCH 17/17] chore: fmt --- linting/src/lib.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/linting/src/lib.rs b/linting/src/lib.rs index 2004485dd24..91c0dd640c7 100644 --- a/linting/src/lib.rs +++ b/linting/src/lib.rs @@ -28,9 +28,9 @@ extern crate rustc_middle; extern crate rustc_session; extern crate rustc_span; +mod ink_utils; mod primitive_topic; mod storage_never_freed; -mod ink_utils; #[doc(hidden)] #[no_mangle] @@ -38,7 +38,10 @@ pub fn register_lints( _sess: &rustc_session::Session, lint_store: &mut rustc_lint::LintStore, ) { - lint_store.register_lints(&[primitive_topic::PRIMITIVE_TOPIC, storage_never_freed::STORAGE_NEVER_FREED]); + lint_store.register_lints(&[ + primitive_topic::PRIMITIVE_TOPIC, + storage_never_freed::STORAGE_NEVER_FREED, + ]); lint_store.register_late_pass(|_| Box::new(primitive_topic::PrimitiveTopic)); lint_store.register_late_pass(|_| Box::new(storage_never_freed::StorageNeverFreed)); }