Ramp up linting (#1672)

This enables a number of rustc lints.

The deny-by-default lints indicate a lack of compiler guarantees, future
incompatibility (with no guarantees in the meantime), introduce
surprising behavior, or are likely to cause undesired behavior.

The warn-by-default lints indicate possible errors, future
incompatibility (with guaranteed behavior in the meantime), or any other
stylistic issues (including idoms).

Author: jhpratt

.bazelrc

@@ -53,6 +53,47 @@ build:self_execute --platform_suffix=self-execute
 build --aspects=@rules_rust//rust:defs.bzl%rustfmt_aspect
 build --aspects=@rules_rust//rust:defs.bzl%rust_clippy_aspect
 
+# Keep this in sync with the top-level Cargo.toml
+build --@rules_rust//:extra_rustc_flag=-Dambiguous_glob_reexports
+build --@rules_rust//:extra_rustc_flag=-Dclashing_extern_declarations
+build --@rules_rust//:extra_rustc_flag=-Dconst_item_mutation
+build --@rules_rust//:extra_rustc_flag=-Ddangling_pointers_from_temporaries
+build --@rules_rust//:extra_rustc_flag=-Dderef_nullptr
+build --@rules_rust//:extra_rustc_flag=-Ddrop_bounds
+build --@rules_rust//:extra_rustc_flag=-Dfuture_incompatible
+build --@rules_rust//:extra_rustc_flag=-Dhidden_glob_reexports
+build --@rules_rust//:extra_rustc_flag=-Dimproper_ctypes
+build --@rules_rust//:extra_rustc_flag=-Dimproper_ctypes_definitions
+build --@rules_rust//:extra_rustc_flag=-Dinvalid_from_utf8
+build --@rules_rust//:extra_rustc_flag=-Dinvalid_macro_export_arguments
+build --@rules_rust//:extra_rustc_flag=-Dinvalid_nan_comparisons
+build --@rules_rust//:extra_rustc_flag=-Dinvalid_reference_casting
+build --@rules_rust//:extra_rustc_flag=-Dinvalid_value
+build --@rules_rust//:extra_rustc_flag=-Dopaque_hidden_inferred_bound
+build --@rules_rust//:extra_rustc_flag=-Doverlapping_range_endpoints
+build --@rules_rust//:extra_rustc_flag=-Dsuspicious_double_ref_op
+build --@rules_rust//:extra_rustc_flag=-Dunconditional_recursion
+build --@rules_rust//:extra_rustc_flag=-Dunexpected_cfgs
+build --@rules_rust//:extra_rustc_flag=-Dunnameable_test_items
+build --@rules_rust//:extra_rustc_flag=-Dunsafe_op_in_unsafe_fn
+build --@rules_rust//:extra_rustc_flag=-Dunstable_syntax_pre_expansion
+build --@rules_rust//:extra_rustc_flag=-Wkeyword_idents
+build --@rules_rust//:extra_rustc_flag=-Wlet_underscore
+build --@rules_rust//:extra_rustc_flag=-Wmacro_use_extern_crate
+build --@rules_rust//:extra_rustc_flag=-Wmeta_variable_misuse
+build --@rules_rust//:extra_rustc_flag=-Wmissing_abi
+build --@rules_rust//:extra_rustc_flag=-Wmissing_copy_implementations
+build --@rules_rust//:extra_rustc_flag=-Wmissing_debug_implementations
+build --@rules_rust//:extra_rustc_flag=-Wnoop_method_call
+build --@rules_rust//:extra_rustc_flag=-Wsingle_use_lifetimes
+build --@rules_rust//:extra_rustc_flag=-Wtrivial_casts
+build --@rules_rust//:extra_rustc_flag=-Wtrivial_numeric_casts
+build --@rules_rust//:extra_rustc_flag=-Wunreachable_pub
+build --@rules_rust//:extra_rustc_flag=-Wunused
+build --@rules_rust//:extra_rustc_flag=-Wunused_import_braces
+build --@rules_rust//:extra_rustc_flag=-Wunused_lifetimes
+build --@rules_rust//:extra_rustc_flag=-Wunused_qualifications
+build --@rules_rust//:extra_rustc_flag=-Wvariant_size_differences
 # TODO(aaronmondal): Extend these flags until we can run with clippy::pedantic.
 build --@rules_rust//:clippy_flags=-Dwarnings,-Dclippy::missing_const_for_fn,-Dclippy::uninlined_format_args,-Dclippy::manual_string_new,-Dclippy::manual_let_else,-Dclippy::single_match_else,-Dclippy::redundant_closure_for_method_calls,-Dclippy::semicolon_if_nothing_returned,-Dclippy::unreadable_literal,-Dclippy::range_plus_one,-Dclippy::inconsistent_struct_constructor,-Dclippy::match_wildcard_for_single_variants,-Dclippy::implicit_clone,-Dclippy::needless_pass_by_value,-Dclippy::explicit_deref_methods,-Dclippy::trivially_copy_pass_by_ref,-Dclippy::unnecessary_wraps,-Dclippy::cast_lossless,-Dclippy::map_unwrap_or,-Dclippy::ref_as_ptr,-Dclippy::inline_always,-Dclippy::redundant_else,-Dclippy::return_self_not_must_use,-Dclippy::match_same_arms,-Dclippy::explicit_iter_loop,-Dclippy::items_after_statements,-Dclippy::explicit_into_iter_loop,-Dclippy::stable_sort_primitive,-Dclippy::ptr_as_ptr,-Dclippy::needless_raw_string_hashes,-Dclippy::default_trait_access,-Dclippy::ignored_unit_patterns,-Dclippy::needless_continue,-Dclippy::wildcard_imports,-Dclippy::doc_markdown,-Dclippy::struct_field_names,-Dclippy::implicit_hasher
 build --@rules_rust//:clippy.toml=//:clippy.toml

Cargo.toml

@@ -75,3 +75,49 @@ tokio-stream = ["fs"]
 tonic-build = ["prost"]
 tonic = ["transport", "tls"]
 uuid = ["v4", "serde"]
+
+[workspace.lints.rust]
+ambiguous-glob-reexports = "deny"
+clashing-extern-declarations = "deny"
+const-item-mutation = "deny"
+dangling-pointers-from-temporaries = "deny"
+deref-nullptr = "deny"
+drop-bounds = "deny"
+future-incompatible = "deny"
+hidden-glob-reexports = "deny"
+improper-ctypes = "deny"
+improper-ctypes-definitions = "deny"
+invalid-from-utf8 = "deny"
+invalid-macro-export-arguments = "deny"
+invalid-nan-comparisons = "deny"
+invalid-reference-casting = "deny"
+invalid-value = "deny"
+opaque-hidden-inferred-bound = "deny"
+overlapping-range-endpoints = "deny"
+suspicious-double-ref-op = "deny"
+unconditional-recursion = "deny"
+unexpected-cfgs = "deny"
+unnameable-test-items = "deny"
+unsafe-op-in-unsafe-fn = "deny"
+unstable-syntax-pre-expansion = "deny"
+
+keyword-idents = "warn"
+let-underscore = "warn"
+macro-use-extern-crate = "warn"
+meta-variable-misuse = "warn"
+missing-abi = "warn"
+missing-copy-implementations = "warn"
+missing-debug-implementations = "warn"
+noop-method-call = "warn"
+single-use-lifetimes = "warn"
+trivial-casts = "warn"
+trivial-numeric-casts = "warn"
+unreachable-pub = "warn"
+unused = { level = "warn", priority = -1 }
+unused-import-braces = "warn"
+unused-lifetimes = "warn"
+unused-qualifications = "warn"
+variant-size-differences = "warn"
+
+[workspace.lints.clippy]
+all = { level = "warn", priority = -1 }

nativelink-config/Cargo.toml

@@ -1,3 +1,5 @@
+lints.workspace = true
+
 [package]
 name = "nativelink-config"
 version = "0.6.0"

nativelink-config/src/cas_server.rs

@@ -327,7 +327,7 @@ pub struct TlsConfig {
 ///
 /// Note: All of these default to hyper's default values unless otherwise
 /// specified.
-#[derive(Deserialize, Debug, Default)]
+#[derive(Deserialize, Debug, Default, Clone, Copy)]
 #[serde(deny_unknown_fields)]
 pub struct HttpServerConfig {
     /// Interval to send keep-alive pings via HTTP2.

nativelink-config/src/lib.rs

@@ -32,13 +32,13 @@ pub struct NamedConfig<Spec> {
     pub spec: Spec,
 }
 
-pub type StoreConfig = NamedConfig<crate::stores::StoreSpec>;
-pub type SchedulerConfig = NamedConfig<crate::schedulers::SchedulerSpec>;
+pub type StoreConfig = NamedConfig<stores::StoreSpec>;
+pub type SchedulerConfig = NamedConfig<schedulers::SchedulerSpec>;
 
 // TODO(aaronmondal): Remove all the iterator impls and the Deserializer once we
 //                    fully migrate to the new config schema.
-pub type StoreConfigs = NamedConfigs<crate::stores::StoreSpec>;
-pub type SchedulerConfigs = NamedConfigs<crate::schedulers::SchedulerSpec>;
+pub type StoreConfigs = NamedConfigs<stores::StoreSpec>;
+pub type SchedulerConfigs = NamedConfigs<schedulers::SchedulerSpec>;
 
 #[derive(Debug)]
 pub struct NamedConfigs<T>(pub Vec<NamedConfig<T>>);

nativelink-config/src/stores.rs

@@ -524,7 +524,7 @@ pub struct FastSlowSpec {
     pub slow: StoreSpec,
 }
 
-#[derive(Serialize, Deserialize, Debug, Default, Clone)]
+#[derive(Serialize, Deserialize, Debug, Default, Clone, Copy)]
 #[serde(deny_unknown_fields)]
 pub struct MemorySpec {
     /// Policy used to evict items out of the store. Failure to set this
@@ -664,7 +664,7 @@ pub struct Lz4Config {
     pub max_decode_block_size: u32,
 }
 
-#[derive(Serialize, Deserialize, Debug, PartialEq, Clone)]
+#[derive(Serialize, Deserialize, Debug, PartialEq, Clone, Copy)]
 #[serde(rename_all = "snake_case")]
 pub enum CompressionAlgorithm {
     /// LZ4 compression algorithm is extremely fast for compression and
@@ -695,7 +695,7 @@ pub struct CompressionSpec {
 /// is touched it updates the timestamp. Inserts and updates will execute the
 /// eviction policy removing any expired entries and/or the oldest entries
 /// until the store size becomes smaller than `max_bytes`.
-#[derive(Serialize, Deserialize, Debug, Default, Clone)]
+#[derive(Serialize, Deserialize, Debug, Default, Clone, Copy)]
 #[serde(deny_unknown_fields)]
 pub struct EvictionPolicy {
     /// Maximum number of bytes before eviction takes place.
@@ -851,7 +851,7 @@ pub struct GrpcSpec {
 }
 
 /// The possible error codes that might occur on an upstream request.
-#[derive(Serialize, Deserialize, Clone, Debug, PartialEq)]
+#[derive(Serialize, Deserialize, Clone, Copy, Debug, PartialEq)]
 pub enum ErrorCode {
     Cancelled = 1,
     Unknown = 2,
@@ -991,7 +991,7 @@ pub struct RedisSpec {
     pub retry: Retry,
 }
 
-#[derive(Debug, Default, Deserialize, Serialize, Clone, PartialEq, Eq)]
+#[derive(Debug, Default, Deserialize, Serialize, Clone, Copy, PartialEq, Eq)]
 #[serde(rename_all = "snake_case")]
 pub enum RedisMode {
     Cluster,
@@ -1000,7 +1000,7 @@ pub enum RedisMode {
     Standard,
 }
 
-#[derive(Clone, Debug, Default, Deserialize, Serialize)]
+#[derive(Clone, Copy, Debug, Default, Deserialize, Serialize)]
 pub struct NoopSpec {}
 
 /// Retry configuration. This configuration is exponential and each iteration

nativelink-error/Cargo.toml

@@ -1,3 +1,5 @@
+lints.workspace = true
+
 [package]
 name = "nativelink-error"
 version = "0.6.0"

nativelink-error/src/lib.rs

@@ -249,15 +249,15 @@ pub trait ResultExt<T> {
     fn err_tip_with_code<F, S>(self, tip_fn: F) -> Result<T, Error>
     where
         Self: Sized,
-        S: std::string::ToString,
-        F: (std::ops::FnOnce(&Error) -> (Code, S)) + Sized;
+        S: ToString,
+        F: (FnOnce(&Error) -> (Code, S)) + Sized;
 
     #[inline]
     fn err_tip<F, S>(self, tip_fn: F) -> Result<T, Error>
     where
         Self: Sized,
-        S: std::string::ToString,
-        F: (std::ops::FnOnce() -> S) + Sized,
+        S: ToString,
+        F: (FnOnce() -> S) + Sized,
     {
         self.err_tip_with_code(|e| (e.code, tip_fn()))
     }
@@ -275,8 +275,8 @@ impl<T, E: Into<Error>> ResultExt<T> for Result<T, E> {
     fn err_tip_with_code<F, S>(self, tip_fn: F) -> Result<T, Error>
     where
         Self: Sized,
-        S: std::string::ToString,
-        F: (std::ops::FnOnce(&Error) -> (Code, S)) + Sized,
+        S: ToString,
+        F: (FnOnce(&Error) -> (Code, S)) + Sized,
     {
         self.map_err(|e| {
             let mut error: Error = e.into();
@@ -310,8 +310,8 @@ impl<T> ResultExt<T> for Option<T> {
     fn err_tip_with_code<F, S>(self, tip_fn: F) -> Result<T, Error>
     where
         Self: Sized,
-        S: std::string::ToString,
-        F: (std::ops::FnOnce(&Error) -> (Code, S)) + Sized,
+        S: ToString,
+        F: (FnOnce(&Error) -> (Code, S)) + Sized,
     {
         self.ok_or_else(|| {
             let mut error = Error {
@@ -375,7 +375,7 @@ impl ErrorKindExt for std::io::ErrorKind {
 }
 
 // Serde definition for tonic::Code. See: https://serde.rs/remote-derive.html
-#[derive(Serialize, Deserialize)]
+#[derive(Debug, Clone, Copy, Serialize, Deserialize)]
 #[serde(remote = "Code")]
 pub enum CodeDef {
     Ok = 0,

nativelink-macro/Cargo.toml

@@ -1,3 +1,5 @@
+lints.workspace = true
+
 [package]
 name = "nativelink-macro"
 version = "0.6.0"

nativelink-macro/src/lib.rs

@@ -12,8 +12,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-extern crate proc_macro;
-
 use proc_macro::TokenStream;
 use quote::quote;
 use syn::{ItemFn, parse_macro_input};