Port to the Linebender lint set (#20)

As documented at: https://linebender.org/wiki/canonical-lints/ --------- Co-authored-by: Kaur Kuut <[email protected]>
linebender · Nov 21, 2024 · 04c34ce · 04c34ce
1 parent 0cdb14a
commit 04c34ce
Show file tree

Hide file tree

Showing 9 changed files with 130 additions and 55 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -4,15 +4,15 @@ env:
   # come automatically. If the version specified here is no longer the latest stable version,
   # then please feel free to submit a PR that adjusts it along with the potential clippy fixes.
   # When updating this, also update RUST_DOCS_COMPILE_VER below to the same version
-  RUST_STABLE_VER: "1.79" # In quotes because otherwise (e.g.) 1.70 would be interpreted as 1.7
+  RUST_STABLE_VER: "1.82" # In quotes because otherwise (e.g.) 1.70 would be interpreted as 1.7
   # The version of rustc we use to test that doc examples compile.
   # This is required because we depend on the unstable `-Zdoctest-xcompile`.
   # See https://github.com/rust-lang/rust/issues/64245
   # This should be the same version of Rust which the above stable was branched from.
   # To get the correct date, use `cargo --version`. E.g for 1.77, this prints
   # cargo 1.77.0 (3fe68eabf 2024-02-29)
   # So the date used when we depended on 1.77 was 2024-02-29.
-  RUST_DOCS_COMPILE_VER: "nightly-2024-06-03"
+  RUST_DOCS_COMPILE_VER: "nightly-2024-10-15"
   # The purpose of checking with the minimum supported Rust toolchain is to detect its staleness.
   # If the compilation fails, then the version specified here needs to be bumped up to reality.
   # Be sure to also update the rust-version property in the workspace Cargo.toml file,

diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -19,13 +19,66 @@ edition = "2021"
 version = "0.1.1"
 
 [workspace.lints]
-rust.unreachable_pub = "warn"
-rust.unsafe_op_in_unsafe_fn = "warn"
+# Unsafe is used in android_trace for FFI.
+rust.unsafe_code = "deny"
+
+# LINEBENDER LINT SET - v1
+# See https://linebender.org/wiki/canonical-lints/
+rust.keyword_idents_2024 = "forbid"
+rust.non_ascii_idents = "forbid"
+rust.non_local_definitions = "forbid"
+rust.unsafe_op_in_unsafe_fn = "forbid"
+
+rust.elided_lifetimes_in_paths = "warn"
+rust.let_underscore_drop = "warn"
 rust.missing_debug_implementations = "warn"
 rust.missing_docs = "warn"
+rust.single_use_lifetimes = "warn"
+rust.trivial_numeric_casts = "warn"
+rust.unexpected_cfgs = "warn"
+rust.unit_bindings = "warn"
+rust.unnameable_types = "warn"
+rust.unreachable_pub = "warn"
+rust.unused_import_braces = "warn"
+rust.unused_lifetimes = "warn"
+rust.unused_macro_rules = "warn"
+rust.unused_qualifications = "warn"
+rust.variant_size_differences = "warn"
 
+clippy.allow_attributes = "warn"
+clippy.allow_attributes_without_reason = "warn"
+clippy.cast_possible_truncation = "warn"
+clippy.collection_is_never_read = "warn"
+clippy.dbg_macro = "warn"
+clippy.debug_assert_with_mut_call = "warn"
 clippy.doc_markdown = "warn"
+clippy.exhaustive_enums = "warn"
+clippy.fn_to_numeric_cast_any = "forbid"
+clippy.infinite_loop = "warn"
+clippy.large_include_file = "warn"
+clippy.large_stack_arrays = "warn"
+clippy.match_same_arms = "warn"
+clippy.mismatching_type_param_order = "warn"
+clippy.missing_assert_message = "warn"
+clippy.missing_errors_doc = "warn"
+clippy.missing_fields_in_debug = "warn"
+clippy.missing_panics_doc = "warn"
+clippy.partial_pub_fields = "warn"
+clippy.return_self_not_must_use = "warn"
+clippy.same_functions_in_if_condition = "warn"
 clippy.semicolon_if_nothing_returned = "warn"
+clippy.shadow_unrelated = "warn"
+clippy.should_panic_without_expect = "warn"
+clippy.todo = "warn"
+clippy.unseparated_literal_suffix = "warn"
+clippy.use_self = "warn"
+clippy.wildcard_imports = "warn"
+
+clippy.cargo_common_metadata = "warn"
+clippy.negative_feature_names = "warn"
+clippy.redundant_feature_names = "warn"
+clippy.wildcard_dependencies = "warn"
+# END LINEBENDER LINT SET
 
 [workspace.dependencies]
 android_trace = { path = "./android_trace", version = "0.1.1", default-features = false }
diff --git a/android_trace/src/ffi.rs b/android_trace/src/ffi.rs
@@ -2,25 +2,32 @@
 // SPDX-License-Identifier: Apache-2.0 OR MIT
 
 #[cfg(not(all(feature = "api_level_23", feature = "api_level_29")))]
-use core::{
-    ffi::{c_void, CStr},
-    mem,
-};
+use core::{ffi::CStr, mem};
 
-use core::ffi::c_char;
+use libc::c_char;
 
 /// # Safety
 ///
 /// `func` must have been produced from a call to `dlsym` which is
-/// reasonably expected to have the right type
+/// reasonably expected to have the right type.
+///
+/// All the preconditions from [`transmute_copy`](core::mem::transmute_copy) apply.
 #[cfg(not(all(feature = "api_level_23", feature = "api_level_29")))]
-unsafe fn transmute_if_not_null<F>(func: *mut c_void) -> Option<F> {
-    assert_eq!(mem::size_of::<F>(), mem::size_of::<*mut c_void>());
+#[allow(
+    unused_qualifications,
+    // reason = "These qualifications are used, because our MSRV is lower than 1.81"
+)]
+unsafe fn transmute_if_not_null<F>(func: *mut libc::c_void) -> Option<F> {
+    assert_eq!(
+        mem::size_of::<F>(),
+        mem::size_of::<*mut libc::c_void>(),
+        "transmute_copy is used because this function is generic"
+    );
     if func.is_null() {
         return None;
     }
-    // Safety:
-    Some(unsafe { mem::transmute_copy::<*mut c_void, F>(&func) })
+    // Safety: The preconditions are guaranteed by the caller.
+    Some(unsafe { mem::transmute_copy::<*mut libc::c_void, F>(&func) })
 }
 
 #[link(name = "android", kind = "dylib")]

