Switch to Cargo.toml based lint configuration (#730)

As of the Cargo included in Rust 1.74, lints can now be configured in `Cargo.toml` across whole crates/workspaces: https://blog.rust-lang.org/2023/11/16/Rust-1.74.0.html https://doc.rust-lang.org/stable/cargo/reference/manifest.html#the-lints-section https://doc.rust-lang.org/stable/cargo/reference/workspaces.html#the-lints-table This reduces the boilerplate, and the chance that we forget to enable lints in some targets. The only thing we need to remember is to add the `[lints] workspace = true` to any new crates in the future. Making this switch exposed a few places where lints weren't enabled and issues had been missed, which have been fixed now. Since this feature requires Rust 1.74, the MSRV has also been bumped (however, libcnb 0.16.0 already requires Rust 1.74, so in practice this is a no-op). GUS-W-14523836.
heroku · Nov 20, 2023 · cf5e184 · cf5e184
1 parent 9ef33d5
commit cf5e184
Show file tree

Hide file tree

Showing 35 changed files with 132 additions and 96 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -14,10 +14,19 @@ members = [
 
 [workspace.package]
 version = "0.0.0"
-rust-version = "1.67"
+rust-version = "1.74"
 edition = "2021"
 publish = false
 
+[workspace.lints.rust]
+unused_crate_dependencies = "warn"
+
+[workspace.lints.clippy]
+pedantic = "warn"
+missing_errors_doc = "allow"
+missing_panics_doc = "allow"
+module_name_repetitions = "allow"
+
 [workspace.dependencies]
 heroku-nodejs-utils = { path = "./common/nodejs-utils" }
 test_support = { path = "./test_support" }

diff --git a/buildpacks/nodejs-corepack/Cargo.toml b/buildpacks/nodejs-corepack/Cargo.toml
@@ -6,6 +6,9 @@ rust-version.workspace = true
 edition.workspace = true
 publish.workspace = true
 
+[lints]
+workspace = true
+
 [dependencies]
 heroku-nodejs-utils.workspace = true
 indoc = "2"

diff --git a/buildpacks/nodejs-corepack/src/main.rs b/buildpacks/nodejs-corepack/src/main.rs
@@ -1,8 +1,3 @@
-#![warn(unused_crate_dependencies)]
-#![warn(clippy::pedantic)]
-#![warn(clippy::cargo)]
-#![allow(clippy::module_name_repetitions)]
-
 use heroku_nodejs_utils::package_json::{PackageJson, PackageJsonError};
 use heroku_nodejs_utils::telemetry::init_tracer;
 use layers::{ManagerLayer, ShimLayer};

diff --git a/buildpacks/nodejs-corepack/tests/integration_test.rs b/buildpacks/nodejs-corepack/tests/integration_test.rs
@@ -1,4 +1,5 @@
-#![warn(clippy::pedantic)]
+// Required due to: https://github.com/rust-lang/rust/issues/95513
+#![allow(unused_crate_dependencies)]
 
 use libcnb_test::assert_contains;
 use test_support::{nodejs_integration_test_with_config, set_package_manager};

diff --git a/buildpacks/nodejs-engine/Cargo.toml b/buildpacks/nodejs-engine/Cargo.toml
@@ -6,6 +6,9 @@ rust-version.workspace = true
 edition.workspace = true
 publish.workspace = true
 
+[lints]
+workspace = true
+
 [dependencies]
 heroku-nodejs-utils.workspace = true
 libcnb = "=0.16.0"

diff --git a/buildpacks/nodejs-engine/src/bin/web_env.rs b/buildpacks/nodejs-engine/src/bin/web_env.rs
@@ -1,4 +1,5 @@
-#![warn(clippy::pedantic)]
+// Required due to: https://github.com/rust-lang/rust/issues/95513
+#![allow(unused_crate_dependencies)]
 
 use libcnb::data::exec_d::ExecDProgramOutputKey;
 use libcnb::data::exec_d_program_output_key;

diff --git a/buildpacks/nodejs-engine/src/main.rs b/buildpacks/nodejs-engine/src/main.rs
@@ -1,8 +1,3 @@
-#![warn(unused_crate_dependencies)]
-#![warn(clippy::pedantic)]
-#![warn(clippy::cargo)]
-#![allow(clippy::module_name_repetitions)]
-
 use crate::layers::{DistLayer, DistLayerError, WebEnvLayer};
 use heroku_nodejs_utils::inv::Inventory;
 use heroku_nodejs_utils::package_json::{PackageJson, PackageJsonError};

diff --git a/buildpacks/nodejs-engine/tests/integration_test.rs b/buildpacks/nodejs-engine/tests/integration_test.rs
@@ -1,4 +1,5 @@
-#![warn(clippy::pedantic)]
+// Required due to: https://github.com/rust-lang/rust/issues/95513
+#![allow(unused_crate_dependencies)]
 
 use libcnb_test::assert_contains;
 use test_support::{

diff --git a/buildpacks/nodejs-function-invoker/Cargo.toml b/buildpacks/nodejs-function-invoker/Cargo.toml
@@ -6,6 +6,9 @@ rust-version.workspace = true
 edition.workspace = true
 publish.workspace = true
 
+[lints]
+workspace = true
+
 [dependencies]
 heroku-nodejs-utils.workspace = true
 libcnb = "=0.16.0"

diff --git a/buildpacks/nodejs-function-invoker/src/main.rs b/buildpacks/nodejs-function-invoker/src/main.rs
@@ -1,8 +1,3 @@
-#![warn(unused_crate_dependencies)]
-#![warn(clippy::pedantic)]
-#![warn(clippy::cargo)]
-#![allow(clippy::module_name_repetitions)]
-
 use crate::function::{
     get_declared_runtime_package_version, get_main, is_function, ExplicitRuntimeDependencyError,
     MainError,

diff --git a/buildpacks/nodejs-function-invoker/tests/integration_test.rs b/buildpacks/nodejs-function-invoker/tests/integration_test.rs
@@ -1,4 +1,5 @@
-#![warn(clippy::pedantic)]
+// Required due to: https://github.com/rust-lang/rust/issues/95513
+#![allow(unused_crate_dependencies)]
 
 use base64::Engine;
 use libcnb_test::{assert_contains, assert_not_contains, TestContext};

diff --git a/buildpacks/nodejs-npm-engine/Cargo.toml b/buildpacks/nodejs-npm-engine/Cargo.toml
@@ -6,6 +6,9 @@ rust-version.workspace = true
 edition.workspace = true
 publish.workspace = true
 
+[lints]
+workspace = true
+
 [dependencies]
 commons = { git = "https://github.com/heroku/buildpacks-ruby", branch = "main" }
 fun_run = "0.1"
@@ -14,7 +17,6 @@ indoc = "2"
 libcnb = "=0.16.0"
 libherokubuildpack = "=0.16.0"
 serde = "1"
-tempfile = "3"
 toml = "0.8"
 
 [dev-dependencies]

diff --git a/buildpacks/nodejs-npm-engine/src/errors.rs b/buildpacks/nodejs-npm-engine/src/errors.rs
@@ -7,7 +7,6 @@ use commons::output::fmt::DEBUG_INFO;
 use heroku_nodejs_utils::package_json::PackageJsonError;
 use heroku_nodejs_utils::vrs::Requirement;
 use indoc::formatdoc;
-use libcnb::Error;
 use std::fmt::Display;
 use std::io::stdout;
 
@@ -29,23 +28,25 @@ pub(crate) enum NpmEngineBuildpackError {
     NpmVersion(npm::VersionError),
 }
 
-pub(crate) fn on_error(error: Error<NpmEngineBuildpackError>) {
+pub(crate) fn on_error(error: libcnb::Error<NpmEngineBuildpackError>) {
     let logger = BuildLog::new(stdout()).without_buildpack_name();
     match error {
-        Error::BuildpackError(buildpack_error) => on_buildpack_error(buildpack_error, logger),
-        framework_error => on_framework_error(framework_error, logger),
+        libcnb::Error::BuildpackError(buildpack_error) => {
+            on_buildpack_error(buildpack_error, logger);
+        }
+        framework_error => on_framework_error(&framework_error, logger),
     }
 }
 
 fn on_buildpack_error(error: NpmEngineBuildpackError, logger: Box<dyn StartedLogger>) {
     match error {
         NpmEngineBuildpackError::PackageJson(e) => on_package_json_error(e, logger),
         NpmEngineBuildpackError::MissingNpmEngineRequirement => {
-            on_missing_npm_engine_requirement_error(logger)
+            on_missing_npm_engine_requirement_error(logger);
         }
-        NpmEngineBuildpackError::InventoryParse(e) => on_inventory_parse_error(e, logger),
+        NpmEngineBuildpackError::InventoryParse(e) => on_inventory_parse_error(&e, logger),
         NpmEngineBuildpackError::NpmVersionResolve(requirement) => {
-            on_npm_version_resolve_error(requirement, logger)
+            on_npm_version_resolve_error(&requirement, logger);
         }
         NpmEngineBuildpackError::NpmEngineLayer(e) => on_npm_engine_layer_error(e, logger),
         NpmEngineBuildpackError::NodeVersion(e) => on_node_version_error(e, logger),
@@ -97,7 +98,7 @@ fn on_missing_npm_engine_requirement_error(logger: Box<dyn StartedLogger>) {
     ", engines_key = fmt::value("engines.npm"), package_json = fmt::value("package.json") });
 }
 
-fn on_inventory_parse_error(error: toml::de::Error, logger: Box<dyn StartedLogger>) {
+fn on_inventory_parse_error(error: &toml::de::Error, logger: Box<dyn StartedLogger>) {
     print_error_details(logger, &error)
         .announce()
         .error(&formatdoc! {"
@@ -108,10 +109,10 @@ fn on_inventory_parse_error(error: toml::de::Error, logger: Box<dyn StartedLogge
             {USE_DEBUG_INFORMATION_AND_RETRY_BUILD}
 
             {SUBMIT_AN_ISSUE}
-        ", npm = fmt::value("npm") })
+        ", npm = fmt::value("npm") });
 }
 
-fn on_npm_version_resolve_error(requirement: Requirement, logger: Box<dyn StartedLogger>) {
+fn on_npm_version_resolve_error(requirement: &Requirement, logger: Box<dyn StartedLogger>) {
     logger.announce().error(&formatdoc! {"
             Error resolving requested {npm} version {requested_version}.
             
@@ -131,7 +132,7 @@ fn on_npm_version_resolve_error(requirement: Requirement, logger: Box<dyn Starte
         requested_version = fmt::value(requirement.to_string()),
         package_json = fmt::value("package.json"),
         engines_key = fmt::value("engines.npm")
-    })
+    });
 }
 
 fn on_npm_engine_layer_error(error: NpmEngineLayerError, logger: Box<dyn StartedLogger>) {
@@ -254,7 +255,10 @@ fn on_npm_version_error(error: npm::VersionError, logger: Box<dyn StartedLogger>
     }
 }
 
-fn on_framework_error(error: Error<NpmEngineBuildpackError>, logger: Box<dyn StartedLogger>) {
+fn on_framework_error(
+    error: &libcnb::Error<NpmEngineBuildpackError>,
+    logger: Box<dyn StartedLogger>,
+) {
     print_error_details(logger, &error)
         .announce()
         .error(&formatdoc! {"

diff --git a/buildpacks/nodejs-npm-engine/src/layers/npm_engine.rs b/buildpacks/nodejs-npm-engine/src/layers/npm_engine.rs
@@ -2,7 +2,7 @@ use crate::errors::NpmEngineBuildpackError;
 use crate::NpmEngineBuildpack;
 use commons::output::fmt;
 use commons::output::section_log::{log_step, log_step_timed, SectionLogger};
-use fun_run::CommandWithName;
+use fun_run::{CommandWithName, NamedOutput};
 use heroku_nodejs_utils::inv::Release;
 use heroku_nodejs_utils::vrs::Version;
 use libcnb::build::BuildContext;
@@ -84,7 +84,7 @@ fn download_and_unpack_release(
     log_step_timed(format!("Downloading {}", fmt::value(download_from)), || {
         download_file(download_from, download_to)
             .map_err(NpmEngineLayerError::Download)
-            .and_then(|_| File::open(download_to).map_err(NpmEngineLayerError::OpenTarball))
+            .and_then(|()| File::open(download_to).map_err(NpmEngineLayerError::OpenTarball))
             .and_then(|mut npm_tgz_file| {
                 decompress_tarball(&mut npm_tgz_file, unpack_into)
                     .map_err(NpmEngineLayerError::DecompressTarball)
@@ -103,7 +103,7 @@ fn remove_existing_npm_installation(npm_cli_script: &Path) -> Result<(), NpmEngi
             "--loglevel=silent",
         ])
         .named_output()
-        .and_then(|cmd| cmd.nonzero_captured())
+        .and_then(NamedOutput::nonzero_captured)
         .map_err(NpmEngineLayerError::RemoveExistingNpmInstall)
         .map(|_| ())
 }
@@ -118,7 +118,7 @@ fn install_npm_package(npm_cli_script: &Path, package: &Path) -> Result<(), NpmE
             &package.to_string_lossy(),
         ])
         .named_output()
-        .and_then(|cmd| cmd.nonzero_captured())
+        .and_then(NamedOutput::nonzero_captured)
         .map_err(NpmEngineLayerError::InstallNpm)
         .map(|_| ())
 }

diff --git a/buildpacks/nodejs-npm-engine/src/main.rs b/buildpacks/nodejs-npm-engine/src/main.rs
@@ -8,7 +8,7 @@ use crate::layers::npm_engine::NpmEngineLayer;
 use commons::output::build_log::{BuildLog, Logger, SectionLogger};
 use commons::output::fmt;
 use commons::output::section_log::log_step;
-use fun_run::CommandWithName;
+use fun_run::{CommandWithName, NamedOutput};
 use heroku_nodejs_utils::inv::{Inventory, Release};
 use heroku_nodejs_utils::package_json::PackageJson;
 use heroku_nodejs_utils::vrs::{Requirement, Version};
@@ -18,8 +18,14 @@ use libcnb::data::layer_name;
 use libcnb::detect::{DetectContext, DetectResult, DetectResultBuilder};
 use libcnb::generic::{GenericMetadata, GenericPlatform};
 use libcnb::{buildpack_main, Buildpack, Env};
+#[cfg(test)]
+use libcnb_test as _;
+#[cfg(test)]
+use serde_json as _;
 use std::io::stdout;
 use std::process::Command;
+#[cfg(test)]
+use test_support as _;
 
 pub(crate) const BUILDPACK_NAME: &str = "Heroku Node.js npm Engine Buildpack";
 
@@ -118,11 +124,11 @@ fn install_npm_release(
     npm_release: Release,
     context: &BuildContext<NpmEngineBuildpack>,
     env: &Env,
-    _section_logger: &dyn SectionLogger,
+    section_logger: &dyn SectionLogger,
 ) -> Result<(), libcnb::Error<NpmEngineBuildpackError>> {
     let node_version = Command::from(node::Version { env })
         .named_output()
-        .and_then(|output| output.nonzero_captured())
+        .and_then(NamedOutput::nonzero_captured)
         .map_err(node::VersionError::Command)
         .and_then(|output| {
             let stdout = output.stdout_lossy();
@@ -137,7 +143,7 @@ fn install_npm_release(
         NpmEngineLayer {
             npm_release,
             node_version,
-            _section_logger,
+            _section_logger: section_logger,
         },
     )?;
 
@@ -150,7 +156,7 @@ fn log_installed_npm_version(
 ) -> Result<(), NpmEngineBuildpackError> {
     Command::from(npm::Version { env })
         .named_output()
-        .and_then(|output| output.nonzero_captured())
+        .and_then(NamedOutput::nonzero_captured)
         .map_err(npm::VersionError::Command)
         .and_then(|output| {
             let stdout = output.stdout_lossy();

diff --git a/buildpacks/nodejs-npm-engine/tests/integration_test.rs b/buildpacks/nodejs-npm-engine/tests/integration_test.rs
@@ -1,3 +1,6 @@
+// Required due to: https://github.com/rust-lang/rust/issues/95513
+#![allow(unused_crate_dependencies)]
+
 use libcnb_test::assert_contains;
 use std::fs;
 use std::path::Path;
@@ -30,7 +33,7 @@ fn test_npm_engine_caching() {
         ctx.rebuild(config, |ctx| {
             assert_contains!(ctx.pack_stdout, "Using cached npm");
             assert_contains!(ctx.pack_stdout, "Successfully installed `npm@9.6.6`");
-        })
+        });
     });
 }
 

diff --git a/buildpacks/nodejs-npm-install/Cargo.toml b/buildpacks/nodejs-npm-install/Cargo.toml
@@ -6,18 +6,18 @@ rust-version.workspace = true
 edition.workspace = true
 publish.workspace = true
 
+[lints]
+workspace = true
+
 [dependencies]
 commons = { git = "https://github.com/heroku/buildpacks-ruby", branch = "main" }
 fun_run = "0.1"
 heroku-nodejs-utils.workspace = true
 indoc = "2"
 libcnb = "=0.16.0"
-libherokubuildpack = "=0.16.0"
 serde = "1"
-toml = "0.8"
 
 [dev-dependencies]
 libcnb-test = "=0.16.0"
 serde_json = "1"
 test_support.workspace = true
-ureq = "2"