From f47cda650530643e9ca648299f4ff8d9aa594297 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Mon, 1 Jan 2024 11:13:48 -0300 Subject: [PATCH 01/11] feat: Lint skeleton and tests for #2017 --- linting/Cargo.toml | 6 ++ linting/src/lib.rs | 2 + linting/src/non_fallible_api.rs | 118 ++++++++++++++++++++++++++++ linting/ui/fail/non_fallible_api.rs | 44 +++++++++++ linting/ui/pass/non_fallible_api.rs | 54 +++++++++++++ 5 files changed, 224 insertions(+) create mode 100644 linting/src/non_fallible_api.rs create mode 100644 linting/ui/fail/non_fallible_api.rs create mode 100644 linting/ui/pass/non_fallible_api.rs diff --git a/linting/Cargo.toml b/linting/Cargo.toml index f622e319e9..568ede8edf 100644 --- a/linting/Cargo.toml +++ b/linting/Cargo.toml @@ -66,6 +66,12 @@ path = "ui/fail/strict_balance_equality.rs" [[example]] name = "no_main_pass" path = "ui/pass/no_main.rs" +[example]] +ame = "non_fallible_api" +ath = "ui/pass/non_fallible_api.rs" +[[example]] +name = "non_fallible_api" +path = "ui/fail/non_fallible_api.rs" [package.metadata.rust-analyzer] rustc_private = true diff --git a/linting/src/lib.rs b/linting/src/lib.rs index e9b7673e1b..3876f4161f 100644 --- a/linting/src/lib.rs +++ b/linting/src/lib.rs @@ -36,6 +36,7 @@ mod no_main; mod primitive_topic; mod storage_never_freed; mod strict_balance_equality; +mod non_fallible_api; #[doc(hidden)] #[no_mangle] @@ -54,6 +55,7 @@ pub fn register_lints( lint_store .register_late_pass(|_| Box::new(strict_balance_equality::StrictBalanceEquality)); lint_store.register_early_pass(|| Box::new(no_main::NoMain)); + lint_store.register_late_pass(|_| Box::new(non_fallible_api::NonFallibleAPI)); } #[test] diff --git a/linting/src/non_fallible_api.rs b/linting/src/non_fallible_api.rs new file mode 100644 index 0000000000..acda3c2b48 --- /dev/null +++ b/linting/src/non_fallible_api.rs @@ -0,0 +1,118 @@ +// 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::{ + diagnostics::span_lint_and_then, + is_lint_allowed, + match_def_path, + source::snippet_opt, +}; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::{ + self, + def::{ + DefKind, + Res, + }, + def_id::DefId, + Arm, + AssocItemKind, + ExprKind, + Impl, + ImplItemKind, + ImplItemRef, + Item, + ItemKind, + Node, + PatKind, + QPath, +}; +use rustc_lint::{ + LateContext, + LateLintPass, +}; +use rustc_middle::ty::{ + self, + Ty, + TyKind, +}; +use rustc_session::{ + declare_lint, + declare_lint_pass, +}; + +declare_lint! { + /// ## What it does + /// + /// The lint detects potentially unsafe uses of methods for which there are safer alternatives. + /// + /// ## Why is this bad? + /// + /// In some standard collections in ink!, there are two types of implementations: non-fallible + /// (e.g. `get`) and fallible (e.g. `try_get`). Fallible alternatives are considered safer, + /// as they perform additional checks for incorrect input parameters and return `Result::Err` + /// when they are used improperly. On the other hand, non-fallible methods do not provide these + /// checks and will panic on incorrect input, placing the responsibility on the user to + /// implement these checks. + /// + /// For more context, see: [ink#1910](https://github.com/paritytech/ink/pull/1910). + /// + /// ## Example + /// + /// Consider the contract that has the following `Mapping` field: + /// + /// ```rust + /// #[ink(storage)] + /// pub struct Example { map: Mapping } + /// ``` + /// + /// The following usage of the non-fallible API is unsafe: + /// + /// ```rust + /// pub fn test(&mut self, a: String, b: AccountId) { + /// // Bad: can panic on incorrect input + /// self.map.insert(a, &b); + /// } + /// ``` + /// + /// It could be replaced with the fallible version of `Mapping::insert`: + /// + /// ```rust + /// pub fn test(&mut self, a: String, b: AccountId) { + /// // Good: returns Result::Err on incorrect input + /// self.map.try_insert(a, &b)?; + /// } + /// ``` + /// + /// Otherwise, the user could explicitly check the encoded size of the argument in their code: + /// + /// ```rust + /// pub fn test(&mut self, a: String, b: AccountId) { + /// // Good: explicitly checked encoded size of the input + /// if String::encoded_size(&a) < ink_env::BUFFER_SIZE { + /// self.map.insert(a, &b); + /// } + /// } + /// ``` + pub NON_FALLIBLE_API, + Warn, + "using non-fallible API" +} + +declare_lint_pass!(NonFallibleAPI => [NON_FALLIBLE_API]); + +impl<'tcx> LateLintPass<'tcx> for NonFallibleAPI { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {} +} diff --git a/linting/ui/fail/non_fallible_api.rs b/linting/ui/fail/non_fallible_api.rs new file mode 100644 index 0000000000..d56975294f --- /dev/null +++ b/linting/ui/fail/non_fallible_api.rs @@ -0,0 +1,44 @@ +#![cfg_attr(not(feature = "std"), no_std, no_main)] + +#[ink::contract] +pub mod non_fallible_api { + use ink::{ + prelude::string::String, + storage::{ + Lazy, + Mapping, + }, + }; + + #[ink(storage)] + pub struct NonFallibleAPI { + map_1: Mapping, + lazy_1: Lazy, + } + + impl NonFallibleAPI { + #[ink(constructor)] + pub fn new() -> Self { + Self { + map_1: Mapping::new(), + lazy_1: Lazy::new(), + } + } + + // Raise warnings when using non-fallible API with argument which encoded size is + // statically unknown. + #[ink(message)] + pub fn non_fallible_not_statically_known(&mut self, a: String, b: String) { + // Mapping + let _ = self.map_1.insert(a.clone(), &b); + let _ = self.map_1.get(a.clone()); + let _ = self.map_1.take(a.clone()); + + // Lazy + let _ = self.lazy_1.get(); + self.lazy_1.set(&a); + } + } +} + +fn main() {} diff --git a/linting/ui/pass/non_fallible_api.rs b/linting/ui/pass/non_fallible_api.rs new file mode 100644 index 0000000000..0e483c8095 --- /dev/null +++ b/linting/ui/pass/non_fallible_api.rs @@ -0,0 +1,54 @@ +#![cfg_attr(not(feature = "std"), no_std, no_main)] + +#[ink::contract] +pub mod non_fallible_api { + use ink::storage::{ + Lazy, + Mapping, + }; + + #[ink(storage)] + pub struct NonFallibleAPI { + map_1: Mapping, + lazy_1: Lazy, + } + + impl NonFallibleAPI { + #[ink(constructor)] + pub fn new() -> Self { + Self { + map_1: Mapping::new(), + lazy_1: Lazy::new(), + } + } + + // Don't generate warnings when using the fallible API + #[ink(message)] + pub fn fallible(&mut self, a: AccountId, b: AccountId) { + // Mapping + let _ = self.map_1.try_insert(a, &b); + let _ = self.map_1.try_get(a); + let _ = self.map_1.try_take(a); + + // Lazy + let _ = self.lazy_1.try_get(); + let _ = self.lazy_1.try_set(&a); + } + + // Don't raise warnings when using non-fallible API with argument which encoded + // size is statically known. + #[ink(message)] + pub fn non_fallible_statically_known(&mut self, a: AccountId, b: AccountId) { + // Mapping + let _ = self.map_1.insert(a, &b); + let _ = self.map_1.get(a); + let _ = self.map_1.take(a); + + // Lazy + let _ = self.lazy_1.get(); + self.lazy_1.set(&a); + } + } +} + +fn main() {} From a1b1bf3acfb375b5a18df4447b9b6fda34fb529f Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Wed, 3 Jan 2024 14:02:13 -0300 Subject: [PATCH 02/11] feat: Basic checks for primitive types only --- linting/Cargo.toml | 8 +- linting/src/lib.rs | 4 +- linting/src/non_fallible_api.rs | 202 +++++++++++++++++++++--- linting/ui/fail/non_fallible_api.stderr | 34 ++++ 4 files changed, 223 insertions(+), 25 deletions(-) create mode 100644 linting/ui/fail/non_fallible_api.stderr diff --git a/linting/Cargo.toml b/linting/Cargo.toml index 568ede8edf..b3270e4d9d 100644 --- a/linting/Cargo.toml +++ b/linting/Cargo.toml @@ -66,11 +66,11 @@ path = "ui/fail/strict_balance_equality.rs" [[example]] name = "no_main_pass" path = "ui/pass/no_main.rs" -[example]] -ame = "non_fallible_api" -ath = "ui/pass/non_fallible_api.rs" [[example]] -name = "non_fallible_api" +name = "non_fallible_api_pass" +path = "ui/pass/non_fallible_api.rs" +[[example]] +name = "non_fallible_api_fail" path = "ui/fail/non_fallible_api.rs" [package.metadata.rust-analyzer] diff --git a/linting/src/lib.rs b/linting/src/lib.rs index 3876f4161f..653b3ae654 100644 --- a/linting/src/lib.rs +++ b/linting/src/lib.rs @@ -25,18 +25,20 @@ extern crate rustc_ast; extern crate rustc_errors; extern crate rustc_hir; extern crate rustc_index; +extern crate rustc_infer; extern crate rustc_lint; extern crate rustc_middle; extern crate rustc_mir_dataflow; extern crate rustc_session; extern crate rustc_span; +extern crate rustc_type_ir; mod ink_utils; mod no_main; +mod non_fallible_api; mod primitive_topic; mod storage_never_freed; mod strict_balance_equality; -mod non_fallible_api; #[doc(hidden)] #[no_mangle] diff --git a/linting/src/non_fallible_api.rs b/linting/src/non_fallible_api.rs index acda3c2b48..21cf8840d0 100644 --- a/linting/src/non_fallible_api.rs +++ b/linting/src/non_fallible_api.rs @@ -12,46 +12,49 @@ // See the License for the specific language governing permissions and // limitations under the License. +use crate::ink_utils::{ + expand_unnamed_consts, + find_contract_impl_id, +}; use clippy_utils::{ diagnostics::span_lint_and_then, - is_lint_allowed, match_def_path, - source::snippet_opt, }; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::{ - self, - def::{ - DefKind, - Res, - }, + self as hir, def_id::DefId, - Arm, - AssocItemKind, + intravisit::{ + walk_body, + walk_expr, + Visitor, + }, + Body, + Expr, ExprKind, - Impl, ImplItemKind, - ImplItemRef, - Item, ItemKind, - Node, - PatKind, - QPath, + PathSegment, }; +use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::{ LateContext, LateLintPass, }; -use rustc_middle::ty::{ - self, - Ty, - TyKind, +use rustc_middle::{ + hir::nested_filter, + ty::{ + self, + Ty, + TypeckResults, + }, }; use rustc_session::{ declare_lint, declare_lint_pass, }; +use rustc_type_ir::sty::TyKind; declare_lint! { /// ## What it does @@ -113,6 +116,165 @@ declare_lint! { declare_lint_pass!(NonFallibleAPI => [NON_FALLIBLE_API]); +#[derive(Debug)] +enum TyToCheck { + Mapping, + Lazy, +} + +impl TyToCheck { + pub fn try_from_adt(cx: &LateContext<'_>, did: DefId) -> Option { + if match_def_path(cx, did, &["ink_storage", "lazy", "Lazy"]) { + return Some(Self::Lazy) + } + + if match_def_path(cx, did, &["ink_storage", "lazy", "mapping", "Mapping"]) { + return Some(Self::Mapping) + } + None + } + + pub fn find_fallible_alternative(&self, method_name: &str) -> Option { + use TyToCheck::*; + match self { + Mapping { .. } => { + match method_name { + "insert" => Some("try_insert".to_string()), + "get" => Some("try_get".to_string()), + "take" => Some("try_take".to_string()), + _ => None, + } + } + Lazy { .. } => { + match method_name { + "get" => Some("try_get".to_string()), + "set" => Some("try_set".to_string()), + _ => None, + } + } + } + } +} + +/// Visitor that finds usage of non-fallible calls in the bodies of functions +struct APIUsageChecker<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + maybe_typeck_results: Option<&'tcx TypeckResults<'tcx>>, +} + +impl<'a, 'tcx> APIUsageChecker<'a, 'tcx> { + pub fn new(cx: &'a LateContext<'tcx>) -> Self { + Self { + cx, + maybe_typeck_results: cx.maybe_typeck_results(), + } + } + + /// Returns true iff type is a primitive, so its encoded size is statically known + fn is_primitive_ty(ty: &Ty) -> bool { + matches!( + ty.kind(), + ty::Int(_) | ty::Uint(_) | ty::Bool | ty::Char | ty::Float(_) + ) + } + + /// Raises warnings if the given method call is potentially unsafe and could be + /// replaced + fn check_method_call( + &self, + receiver_ty: &TyToCheck, + method_path: &PathSegment, + method_name: &str, + arg_ty: &Ty<'tcx>, + ) { + if_chain! { + if !Self::is_primitive_ty(arg_ty); + if let Some(fallible_method) = receiver_ty.find_fallible_alternative(method_name); + then { + span_lint_and_then( + self.cx, + NON_FALLIBLE_API, + method_path.ident.span, + format!( + "using a non-fallible `{:?}::{}` with an argument that may not fit into the static buffer", + receiver_ty, + method_name, + ).as_str(), + |diag| { + diag.span_suggestion( + method_path.ident.span, + format!("consider using `{}`", fallible_method), + "", + Applicability::Unspecified, + ); + }, + ) + } + } + } +} + +impl<'a, 'tcx> Visitor<'tcx> for APIUsageChecker<'a, 'tcx> { + type NestedFilter = nested_filter::OnlyBodies; + + fn visit_expr(&mut self, e: &'tcx Expr<'tcx>) { + if_chain! { + if let ExprKind::MethodCall(method_path, receiver, actual_args, _) = &e.kind; + if let Some(typeck_results) = self.maybe_typeck_results; + let ty = typeck_results.expr_ty(receiver); + if let TyKind::Adt(def, substs) = ty.kind(); + if let Some(ty) = TyToCheck::try_from_adt(self.cx, def.0.0.did); + then { + substs + .iter() + .take(substs.len() - 1) + .filter_map(|subst| subst.as_type()) + .for_each(|arg_ty| { + self.check_method_call( + &ty, + method_path, + &method_path.ident.to_string(), + &arg_ty) + }) + } + } + walk_expr(self, e); + } + + fn visit_body(&mut self, body: &'tcx Body<'_>) { + let old_maybe_typeck_results = self + .maybe_typeck_results + .replace(self.cx.tcx.typeck_body(body.id())); + walk_body(self, body); + self.maybe_typeck_results = old_maybe_typeck_results; + } + + fn nested_visit_map(&mut self) -> Self::Map { + self.cx.tcx.hir() + } +} + impl<'tcx> LateLintPass<'tcx> for NonFallibleAPI { - fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {} + fn check_mod( + &mut self, + cx: &LateContext<'tcx>, + m: &'tcx hir::Mod<'tcx>, + _: hir::HirId, + ) { + if_chain! { + 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| { + let impl_item = cx.tcx.hir().impl_item(impl_item.id); + if let ImplItemKind::Fn(..) = impl_item.kind { + let mut visitor = APIUsageChecker::new(cx); + visitor.visit_impl_item(impl_item); + } + }) + } + } + } } diff --git a/linting/ui/fail/non_fallible_api.stderr b/linting/ui/fail/non_fallible_api.stderr new file mode 100644 index 0000000000..254bf68ae0 --- /dev/null +++ b/linting/ui/fail/non_fallible_api.stderr @@ -0,0 +1,34 @@ +error: using a non-fallible `Mapping::insert` with an argument that may not fit into the static buffer + --> $DIR/non_fallible_api.rs:33:32 + | +LL | let _ = self.map_1.insert(a.clone(), &b); + | ^^^^^^ help: consider using `try_insert` + | + = note: `-D non-fallible-api` implied by `-D warnings` + +error: using a non-fallible `Mapping::get` with an argument that may not fit into the static buffer + --> $DIR/non_fallible_api.rs:34:32 + | +LL | let _ = self.map_1.get(a.clone()); + | ^^^ help: consider using `try_get` + +error: using a non-fallible `Mapping::take` with an argument that may not fit into the static buffer + --> $DIR/non_fallible_api.rs:35:32 + | +LL | let _ = self.map_1.take(a.clone()); + | ^^^^ help: consider using `try_take` + +error: using a non-fallible `Lazy::get` with an argument that may not fit into the static buffer + --> $DIR/non_fallible_api.rs:38:33 + | +LL | let _ = self.lazy_1.get(); + | ^^^ help: consider using `try_get` + +error: using a non-fallible `Lazy::set` with an argument that may not fit into the static buffer + --> $DIR/non_fallible_api.rs:39:25 + | +LL | self.lazy_1.set(&a); + | ^^^ help: consider using `try_set` + +error: aborting due to 5 previous errors + From 7cf66038da8c62bd708ece3ced5444f49106951a Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Fri, 5 Jan 2024 10:12:18 -0300 Subject: [PATCH 03/11] feat: Recursive checks if nested types have statically known encoded size Improve the heuristcs to check if nested types in ADTs, tuples and arrays have statically encoded types, and therefore should not be reported. This removes false positives on the default ink types like `AccountId` (alias to `[u8; 32]`). --- linting/extra/Cargo.toml | 3 +- linting/extra/src/lib.rs | 1 - linting/extra/src/non_fallible_api.rs | 45 +++++++++++++++++------ linting/extra/ui/pass/non_fallible_api.rs | 6 +++ 4 files changed, 42 insertions(+), 13 deletions(-) diff --git a/linting/extra/Cargo.toml b/linting/extra/Cargo.toml index 51f5ba6caf..3fff7174a2 100644 --- a/linting/extra/Cargo.toml +++ b/linting/extra/Cargo.toml @@ -23,11 +23,12 @@ dylint_linting = "2.1.12" if_chain = "1.0.2" log = "0.4.14" regex = "1.5.4" +# ink! dependencies used in the linter implementation +ink_env = { path = "../../crates/env", default-features = false } [dev-dependencies] dylint_testing = "2.1.12" # The following are ink! dependencies, they are only required for the `ui` tests. -ink_env = { path = "../../crates/env", default-features = false } ink = { path = "../../crates/ink", default-features = false, features = ["std"] } ink_metadata = { path = "../../crates/metadata", default-features = false } ink_primitives = { path = "../../crates/primitives", default-features = false } diff --git a/linting/extra/src/lib.rs b/linting/extra/src/lib.rs index 646e698d8e..12b7fca07c 100644 --- a/linting/extra/src/lib.rs +++ b/linting/extra/src/lib.rs @@ -25,7 +25,6 @@ extern crate rustc_ast; extern crate rustc_errors; extern crate rustc_hir; extern crate rustc_index; -extern crate rustc_infer; extern crate rustc_lint; extern crate rustc_middle; extern crate rustc_mir_dataflow; diff --git a/linting/extra/src/non_fallible_api.rs b/linting/extra/src/non_fallible_api.rs index 21cf8840d0..ef5bb378cd 100644 --- a/linting/extra/src/non_fallible_api.rs +++ b/linting/extra/src/non_fallible_api.rs @@ -37,7 +37,6 @@ use rustc_hir::{ ItemKind, PathSegment, }; -use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::{ LateContext, LateLintPass, @@ -46,6 +45,7 @@ use rustc_middle::{ hir::nested_filter, ty::{ self, + ConstKind, Ty, TypeckResults, }, @@ -170,12 +170,35 @@ impl<'a, 'tcx> APIUsageChecker<'a, 'tcx> { } } - /// Returns true iff type is a primitive, so its encoded size is statically known - fn is_primitive_ty(ty: &Ty) -> bool { - matches!( - ty.kind(), - ty::Int(_) | ty::Uint(_) | ty::Bool | ty::Char | ty::Float(_) - ) + /// Returns true iff the given type has statically known size when encoded with + /// `scale_codec` + fn is_statically_known(&self, ty: &Ty<'tcx>) -> bool { + match ty.kind() { + ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Str => { + true + } + ty::Tuple(inner_tys) => { + inner_tys.iter().all(|ty| self.is_statically_known(&ty)) + } + ty::Ref(_, inner, _) => self.is_statically_known(inner), + ty::Adt(adt_def, substs) => { + adt_def.variants().iter().all(|variant| { + variant.fields.iter().all(|field| { + self.is_statically_known(&field.ty(self.cx.tcx, substs)) + }) + }) + } + ty::Array(inner_ty, len_const) => { + if_chain! { + if self.is_statically_known(inner_ty); + if let ConstKind::Value(ty::ValTree::Leaf(elements_count)) = len_const.kind(); + if let Ok(elements_size) = elements_count.try_to_target_usize(self.cx.tcx); + if elements_size < (ink_env::BUFFER_SIZE as u64); + then { true } else { false } + } + } + _ => false, + } } /// Raises warnings if the given method call is potentially unsafe and could be @@ -185,10 +208,10 @@ impl<'a, 'tcx> APIUsageChecker<'a, 'tcx> { receiver_ty: &TyToCheck, method_path: &PathSegment, method_name: &str, - arg_ty: &Ty<'tcx>, + arg_ty: Ty<'tcx>, ) { if_chain! { - if !Self::is_primitive_ty(arg_ty); + if !self.is_statically_known(&arg_ty); if let Some(fallible_method) = receiver_ty.find_fallible_alternative(method_name); then { span_lint_and_then( @@ -219,7 +242,7 @@ impl<'a, 'tcx> Visitor<'tcx> for APIUsageChecker<'a, 'tcx> { fn visit_expr(&mut self, e: &'tcx Expr<'tcx>) { if_chain! { - if let ExprKind::MethodCall(method_path, receiver, actual_args, _) = &e.kind; + if let ExprKind::MethodCall(method_path, receiver, _, _) = &e.kind; if let Some(typeck_results) = self.maybe_typeck_results; let ty = typeck_results.expr_ty(receiver); if let TyKind::Adt(def, substs) = ty.kind(); @@ -234,7 +257,7 @@ impl<'a, 'tcx> Visitor<'tcx> for APIUsageChecker<'a, 'tcx> { &ty, method_path, &method_path.ident.to_string(), - &arg_ty) + arg_ty) }) } } diff --git a/linting/extra/ui/pass/non_fallible_api.rs b/linting/extra/ui/pass/non_fallible_api.rs index 0e483c8095..701450f9c1 100644 --- a/linting/extra/ui/pass/non_fallible_api.rs +++ b/linting/extra/ui/pass/non_fallible_api.rs @@ -10,6 +10,8 @@ pub mod non_fallible_api { #[ink(storage)] pub struct NonFallibleAPI { map_1: Mapping, + map_2: Mapping, + map_3: Mapping, lazy_1: Lazy, } @@ -18,6 +20,8 @@ pub mod non_fallible_api { pub fn new() -> Self { Self { map_1: Mapping::new(), + map_2: Mapping::new(), + map_3: Mapping::new(), lazy_1: Lazy::new(), } } @@ -43,6 +47,8 @@ pub mod non_fallible_api { let _ = self.map_1.insert(a, &b); let _ = self.map_1.get(a); let _ = self.map_1.take(a); + let _ = self.map_2.insert(a, &[b; 1]); + let _ = self.map_3.insert(a, &(b, b)); // Lazy let _ = self.lazy_1.get(); From 812527b7ab87af570650f8a26f69ec2966572f7f Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Fri, 5 Jan 2024 11:56:09 -0300 Subject: [PATCH 04/11] chore: Update CHANGELOG --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2121cad050..cbdcf51906 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added +- Linter: `non_fallible_api` lint - [#2004](https://github.com/paritytech/ink/pull/2004) ## Version 5.0.0-rc From 3b5c8199c2196cd352d233c01f62897f9e13e478 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Fri, 5 Jan 2024 12:06:24 -0300 Subject: [PATCH 05/11] feat(tests): Test more statically unknown types in the `fail` test --- linting/extra/ui/fail/non_fallible_api.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/linting/extra/ui/fail/non_fallible_api.rs b/linting/extra/ui/fail/non_fallible_api.rs index d56975294f..35cf109ac5 100644 --- a/linting/extra/ui/fail/non_fallible_api.rs +++ b/linting/extra/ui/fail/non_fallible_api.rs @@ -1,9 +1,16 @@ #![cfg_attr(not(feature = "std"), no_std, no_main)] +pub type TyAlias1 = ink::prelude::vec::Vec; +pub type TyAlias2 = TyAlias1; + #[ink::contract] pub mod non_fallible_api { + use crate::TyAlias2; use ink::{ - prelude::string::String, + prelude::{ + string::String, + vec::Vec, + }, storage::{ Lazy, Mapping, @@ -13,7 +20,9 @@ pub mod non_fallible_api { #[ink(storage)] pub struct NonFallibleAPI { map_1: Mapping, + map_2: Mapping, lazy_1: Lazy, + lazy_2: Lazy<(String, String)>, } impl NonFallibleAPI { @@ -21,7 +30,9 @@ pub mod non_fallible_api { pub fn new() -> Self { Self { map_1: Mapping::new(), + map_2: Mapping::new(), lazy_1: Lazy::new(), + lazy_2: Lazy::new(), } } @@ -33,10 +44,14 @@ pub mod non_fallible_api { let _ = self.map_1.insert(a.clone(), &b); let _ = self.map_1.get(a.clone()); let _ = self.map_1.take(a.clone()); + let mut v = Vec::new(); + v.push(42); + let _ = self.map_2.insert(42, &v); // Lazy let _ = self.lazy_1.get(); self.lazy_1.set(&a); + self.lazy_2.set(&(a.clone(), a)); } } } From e82c0cfd8d2639648adf2d224ffabc18fde49ba4 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Fri, 5 Jan 2024 12:08:57 -0300 Subject: [PATCH 06/11] feat(tests): Type aliases in `pass` --- linting/extra/ui/pass/non_fallible_api.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/linting/extra/ui/pass/non_fallible_api.rs b/linting/extra/ui/pass/non_fallible_api.rs index 701450f9c1..b466544833 100644 --- a/linting/extra/ui/pass/non_fallible_api.rs +++ b/linting/extra/ui/pass/non_fallible_api.rs @@ -1,7 +1,13 @@ #![cfg_attr(not(feature = "std"), no_std, no_main)] +use ink_primitives::AccountId; + +pub type TyAlias1 = AccountId; +pub type TyAlias2 = TyAlias1; + #[ink::contract] pub mod non_fallible_api { + use crate::TyAlias2; use ink::storage::{ Lazy, Mapping, @@ -13,6 +19,7 @@ pub mod non_fallible_api { map_2: Mapping, map_3: Mapping, lazy_1: Lazy, + lazy_2: Lazy, } impl NonFallibleAPI { @@ -23,6 +30,7 @@ pub mod non_fallible_api { map_2: Mapping::new(), map_3: Mapping::new(), lazy_1: Lazy::new(), + lazy_2: Lazy::new(), } } @@ -53,6 +61,8 @@ pub mod non_fallible_api { // Lazy let _ = self.lazy_1.get(); self.lazy_1.set(&a); + let _ = self.lazy_2.get(); + self.lazy_2.set(&a); } } } From 6169d30039d573af7321c727dfc70ee2c1edbe63 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Fri, 5 Jan 2024 12:17:57 -0300 Subject: [PATCH 07/11] fix: Lint suppressions --- linting/extra/src/lib.rs | 1 + linting/extra/src/non_fallible_api.rs | 2 ++ linting/extra/ui/pass/non_fallible_api.rs | 18 +++++++++++++++--- 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/linting/extra/src/lib.rs b/linting/extra/src/lib.rs index 12b7fca07c..00cc666d70 100644 --- a/linting/extra/src/lib.rs +++ b/linting/extra/src/lib.rs @@ -48,6 +48,7 @@ pub fn register_lints( primitive_topic::PRIMITIVE_TOPIC, storage_never_freed::STORAGE_NEVER_FREED, strict_balance_equality::STRICT_BALANCE_EQUALITY, + non_fallible_api::NON_FALLIBLE_API, ]); lint_store.register_late_pass(|_| Box::new(primitive_topic::PrimitiveTopic)); lint_store.register_late_pass(|_| Box::new(storage_never_freed::StorageNeverFreed)); diff --git a/linting/extra/src/non_fallible_api.rs b/linting/extra/src/non_fallible_api.rs index ef5bb378cd..4850655691 100644 --- a/linting/extra/src/non_fallible_api.rs +++ b/linting/extra/src/non_fallible_api.rs @@ -18,6 +18,7 @@ use crate::ink_utils::{ }; use clippy_utils::{ diagnostics::span_lint_and_then, + is_lint_allowed, match_def_path, }; use if_chain::if_chain; @@ -242,6 +243,7 @@ impl<'a, 'tcx> Visitor<'tcx> for APIUsageChecker<'a, 'tcx> { fn visit_expr(&mut self, e: &'tcx Expr<'tcx>) { if_chain! { + if !is_lint_allowed(self.cx, NON_FALLIBLE_API, e.hir_id); if let ExprKind::MethodCall(method_path, receiver, _, _) = &e.kind; if let Some(typeck_results) = self.maybe_typeck_results; let ty = typeck_results.expr_ty(receiver); diff --git a/linting/extra/ui/pass/non_fallible_api.rs b/linting/extra/ui/pass/non_fallible_api.rs index b466544833..b275c87b04 100644 --- a/linting/extra/ui/pass/non_fallible_api.rs +++ b/linting/extra/ui/pass/non_fallible_api.rs @@ -8,9 +8,12 @@ pub type TyAlias2 = TyAlias1; #[ink::contract] pub mod non_fallible_api { use crate::TyAlias2; - use ink::storage::{ - Lazy, - Mapping, + use ink::{ + prelude::string::String, + storage::{ + Lazy, + Mapping, + }, }; #[ink(storage)] @@ -20,6 +23,7 @@ pub mod non_fallible_api { map_3: Mapping, lazy_1: Lazy, lazy_2: Lazy, + lazy_3: Lazy, } impl NonFallibleAPI { @@ -31,6 +35,7 @@ pub mod non_fallible_api { map_3: Mapping::new(), lazy_1: Lazy::new(), lazy_2: Lazy::new(), + lazy_3: Lazy::new(), } } @@ -64,6 +69,13 @@ pub mod non_fallible_api { let _ = self.lazy_2.get(); self.lazy_2.set(&a); } + + // Check if local suppressions work + #[ink(message)] + pub fn suppressions(&mut self, a: String) { + #[cfg_attr(dylint_lib = "ink_linting", allow(non_fallible_api))] + self.lazy_3.set(&a); + } } } From 3dda9a6ac00d854ee37c38579ae8db4ae9c8bcdf Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Fri, 5 Jan 2024 12:18:37 -0300 Subject: [PATCH 08/11] chore: Add new stderr --- linting/extra/ui/fail/non_fallible_api.stderr | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/linting/extra/ui/fail/non_fallible_api.stderr b/linting/extra/ui/fail/non_fallible_api.stderr index 254bf68ae0..84ed53d429 100644 --- a/linting/extra/ui/fail/non_fallible_api.stderr +++ b/linting/extra/ui/fail/non_fallible_api.stderr @@ -1,5 +1,5 @@ error: using a non-fallible `Mapping::insert` with an argument that may not fit into the static buffer - --> $DIR/non_fallible_api.rs:33:32 + --> $DIR/non_fallible_api.rs:44:32 | LL | let _ = self.map_1.insert(a.clone(), &b); | ^^^^^^ help: consider using `try_insert` @@ -7,28 +7,40 @@ LL | let _ = self.map_1.insert(a.clone(), &b); = note: `-D non-fallible-api` implied by `-D warnings` error: using a non-fallible `Mapping::get` with an argument that may not fit into the static buffer - --> $DIR/non_fallible_api.rs:34:32 + --> $DIR/non_fallible_api.rs:45:32 | LL | let _ = self.map_1.get(a.clone()); | ^^^ help: consider using `try_get` error: using a non-fallible `Mapping::take` with an argument that may not fit into the static buffer - --> $DIR/non_fallible_api.rs:35:32 + --> $DIR/non_fallible_api.rs:46:32 | LL | let _ = self.map_1.take(a.clone()); | ^^^^ help: consider using `try_take` +error: using a non-fallible `Mapping::insert` with an argument that may not fit into the static buffer + --> $DIR/non_fallible_api.rs:49:32 + | +LL | let _ = self.map_2.insert(42, &v); + | ^^^^^^ help: consider using `try_insert` + error: using a non-fallible `Lazy::get` with an argument that may not fit into the static buffer - --> $DIR/non_fallible_api.rs:38:33 + --> $DIR/non_fallible_api.rs:52:33 | LL | let _ = self.lazy_1.get(); | ^^^ help: consider using `try_get` error: using a non-fallible `Lazy::set` with an argument that may not fit into the static buffer - --> $DIR/non_fallible_api.rs:39:25 + --> $DIR/non_fallible_api.rs:53:25 | LL | self.lazy_1.set(&a); | ^^^ help: consider using `try_set` -error: aborting due to 5 previous errors +error: using a non-fallible `Lazy::set` with an argument that may not fit into the static buffer + --> $DIR/non_fallible_api.rs:54:25 + | +LL | self.lazy_2.set(&(a.clone(), a)); + | ^^^ help: consider using `try_set` + +error: aborting due to 7 previous errors From b29cf273dd824e4ceed7fca5d25a2466ba79f39f Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Tue, 9 Jan 2024 06:54:29 -0300 Subject: [PATCH 09/11] chore: cleanup --- linting/extra/src/non_fallible_api.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/linting/extra/src/non_fallible_api.rs b/linting/extra/src/non_fallible_api.rs index 4850655691..d21b2974ae 100644 --- a/linting/extra/src/non_fallible_api.rs +++ b/linting/extra/src/non_fallible_api.rs @@ -138,7 +138,7 @@ impl TyToCheck { pub fn find_fallible_alternative(&self, method_name: &str) -> Option { use TyToCheck::*; match self { - Mapping { .. } => { + Mapping => { match method_name { "insert" => Some("try_insert".to_string()), "get" => Some("try_get".to_string()), @@ -146,7 +146,7 @@ impl TyToCheck { _ => None, } } - Lazy { .. } => { + Lazy => { match method_name { "get" => Some("try_get".to_string()), "set" => Some("try_set".to_string()), From 75921a4e2d90c07c0494584a95025800cc72a6fc Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Tue, 9 Jan 2024 07:37:52 -0300 Subject: [PATCH 10/11] chore(doc): Update examples --- linting/extra/src/non_fallible_api.rs | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/linting/extra/src/non_fallible_api.rs b/linting/extra/src/non_fallible_api.rs index d21b2974ae..d709ece227 100644 --- a/linting/extra/src/non_fallible_api.rs +++ b/linting/extra/src/non_fallible_api.rs @@ -85,29 +85,23 @@ declare_lint! { /// The following usage of the non-fallible API is unsafe: /// /// ```rust - /// pub fn test(&mut self, a: String, b: AccountId) { - /// // Bad: can panic on incorrect input - /// self.map.insert(a, &b); - /// } + /// // Bad: can panic if `input_string` doesn't fit into the static buffer + /// self.map.insert(input_string, &self.sender); /// ``` /// /// It could be replaced with the fallible version of `Mapping::insert`: /// /// ```rust - /// pub fn test(&mut self, a: String, b: AccountId) { - /// // Good: returns Result::Err on incorrect input - /// self.map.try_insert(a, &b)?; - /// } + /// // Good: returns Result::Err on incorrect input + /// self.map.try_insert(input_string, &self.sender); /// ``` /// /// Otherwise, the user could explicitly check the encoded size of the argument in their code: /// /// ```rust - /// pub fn test(&mut self, a: String, b: AccountId) { - /// // Good: explicitly checked encoded size of the input - /// if String::encoded_size(&a) < ink_env::BUFFER_SIZE { - /// self.map.insert(a, &b); - /// } + /// // Good: explicitly checked encoded size of the input + /// if String::encoded_size(&input_string) < ink_env::BUFFER_SIZE { + /// self.map.insert(input_string, &self.sender); /// } /// ``` pub NON_FALLIBLE_API, From 2abfbd456887e694089bc04c8ed6590ca623f21b Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Wed, 10 Jan 2024 11:50:33 -0300 Subject: [PATCH 11/11] trigger GitHub actions