diff --git a/android_trace/src/lib.rs b/android_trace/src/lib.rs
@@ -13,6 +13,16 @@
 //! </style>
 #![doc =  include_str!("../README.md")]
 #![cfg_attr(docsrs, feature(doc_auto_cfg))]
+#![allow(
+    unsafe_code,
+    // reason = "This crate does FFI, and so must be able to use unsafe"
+)]
+// LINEBENDER LINT SET - v1
+// See https://linebender.org/wiki/canonical-lints/
+// These lints aren't included in Cargo.toml because they
+// shouldn't apply to examples and tests
+#![warn(unused_crate_dependencies)]
+#![warn(clippy::print_stdout, clippy::print_stderr)]
 
 #[cfg(not(feature = "api_level_23"))]
 use ffi::ATraceAPILevel23Methods;

diff --git a/tracing_android_trace/Cargo.toml b/tracing_android_trace/Cargo.toml
@@ -30,11 +30,8 @@ workspace = true
 tracing-subscriber = { version = "0.3.18", default-features = false, features = [
     "std",
     "fmt",
-    "smallvec",
 ] }
 tracing = "0.1.40"
-tracing-core = "0.1.32"
-smallvec = "1.13.1"
 thread_local = "1.1.8"
 
 [features]
@@ -45,9 +42,6 @@ api_level_23 = ["android_trace/api_level_23"]
 # Assume that Android API level 29 is available, to avoid runtime symbol lookups entirely
 api_level_29 = ["android_trace/api_level_29"]
 
