From cfada8405c212745e183205f2aa73c12200d4c1a Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Tue, 15 Aug 2023 13:54:01 +0700 Subject: [PATCH] Linter: Warn when number primitive is annotated with `#[ink(topic)]` (#1837) * The initial structure of the linting crate and tests Needed for #1436 * Implement the lint that checks primitive numeric types in expanded code * The initial implementation of the `primitive_topic` lint * chore: Run clippy * fix(fmt) * fix: Lint suppressions The problem with suppressions is that `rustc` saves the state of the last visited node and checks its attributes to decide if the lint warning should be suppressed there (see: [LateContext::last_node_with_lint_attrs](https://github.com/rust-lang/rust/blob/c44324a4fe8e96f9d6473255df6c3a481caca76f/compiler/rustc_lint/src/context.rs#L1105)). This commit adds a call to the utility function from clippy that checks if the lint is suppressed for the field definition node, not for the last node (which is `Topics` impl). * chore: cleanup * chore: make warning messages more consistent * feat: support events 2.0 Rework the lint after #1827 is merged * chore: Fix comments; remove needless empty file * chore: Update `clippy_utils` version * fix: Cargo.toml --------- Co-authored-by: Georgiy Komarov --- linting/Cargo.toml | 14 +- linting/rust-toolchain.toml | 2 +- linting/src/lib.rs | 26 +-- linting/src/primitive_topic.rs | 223 +++++++++++++++++++++++++ linting/ui/fail/primitive_topic.rs | 36 ++++ linting/ui/fail/primitive_topic.stderr | 28 ++++ linting/ui/pass/primitive_topic.rs | 31 ++++ 7 files changed, 343 insertions(+), 17 deletions(-) create mode 100644 linting/src/primitive_topic.rs create mode 100644 linting/ui/fail/primitive_topic.rs create mode 100644 linting/ui/fail/primitive_topic.stderr create mode 100644 linting/ui/pass/primitive_topic.rs diff --git a/linting/Cargo.toml b/linting/Cargo.toml index bd5024e310..a5861b1f8e 100644 --- a/linting/Cargo.toml +++ b/linting/Cargo.toml @@ -18,18 +18,18 @@ include = ["Cargo.toml", "*.rs", "LICENSE"] crate-type = ["cdylib"] [dependencies] -clippy_utils = { git = "https://github.com/rust-lang/rust-clippy", rev = "1480cea393d0cee195e59949eabdfbcf1230f7f9" } -dylint_linting = "2.0.0" +clippy_utils = { git = "https://github.com/rust-lang/rust-clippy", rev = "1d334696587ac22b3a9e651e7ac684ac9e0697b2" } +dylint_linting = "2.1.12" if_chain = "1.0.2" log = "0.4.14" regex = "1.5.4" [dev-dependencies] -dylint_testing = "2.0.0" +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 } +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 } ink_storage = { path = "../crates/storage", default-features = false } @@ -45,6 +45,12 @@ scale-info = { version = "2.6", default-features = false, features = ["derive"] # # Those files require the ink! dependencies though, by having # them as examples here, they inherit the `dev-dependencies`. +[[example]] +name = "primitive_topic_pass" +path = "ui/pass/primitive_topic.rs" +[[example]] +name = "primitive_topic_fail" +path = "ui/fail/primitive_topic.rs" [package.metadata.rust-analyzer] rustc_private = true diff --git a/linting/rust-toolchain.toml b/linting/rust-toolchain.toml index fdec8427c3..cac65e4fce 100644 --- a/linting/rust-toolchain.toml +++ b/linting/rust-toolchain.toml @@ -2,5 +2,5 @@ # https://github.com/trailofbits/dylint/blob/ef7210cb08aac920c18d2141604efe210029f2a2/internal/template/rust-toolchain [toolchain] -channel = "nightly-2023-01-27" +channel = "nightly-2023-07-14" components = ["llvm-tools-preview", "rustc-dev"] diff --git a/linting/src/lib.rs b/linting/src/lib.rs index 266d33d027..5b1dc50b4b 100644 --- a/linting/src/lib.rs +++ b/linting/src/lib.rs @@ -1,18 +1,16 @@ // Copyright (C) Parity Technologies (UK) Ltd. -// This file is part of cargo-contract. // -// cargo-contract is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. +// 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 // -// cargo-contract is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. +// http://www.apache.org/licenses/LICENSE-2.0 // -// You should have received a copy of the GNU General Public License -// along with cargo-contract. If not, see . +// 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. #![doc( html_logo_url = "https://use.ink/img/crate-docs/logo.png", @@ -30,12 +28,16 @@ extern crate rustc_middle; extern crate rustc_session; extern crate rustc_span; +mod primitive_topic; + #[doc(hidden)] #[no_mangle] pub fn register_lints( _sess: &rustc_session::Session, - _lint_store: &mut rustc_lint::LintStore, + lint_store: &mut rustc_lint::LintStore, ) { + lint_store.register_lints(&[primitive_topic::PRIMITIVE_TOPIC]); + lint_store.register_late_pass(|_| Box::new(primitive_topic::PrimitiveTopic)); } #[test] diff --git a/linting/src/primitive_topic.rs b/linting/src/primitive_topic.rs new file mode 100644 index 0000000000..14bcdd2451 --- /dev/null +++ b/linting/src/primitive_topic.rs @@ -0,0 +1,223 @@ +// 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:** Checks for ink! contracts that use + /// the [`#[ink(topic)]`](https://use.ink/macros-attributes/topic) annotation with primitive + /// number types. Topics are discrete events for which it makes sense to filter. Typical + /// examples of fields that should be filtered are `AccountId`, `bool` or `enum` variants. + /// + /// **Why is this bad?** It typically doesn't make sense to annotate types like `u32` or `i32` + /// as a topic, if those fields can take continuous values that could be anywhere between + /// `::MIN` and `::MAX`. An example of a case where it doesn't make sense at all to have a + /// topic on the storage field is something like `value: Balance` in the examle below. + /// + /// **Example:** + /// + /// ```rust + /// // Good + /// // Filtering transactions based on source and destination addresses. + /// #[ink(event)] + /// pub struct Transaction { + /// #[ink(topic)] + /// src: Option, + /// #[ink(topic)] + /// dst: Option, + /// value: Balance, + /// } + /// ``` + /// + /// ```rust + /// // Bad + /// // It typically makes no sense to filter `Balance`, since its value may varies from `::MAX` + /// // to `::MIN`. + /// #[ink(event)] + /// pub struct Transaction { + /// #[ink(topic)] + /// src: Option, + /// #[ink(topic)] + /// dst: Option, + /// #[ink(topic)] + /// value: Balance, + /// } + /// ``` + pub PRIMITIVE_TOPIC, + Warn, + "`#[ink(topic)]` should not be used with a number primitive" +} + +declare_lint_pass!(PrimitiveTopic => [PRIMITIVE_TOPIC]); + +/// Returns `true` if `item` is an implementation of `::ink::env::Event` for a storage +/// struct. If that's the case, it returns the name of this struct. +fn is_ink_event_impl<'tcx>(cx: &LateContext<'tcx>, item: &'tcx Item<'_>) -> bool { + if let Some(trait_ref) = cx.tcx.impl_trait_ref(item.owner_id) { + match_def_path( + cx, + trait_ref.skip_binder().def_id, + &["ink_env", "event", "Event"], + ) + } else { + false + } +} + +/// Returns `true` if `impl_item` is the `topics` function +fn is_topics_function(impl_item: &ImplItemRef) -> bool { + impl_item.kind == AssocItemKind::Fn { has_self: true } + && impl_item.ident.name.as_str() == "topics" +} + +/// Returns `true` if `ty` is a numerical primitive type that should not be annotated with +/// `#[ink(topic)]` +fn is_primitive_number_ty(ty: &Ty) -> bool { + matches!(ty.kind(), ty::Int(_) | ty::Uint(_)) +} + +/// Reports a topic-annotated field with a numerical primitive type +fn report_field(cx: &LateContext, event_def_id: DefId, field_name: &str) { + if_chain! { + if let Some(Node::Item(event_node)) = cx.tcx.hir().get_if_local(event_def_id); + if let ItemKind::Struct(ref struct_def, _) = event_node.kind; + if let Some(field) = struct_def.fields().iter().find(|f|{ f.ident.as_str() == field_name }); + if !is_lint_allowed(cx, PRIMITIVE_TOPIC, field.hir_id); + then { + span_lint_and_then( + cx, + PRIMITIVE_TOPIC, + field.span, + "using `#[ink(topic)]` for a field with a primitive number type", + |diag| { + let snippet = snippet_opt(cx, field.span).expect("snippet must exist"); + diag.span_suggestion( + field.span, + "consider removing `#[ink(topic)]`".to_string(), + snippet, + Applicability::Unspecified, + ); + }, + ) + } + } +} + +/// Returns `DefId` of the event struct for which `Topics` is implemented +fn get_event_def_id(topics_impl: &Impl) -> Option { + if_chain! { + if let rustc_hir::TyKind::Path(qpath) = &topics_impl.self_ty.kind; + if let QPath::Resolved(_, path) = qpath; + if let Res::Def(DefKind::Struct, def_id) = path.res; + then { Some(def_id) } + else { None } + } +} + +impl<'tcx> LateLintPass<'tcx> for PrimitiveTopic { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { + if_chain! { + if let ItemKind::Impl(topics_impl) = &item.kind; + if is_ink_event_impl(cx, item); + if let Some(event_def_id) = get_event_def_id(topics_impl); + then { + topics_impl.items.iter().for_each(|impl_item| { + if_chain! { + // We need to extract field patterns from the event sturct matched in the + // `topics` function to access their inferred types. + // Here is the simplified example of the expanded code: + // ``` + // fn topics(/* ... */) { + // match self { + // MyEvent { + // field_1: __binding_0, + // field_2: __binding_1, + // /* ... */ + // .. + // } => { /* ... */ } + // } + // } + // ``` + if is_topics_function(impl_item); + let impl_item = cx.tcx.hir().impl_item(impl_item.id); + if let ImplItemKind::Fn(_, eid) = impl_item.kind; + let body = cx.tcx.hir().body(eid).value; + if let ExprKind::Block (block, _) = body.kind; + if let Some(match_self) = block.expr; + if let ExprKind::Match(_, [Arm { pat: arm_pat, .. }], _) = match_self.kind; + if let PatKind::Struct(_, pat_fields, _) = &arm_pat.kind; + then { + pat_fields.iter().for_each(|pat_field| { + cx.tcx + .has_typeck_results(pat_field.hir_id.owner.def_id) + .then(|| { + if let TyKind::Ref(_, ty, _) = cx + .tcx + .typeck(pat_field.hir_id.owner.def_id) + .pat_ty(pat_field.pat) + .kind() + { + if is_primitive_number_ty(ty) { + report_field(cx, + event_def_id, + pat_field.ident.as_str()) + } + } + }); + }) + } + } + }) + } + } + } +} diff --git a/linting/ui/fail/primitive_topic.rs b/linting/ui/fail/primitive_topic.rs new file mode 100644 index 0000000000..6c0cb5d00e --- /dev/null +++ b/linting/ui/fail/primitive_topic.rs @@ -0,0 +1,36 @@ +pub type TyAlias1 = i32; +pub type TyAlias2 = TyAlias1; + +#[ink::contract] +pub mod primitive_topic { + + #[ink(event)] + pub struct Transaction { + // Bad + #[ink(topic)] + value_1: u8, + // Bad + #[ink(topic)] + value_2: Balance, + // Bad + #[ink(topic)] + value_3: crate::TyAlias1, + // Bad + #[ink(topic)] + value_4: crate::TyAlias2, + } + + #[ink(storage)] + pub struct PrimitiveTopic {} + + impl PrimitiveTopic { + #[ink(constructor)] + pub fn new() -> Self { + Self {} + } + #[ink(message)] + pub fn do_nothing(&mut self) {} + } +} + +fn main() {} diff --git a/linting/ui/fail/primitive_topic.stderr b/linting/ui/fail/primitive_topic.stderr new file mode 100644 index 0000000000..634dacf4b8 --- /dev/null +++ b/linting/ui/fail/primitive_topic.stderr @@ -0,0 +1,28 @@ +error: using `#[ink(topic)]` for a field with a primitive number type + --> $DIR/primitive_topic.rs:11:9 + | +LL | value_1: u8, + | ^^^^^^^^^^^ help: consider removing `#[ink(topic)]`: `value_1: u8` + | + = note: `-D primitive-topic` implied by `-D warnings` + +error: using `#[ink(topic)]` for a field with a primitive number type + --> $DIR/primitive_topic.rs:14:9 + | +LL | value_2: Balance, + | ^^^^^^^^^^^^^^^^ help: consider removing `#[ink(topic)]`: `value_2: Balance` + +error: using `#[ink(topic)]` for a field with a primitive number type + --> $DIR/primitive_topic.rs:17:9 + | +LL | value_3: crate::TyAlias1, + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `#[ink(topic)]`: `value_3: crate::TyAlias1` + +error: using `#[ink(topic)]` for a field with a primitive number type + --> $DIR/primitive_topic.rs:20:9 + | +LL | value_4: crate::TyAlias2, + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `#[ink(topic)]`: `value_4: crate::TyAlias2` + +error: aborting due to 4 previous errors + diff --git a/linting/ui/pass/primitive_topic.rs b/linting/ui/pass/primitive_topic.rs new file mode 100644 index 0000000000..6cc5428072 --- /dev/null +++ b/linting/ui/pass/primitive_topic.rs @@ -0,0 +1,31 @@ +#[ink::contract] +pub mod primitive_topic { + + #[ink(event)] + pub struct Transaction { + #[ink(topic)] + src: Option, + #[ink(topic)] + dst: Option, + // Good: no topic annotation + value_1: Balance, + // Good: suppressed warning + #[ink(topic)] + #[cfg_attr(dylint_lib = "ink_linting", allow(primitive_topic))] + value_2: Balance, + } + + #[ink(storage)] + pub struct PrimitiveTopic {} + + impl PrimitiveTopic { + #[ink(constructor)] + pub fn new() -> Self { + Self {} + } + #[ink(message)] + pub fn do_nothing(&mut self) {} + } +} + +fn main() {}