diff --git a/Cargo.lock b/Cargo.lock index c63a80f591ca..1d582470c329 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9832,6 +9832,7 @@ dependencies = [ "risingwave_pb", "risingwave_sqlparser", "similar", + "thiserror-ext", "tokio-postgres", "tracing", "tracing-subscriber", diff --git a/ci/Dockerfile b/ci/Dockerfile index 810a3289b66a..963a6fd9f3b9 100644 --- a/ci/Dockerfile +++ b/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 \ cargo-make@0.36.10 \ + cargo-dylint@2.6.0 dylint-link@2.6.0 \ sqllogictest-bin@0.19.1 \ sccache@0.7.4 \ && cargo cache -a \ diff --git a/ci/build-ci-image.sh b/ci/build-ci-image.sh index bbdc5d7bfa9e..0c851410f3c2 100755 --- a/ci/build-ci-image.sh +++ b/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}" diff --git a/ci/docker-compose.yml b/ci/docker-compose.yml index 0ebb9e77eeb2..045cc69d615c 100644 --- a/ci/docker-compose.yml +++ b/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 diff --git a/ci/scripts/check-dylint.sh b/ci/scripts/check-dylint.sh new file mode 100755 index 000000000000..77dc5dbff71f --- /dev/null +++ b/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 diff --git a/ci/scripts/common.sh b/ci/scripts/common.sh old mode 100644 new mode 100755 diff --git a/ci/scripts/docker-hdfs.sh b/ci/scripts/docker-hdfs.sh old mode 100644 new mode 100755 diff --git a/ci/scripts/e2e-elasticsearch-sink-test.sh b/ci/scripts/e2e-elasticsearch-sink-test.sh old mode 100644 new mode 100755 diff --git a/ci/scripts/e2e-sink-test.sh b/ci/scripts/e2e-sink-test.sh index c20f422532b7..300012680a19 100755 --- a/ci/scripts/e2e-sink-test.sh +++ b/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" diff --git a/ci/scripts/pr.env.sh b/ci/scripts/pr.env.sh old mode 100644 new mode 100755 diff --git a/ci/workflows/pull-request.yml b/ci/workflows/pull-request.yml index 0e8e015dc68d..b5b2bd018e06 100644 --- a/ci/workflows/pull-request.yml +++ b/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: | diff --git a/lints/Cargo.toml b/lints/Cargo.toml index 6b8f4bbc3d8f..7974f6970bd9 100644 --- a/lints/Cargo.toml +++ b/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" diff --git a/lints/README.md b/lints/README.md index ba8a7aebb921..5007474227ab 100644 --- a/lints/README.md +++ b/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 diff --git a/lints/src/lib.rs b/lints/src/lib.rs index 264e0d47b62a..d2c78515272f 100644 --- a/lints/src/lib.rs +++ b/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::::default() }); + // Actual lints. lint_store.register_lints(&[format_error::FORMAT_ERROR]); lint_store.register_late_pass(|_| Box::::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); } diff --git a/src/frontend/src/handler/create_sql_function.rs b/src/frontend/src/handler/create_sql_function.rs index 66429c19fad1..166e940b6081 100644 --- a/src/frontend/src/handler/create_sql_function.rs +++ b/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 { diff --git a/src/meta/src/hummock/manager/checkpoint.rs b/src/meta/src/hummock/manager/checkpoint.rs index 81fc47330ef2..8f3a7cdff695 100644 --- a/src/meta/src/hummock/manager/checkpoint.rs +++ b/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 ); } diff --git a/src/storage/src/monitor/traced_store.rs b/src/storage/src/monitor/traced_store.rs index 7821572076bb..afd2e4e0da92 100644 --- a/src/storage/src/monitor/traced_store.rs +++ b/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 TracedStateStoreIter { 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 diff --git a/src/tests/sqlsmith/Cargo.toml b/src/tests/sqlsmith/Cargo.toml index 29a1ee26c91d..8e76dc20d71b 100644 --- a/src/tests/sqlsmith/Cargo.toml +++ b/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" diff --git a/src/tests/sqlsmith/tests/frontend/mod.rs b/src/tests/sqlsmith/tests/frontend/mod.rs index 8cb869aaf0e6..5c58f028b933 100644 --- a/src/tests/sqlsmith/tests/frontend/mod.rs +++ b/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 = std::result::Result; @@ -48,7 +49,7 @@ async fn handle(session: Arc, stmt: Statement, sql: Arc) -> 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(())