-
Notifications
You must be signed in to change notification settings - Fork 426
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
…2045) * feat: Lint skeleton and tests for #2017 * feat: Basic checks for primitive types only * 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]`). * chore: Update CHANGELOG * feat(tests): Test more statically unknown types in the `fail` test * feat(tests): Type aliases in `pass` * fix: Lint suppressions * chore: Add new stderr * chore: cleanup * chore(doc): Update examples * trigger GitHub actions
- Loading branch information
Showing
7 changed files
with
501 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,299 @@ | ||
| // 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 crate::ink_utils::{ | ||
| expand_unnamed_consts, | ||
| find_contract_impl_id, | ||
| }; | ||
| use clippy_utils::{ | ||
| diagnostics::span_lint_and_then, | ||
| is_lint_allowed, | ||
| match_def_path, | ||
| }; | ||
| use if_chain::if_chain; | ||
| use rustc_errors::Applicability; | ||
| use rustc_hir::{ | ||
| self as hir, | ||
| def_id::DefId, | ||
| intravisit::{ | ||
| walk_body, | ||
| walk_expr, | ||
| Visitor, | ||
| }, | ||
| Body, | ||
| Expr, | ||
| ExprKind, | ||
| ImplItemKind, | ||
| ItemKind, | ||
| PathSegment, | ||
| }; | ||
| use rustc_lint::{ | ||
| LateContext, | ||
| LateLintPass, | ||
| }; | ||
| use rustc_middle::{ | ||
| hir::nested_filter, | ||
| ty::{ | ||
| self, | ||
| ConstKind, | ||
| Ty, | ||
| TypeckResults, | ||
| }, | ||
| }; | ||
| use rustc_session::{ | ||
| declare_lint, | ||
| declare_lint_pass, | ||
| }; | ||
| use rustc_type_ir::sty::TyKind; | ||
|
|
||
| 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<String, AccountId> } | ||
| /// ``` | ||
| /// | ||
| /// The following usage of the non-fallible API is unsafe: | ||
| /// | ||
| /// ```rust | ||
| /// // 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 | ||
| /// // 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 | ||
| /// // 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, | ||
| Warn, | ||
| "using non-fallible API" | ||
| } | ||
|
|
||
| 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<Self> { | ||
| 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<String> { | ||
| 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 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 | ||
| /// replaced | ||
| fn check_method_call( | ||
| &self, | ||
| receiver_ty: &TyToCheck, | ||
| method_path: &PathSegment, | ||
| method_name: &str, | ||
| arg_ty: Ty<'tcx>, | ||
| ) { | ||
| if_chain! { | ||
| if !self.is_statically_known(&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 !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); | ||
| 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_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); | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.