feat(ci): integrate with cargo-dylint (#14713)

Signed-off-by: Bugen Zhao <[email protected]>

ci/Dockerfile

@@ -48,6 +48,7 @@ ENV CARGO_REGISTRIES_CRATES_IO_PROTOCOL=sparse
 RUN curl -L --proto '=https' --tlsv1.2 -sSf https://raw.githubusercontent.com/cargo-bins/cargo-binstall/main/install-from-binstall-release.sh | bash
 RUN cargo binstall -y --no-symlinks cargo-llvm-cov cargo-nextest cargo-hakari cargo-sort cargo-cache cargo-audit \
     [email protected] \
+    [email protected] [email protected] \
     [email protected] \
     [email protected] \
     && cargo cache -a \

ci/build-ci-image.sh

@@ -10,7 +10,7 @@ cat ../rust-toolchain
 # shellcheck disable=SC2155
 
 # REMEMBER TO ALSO UPDATE ci/docker-compose.yml
-export BUILD_ENV_VERSION=v20240104_1
+export BUILD_ENV_VERSION=v20240122_1
 
 export BUILD_TAG="public.ecr.aws/x5u3w5h6/rw-build-env:${BUILD_ENV_VERSION}"
 

ci/docker-compose.yml

@@ -71,7 +71,7 @@ services:
       retries: 5
 
   source-test-env:
-    image: public.ecr.aws/x5u3w5h6/rw-build-env:v20240104_1
+    image: public.ecr.aws/x5u3w5h6/rw-build-env:v20240122_1
     depends_on:
       - mysql
       - db
@@ -81,7 +81,7 @@ services:
       - ..:/risingwave
 
   sink-test-env:
-    image: public.ecr.aws/x5u3w5h6/rw-build-env:v20240104_1
+    image: public.ecr.aws/x5u3w5h6/rw-build-env:v20240122_1
     depends_on:
       - mysql
       - db
@@ -93,12 +93,12 @@ services:
       - ..:/risingwave
 
   rw-build-env:
-    image: public.ecr.aws/x5u3w5h6/rw-build-env:v20240104_1
+    image: public.ecr.aws/x5u3w5h6/rw-build-env:v20240122_1
     volumes:
       - ..:/risingwave
 
   ci-flamegraph-env:
-    image: public.ecr.aws/x5u3w5h6/rw-build-env:v20240104_1
+    image: public.ecr.aws/x5u3w5h6/rw-build-env:v20240122_1
     # NOTE(kwannoel): This is used in order to permit
     # syscalls for `nperf` (perf_event_open),
     # so it can do CPU profiling.
@@ -109,7 +109,7 @@ services:
       - ..:/risingwave
 
   regress-test-env:
-    image: public.ecr.aws/x5u3w5h6/rw-build-env:v20240104_1
+    image: public.ecr.aws/x5u3w5h6/rw-build-env:v20240122_1
     depends_on:
       db:
         condition: service_healthy

ci/scripts/check-dylint.sh

@@ -0,0 +1,14 @@
+#!/usr/bin/env bash
+
+# Exits as soon as any line fails.
+set -euo pipefail
+
+source ci/scripts/common.sh
+unset RUSTC_WRAPPER # disable sccache, see https://github.com/mozilla/sccache/issues/861
+
+echo "--- Run dylint check (dev, all features)"
+# Instead of `-D warnings`, we only deny warnings from our own lints. This is because...
+# - Warnings from `check` or `clippy` are already checked in `check.sh`.
+# - The toolchain used for linting could be slightly different from the one used to
+#   compile RisingWave. Warnings from `rustc` itself may produce false positives.
+DYLINT_RUSTFLAGS="-A warnings -D rw_warnings" cargo dylint --all -- --all-targets --all-features --locked

ci/scripts/e2e-sink-test.sh

@@ -109,7 +109,6 @@ else
 fi
 
 echo "--- testing elasticsearch sink"
-chmod +x ./ci/scripts/e2e-elasticsearch-sink-test.sh
 ./ci/scripts/e2e-elasticsearch-sink-test.sh
 if [ $? -eq 0 ]; then
   echo "elasticsearch sink check passed"

ci/workflows/pull-request.yml

@@ -370,6 +370,31 @@ steps:
     timeout_in_minutes: 25
     retry: *auto-retry
 
+  - label: "check dylint"
+    command: "ci/scripts/check-dylint.sh"
+    if: |
+      !(build.pull_request.labels includes "ci/skip-ci") && build.env("CI_STEPS") == null
+      || build.pull_request.labels includes "ci/run-check"
+      || build.env("CI_STEPS") =~ /(^|,)check(,|$$)/
+    plugins:
+      - gencer/cache#v2.4.10:
+          id: cache
+          key: "v1-cache-{{ id }}-{{ runner.os }}-{{ checksum 'Cargo.lock' }}"
+          restore-keys:
+            - "v1-cache-{{ id }}-{{ runner.os }}-"
+            - "v1-cache-{{ id }}-"
+          backend: s3
+          s3:
+            bucket: ci-cache-bucket
+            args: "--no-progress"
+          paths:
+            - ".cargo/advisory-db"
+      - docker-compose#v4.9.0:
+          run: rw-build-env
+          config: ci/docker-compose.yml
+    timeout_in_minutes: 25
+    retry: *auto-retry
+
   - label: "unit test (deterministic simulation)"
     command: "ci/scripts/deterministic-unit-test.sh"
     if: |

lints/Cargo.toml

@@ -12,6 +12,7 @@ name = "format_error"
 path = "ui/format_error.rs"
 
 # See `README.md` before bumping the version.
+# Remember to update the version in `ci/Dockerfile` as well.
 [dependencies]
 clippy_utils = { git = "https://github.com/rust-lang/rust-clippy", rev = "6fd0258e45105161b7e759a22e7350958e5cb0b1" }
 dylint_linting = "2.6.0"

lints/README.md

@@ -7,7 +7,7 @@ See [cargo dylint](https://github.com/trailofbits/dylint) for more information.
 ## Install `cargo-dylint`
 
 ```bash
-cargo install dylint
+cargo install cargo-dylint dylint-link
 ```
 
 ## Run lints

lints/src/lib.rs

@@ -32,10 +32,39 @@ dylint_linting::dylint_library!();
 #[allow(clippy::no_mangle_with_rust_abi)]
 #[no_mangle]
 pub fn register_lints(_sess: &rustc_session::Session, lint_store: &mut rustc_lint::LintStore) {
+    // -- Begin lint registration --
+
+    // Preparation steps.
     lint_store.register_early_pass(|| {
         Box::<utils::format_args_collector::FormatArgsCollector>::default()
     });
 
+    // Actual lints.
     lint_store.register_lints(&[format_error::FORMAT_ERROR]);
     lint_store.register_late_pass(|_| Box::<format_error::FormatError>::default());
+
+    // --  End lint registration  --
+
+    // Register lints into groups.
+    // Note: use `rw_` instead of `rw::` to avoid "error[E0602]: unknown lint tool: `rw`".
+    register_group(lint_store, "rw_all", |_| true);
+    register_group(lint_store, "rw_warnings", |l| {
+        l.default_level >= rustc_lint::Level::Warn
+    });
+}
+
+fn register_group(
+    lint_store: &mut rustc_lint::LintStore,
+    name: &'static str,
+    filter_predicate: impl Fn(&rustc_lint::Lint) -> bool,
+) {
+    let lints = lint_store
+        .get_lints()
+        .iter()
+        .filter(|l| l.name.starts_with("rw::"))
+        .filter(|l| filter_predicate(l))
+        .map(|l| rustc_lint::LintId::of(l))
+        .collect();
+
+    lint_store.register_group(true, name, None, lints);
 }

src/frontend/src/handler/create_sql_function.rs

@@ -24,6 +24,7 @@ use risingwave_sqlparser::ast::{
     CreateFunctionBody, FunctionDefinition, ObjectName, OperateFunctionArg,
 };
 use risingwave_sqlparser::parser::{Parser, ParserError};
+use thiserror_ext::AsReport;
 
 use super::*;
 use crate::binder::UdfContext;
@@ -176,9 +177,10 @@ pub async fn handle_create_sql_function(
 
         if let Ok(expr) = UdfContext::extract_udf_expression(ast) {
             if let Err(e) = binder.bind_expr(expr) {
-                return Err(ErrorCode::InvalidInputSyntax(
-                    format!("failed to conduct semantic check, please see if you are calling non-existence functions.\nDetailed error: {e}")
-                )
+                return Err(ErrorCode::InvalidInputSyntax(format!(
+                    "failed to conduct semantic check, please see if you are calling non-existence functions: {}",
+                    e.as_report()
+                ))
                 .into());
             }
         } else {

src/meta/src/hummock/manager/checkpoint.rs

@@ -25,6 +25,7 @@ use risingwave_hummock_sdk::version::HummockVersion;
 use risingwave_hummock_sdk::HummockVersionId;
 use risingwave_pb::hummock::hummock_version_checkpoint::{PbStaleObjects, StaleObjects};
 use risingwave_pb::hummock::{PbHummockVersionArchive, PbHummockVersionCheckpoint};
+use thiserror_ext::AsReport;
 
 use crate::hummock::error::Result;
 use crate::hummock::manager::{read_lock, write_lock};
@@ -178,7 +179,8 @@ impl HummockManager {
         if let Some(archive) = archive {
             if let Err(e) = self.write_version_archive(&archive).await {
                 tracing::warn!(
-                    "failed to write version archive {}, {e}",
+                    error = %e.as_report(),
+                    "failed to write version archive {}",
                     archive.version.as_ref().unwrap().id
                 );
             }

src/storage/src/monitor/traced_store.rs

@@ -23,6 +23,7 @@ use risingwave_hummock_trace::{
     init_collector, should_use_trace, ConcurrentId, MayTraceSpan, OperationResult, StorageType,
     TraceResult, TraceSpan, TracedBytes, TracedSealCurrentEpochOptions, LOCAL_ID,
 };
+use thiserror_ext::AsReport;
 
 use super::identity;
 use crate::error::{StorageError, StorageResult};
@@ -357,7 +358,7 @@ impl<S: StateStoreIterItemStream> TracedStateStoreIter<S> {
         while let Some((key, value)) = inner
             .try_next()
             .await
-            .inspect_err(|e| tracing::error!("Failed in next: {:?}", e))?
+            .inspect_err(|e| tracing::error!(error = %e.as_report(), "Failed in next"))?
         {
             self.span.may_send_iter_next();
             self.span

src/tests/sqlsmith/Cargo.toml

@@ -28,6 +28,7 @@ risingwave_frontend = { workspace = true }
 risingwave_pb = { workspace = true }
 risingwave_sqlparser = { workspace = true }
 similar = "2.4.0"
+thiserror-ext = { workspace = true }
 tokio = { version = "0.2", package = "madsim-tokio" }
 tokio-postgres = "0.7"
 tracing = "0.1"

src/tests/sqlsmith/tests/frontend/mod.rs

@@ -28,6 +28,7 @@ use risingwave_sqlparser::ast::Statement;
 use risingwave_sqlsmith::{
     is_permissible_error, mview_sql_gen, parse_create_table_statements, parse_sql, sql_gen, Table,
 };
+use thiserror_ext::AsReport;
 use tokio::runtime::Runtime;
 
 type Result<T> = std::result::Result<T, Failed>;
@@ -48,7 +49,7 @@ async fn handle(session: Arc<SessionImpl>, stmt: Statement, sql: Arc<str>) -> Re
     let result = handler::handle(session.clone(), stmt, sql, vec![])
         .await
         .map(|_| ())
-        .map_err(|e| format!("Error Reason:\n{}", e).into());
+        .map_err(|e| format!("Error Reason:\n{}", e.as_report()).into());
     validate_result(result)
 }
 
@@ -183,20 +184,26 @@ fn run_batch_query(
     let mut binder = Binder::new(&session);
     let bound = binder
         .bind(stmt)
-        .map_err(|e| Failed::from(format!("Failed to bind:\nReason:\n{}", e)))?;
+        .map_err(|e| Failed::from(format!("Failed to bind:\nReason:\n{}", e.as_report())))?;
     let mut planner = Planner::new(context);
-    let mut logical_plan = planner
-        .plan(bound)
-        .map_err(|e| Failed::from(format!("Failed to generate logical plan:\nReason:\n{}", e)))?;
-    let batch_plan = logical_plan
-        .gen_batch_plan()
-        .map_err(|e| Failed::from(format!("Failed to generate batch plan:\nReason:\n{}", e)))?;
+    let mut logical_plan = planner.plan(bound).map_err(|e| {
+        Failed::from(format!(
+            "Failed to generate logical plan:\nReason:\n{}",
+            e.as_report()
+        ))
+    })?;
+    let batch_plan = logical_plan.gen_batch_plan().map_err(|e| {
+        Failed::from(format!(
+            "Failed to generate batch plan:\nReason:\n{}",
+            e.as_report()
+        ))
+    })?;
     logical_plan
         .gen_batch_distributed_plan(batch_plan)
         .map_err(|e| {
             Failed::from(format!(
                 "Failed to generate batch distributed plan:\nReason:\n{}",
-                e
+                e.as_report()
             ))
         })?;
     Ok(())