-[dev-dependencies]
-static_assertions = "1.1.0"
-
 [target.'cfg(target_os = "android")'.dependencies]
 # We only depend on android_trace on Android so that we can customise the
 # `compile_error` message

diff --git a/tracing_android_trace/src/async_layer.rs b/tracing_android_trace/src/async_layer.rs
@@ -45,7 +45,7 @@ impl AndroidTraceAsyncLayer {
     /// Note that this takes ownership because `AndroidTrace` has a trivial `Clone`
     pub fn with_trace(trace: AndroidTrace) -> Self {
         let could_use_api_level_29 = trace.could_use_api_level_29();
-        AndroidTraceAsyncLayer {
+        Self {
             trace,
             fmt_fields: DefaultFields::new(),
             could_use_api_level_29,
@@ -65,6 +65,10 @@ pub(crate) struct ATraceExtensionAsync {
     cookie: i32,
 }
 
+#[allow(
+    clippy::print_stderr,
+    // reason = "tracing::warn could lead to an infinite loop inside the tracing layer"
+)]
 impl<S> tracing_subscriber::Layer<S> for AndroidTraceAsyncLayer
 where
     S: tracing::Subscriber + for<'a> LookupSpan<'a>,
@@ -88,11 +92,16 @@ where
                 let name = CString::new(name);
                 match name {
                     Ok(name) => {
-                        // We truncate on purpose here. The scenario where this breaks are rare
-                        let cookie = id.into_u64() as u32 as i32;
+                        #[allow(
+                            clippy::cast_possible_truncation,
+                            // reason = "The cookies have to be i32, but the available source is u64"
+                        )]
+                        let cookie = (id.into_u64() % u32::MAX as u64) as u32 as i32;
                         extensions
                             .insert::<ATraceExtensionAsync>(ATraceExtensionAsync { name, cookie });
                     }
+                    // This error printing style is based on the precedent of tracing_subscriber.
+                    // Using `tracing::warn` or similar could lead to an infinite loop.
                     Err(e) => eprintln!(
                         concat!(
                             "[tracing_android_trace] Unable to format the following ",

diff --git a/tracing_android_trace/src/lib.rs b/tracing_android_trace/src/lib.rs
@@ -19,6 +19,12 @@
 #![doc = include_str!("../README.md")]
 #![cfg_attr(docsrs, feature(doc_auto_cfg))]
 #![forbid(unsafe_code)]
+// LINEBENDER LINT SET - v1
+// See https://linebender.org/wiki/canonical-lints/
+// These lints aren't included in Cargo.toml because they
+// shouldn't apply to examples and tests
+#![warn(unused_crate_dependencies)]
+#![warn(clippy::print_stdout, clippy::print_stderr)]
 
 #[cfg(not(target_os = "android"))]
 compile_error!(

diff --git a/tracing_android_trace/src/sync_layer.rs b/tracing_android_trace/src/sync_layer.rs
@@ -82,7 +82,7 @@ impl AndroidTraceLayer {
     ///
     /// Note that this takes ownership because `AndroidTrace` has a trivial `Clone`
     pub fn with_trace(trace: AndroidTrace) -> Self {
-        AndroidTraceLayer {
+        Self {
             trace,
             fmt_fields: DefaultFields::new(),
             current_actual_stack: ThreadLocal::new(),
@@ -101,14 +101,18 @@ struct ATraceExtension {
     name: CString,
 }
 
+#[allow(
+    clippy::print_stderr,
+    // reason = "tracing::warn could lead to an infinite loop inside the tracing layer"
+)]
 impl<S> tracing_subscriber::Layer<S> for AndroidTraceLayer
 where
     S: tracing::Subscriber + for<'a> LookupSpan<'a>,
 {
     fn on_new_span(
         &self,
         attrs: &span::Attributes<'_>,
-        id: &span::Id,
+        id: &Id,
         ctx: tracing_subscriber::layer::Context<'_, S>,
     ) {
         if self.trace.is_enabled().unwrap_or(false) {
@@ -143,7 +147,7 @@ where
 
     fn on_record(
         &self,
-        _span: &span::Id,
+        _span: &Id,
         _values: &span::Record<'_>,
         _ctx: tracing_subscriber::layer::Context<'_, S>,
     ) {
@@ -153,8 +157,8 @@ where
 
     fn on_follows_from(
         &self,
-        _span: &span::Id,
-        _follows: &span::Id,
+        _span: &Id,
+        _follows: &Id,
         _ctx: tracing_subscriber::layer::Context<'_, S>,
     ) {
         // Not meaningfully implementable
@@ -164,7 +168,7 @@ where
         // TODO: Does it make sense to do anything here?
     }
 
-    fn on_enter(&self, id: &span::Id, ctx: tracing_subscriber::layer::Context<'_, S>) {
+    fn on_enter(&self, id: &Id, ctx: tracing_subscriber::layer::Context<'_, S>) {
         let span = ctx.span(id).expect("Span not found, this is a bug");
         let extensions = span.extensions();
         // The extension is optional in case tracing is disabled
@@ -175,17 +179,17 @@ where
         }
     }
 
-    fn on_exit(&self, id: &span::Id, ctx: tracing_subscriber::layer::Context<'_, S>) {
+    fn on_exit(&self, exiting_id: &Id, ctx: tracing_subscriber::layer::Context<'_, S>) {
         // For some reason, the `S`'s `exit` method is called *before* on_exit, so the span which is exiting is
         // no longer in the current stack
         // Because of this, to find the place it *used* to be, we find the item which was the parent of the current item
         // in the stack
-        let this_span = ctx.span(id).expect("Span not found, this is a bug");
-        let Some(stack) = self.current_actual_stack.get() else {
+        let this_span = ctx.span(exiting_id).expect("Span not found, this is a bug");
+        let Some(data) = self.current_actual_stack.get() else {
             // No spans had the extension, so nothing to do
             return;
         };
-        let mut data = stack.borrow_mut();
+        let mut data = data.borrow_mut();
         let stack = &mut data.stack;
         if stack.is_empty() {
             // This span should have been already closed in the case where a previous span was not
@@ -201,16 +205,22 @@ where
             return;
         }
         let last = stack.last().unwrap().as_ref();
-        debug_assert!(last.is_some());
-        if last == Some(id) {
+        debug_assert!(
+            last.is_some(),
+            "We're exiting a span, so we should have entered one previously"
+        );
+        if last == Some(exiting_id) {
             stack.pop();
             // Fast path, if we were at the top of the stack (i.e. the current top is our parent)
             // Matches the call in `on_enter`
             self.trace.end_section();
             #[cfg(debug_assertions)]
             {
                 let extensions = this_span.extensions();
-                debug_assert!(extensions.get::<ATraceExtension>().is_some());
+                debug_assert!(
+                    extensions.get::<ATraceExtension>().is_some(),
+                    "We believe we handled this span's creation, so it should have the extension"
+                );
             }
             // Clear all the dangling items on the stack
             while let Some(None) = stack.last() {
@@ -228,7 +238,7 @@ where
             let mut index_of_this = None;
             for (idx, item) in stack.iter_mut().enumerate().rev() {
                 self.trace.end_section();
-                if item.as_ref() == Some(id) {
+                if item.as_ref() == Some(exiting_id) {
                     index_of_this = Some(idx);
                     *item = None;
                     break;
@@ -248,7 +258,10 @@ where
                 stack.clear();
                 data.extra_unclosed_values = extra_values;
                 let extensions = this_span.extensions();
-                debug_assert!(extensions.get::<ATraceExtension>().is_none());
+                debug_assert!(
+                    extensions.get::<ATraceExtension>().is_none(),
+                    "We don't close a span as we believe it wasn't entered, but it was entered"
+                );
                 return;
             };
             for id in stack[index_of_this..].iter() {