diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b869bd374..3a937ffe3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -180,7 +180,7 @@ jobs: uses: actions-rs/toolchain@v1 with: profile: minimal - toolchain: stable + toolchain: nightly-2023-12-28 default: true target: wasm32-unknown-unknown components: rust-src, clippy diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a780ff9a..2841fc3fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Add a user-friendly view of contract storage data in the form of a table - [#1414](https://github.com/paritytech/cargo-contract/pull/1414) +### Changed +- Mandatory dylint-based lints - [#1412](https://github.com/paritytech/cargo-contract/pull/1412) + ## [4.0.0-rc.1] ### Changed diff --git a/README.md b/README.md index bb074733f..581d0c5ed 100644 --- a/README.md +++ b/README.md @@ -42,7 +42,7 @@ Modern releases of gcc and clang, as well as Visual Studio 2019+ should work. - Step 2: `cargo install --force --locked cargo-contract`. -- Step 3: (**Optional**) Install `dylint` for linting. +- Step 3: Install `dylint` for linting. - (MacOS) `brew install openssl` - `cargo install cargo-dylint dylint-link`. @@ -163,7 +163,7 @@ Generate schema and print it to STDOUT. ##### `cargo contract verify-schema` -Verify a metadata file or a contract bundle containing metadata against the schema file. +Verify a metadata file or a contract bundle containing metadata against the schema file. ##### `cargo contract storage` diff --git a/build-image/Dockerfile b/build-image/Dockerfile index 1f2541039..2f528c1dc 100644 --- a/build-image/Dockerfile +++ b/build-image/Dockerfile @@ -91,6 +91,8 @@ RUN apt-get -y update && apt-get -y install gcc=${GCC_VERSION} g++=${G_VERSION} fi \ && echo "Executing ${COMMAND}" \ && eval "${COMMAND}" \ + && echo "Installing linting dependencies" \ + && cargo install cargo-dylint dylint-link \ # Cleanup after `cargo install` && rm -rf ${CARGO_HOME}/"registry" ${CARGO_HOME}/"git" /root/.cache/sccache \ # apt clean up diff --git a/crates/build/README.md b/crates/build/README.md index d789dfb0b..6e03a498c 100644 --- a/crates/build/README.md +++ b/crates/build/README.md @@ -32,7 +32,7 @@ let args = contract_build::ExecuteArgs { unstable_flags: UnstableFlags::default(), optimization_passes: Some(OptimizationPasses::default()), keep_debug_symbols: false, - dylint: false, + extra_lints: false, output_type: OutputType::Json, skip_wasm_validation: false, target: Target::Wasm, diff --git a/crates/build/src/lib.rs b/crates/build/src/lib.rs index e3fb0ca09..b6189f07c 100644 --- a/crates/build/src/lib.rs +++ b/crates/build/src/lib.rs @@ -94,9 +94,7 @@ use parity_wasm::elements::{ }; use semver::Version; use std::{ - collections::VecDeque, fs, - io, path::{ Path, PathBuf, @@ -112,6 +110,19 @@ pub const DEFAULT_MAX_MEMORY_PAGES: u32 = 16; /// Version of the currently executing `cargo-contract` binary. const VERSION: &str = env!("CARGO_PKG_VERSION"); +/// Configuration of the linting module. +/// +/// Ensure it is kept up-to-date when updating `cargo-contract`. +pub(crate) mod linting { + /// Toolchain used to build ink_linting: + /// https://github.com/paritytech/ink/blob/master/linting/rust-toolchain.toml + pub const TOOLCHAIN_VERSION: &str = "nightly-2023-12-28"; + /// Git repository with ink_linting libraries + pub const GIT_URL: &str = "https://github.com/paritytech/ink/"; + /// Git revision number of the linting crate + pub const GIT_REV: &str = "1c029a153ead15cd0bd76631613967c9e679e0c1"; +} + /// Arguments to use when executing `build` or `check` commands. #[derive(Clone)] pub struct ExecuteArgs { @@ -125,7 +136,7 @@ pub struct ExecuteArgs { pub unstable_flags: UnstableFlags, pub optimization_passes: Option, pub keep_debug_symbols: bool, - pub dylint: bool, + pub extra_lints: bool, pub output_type: OutputType, pub skip_wasm_validation: bool, pub target: Target, @@ -145,7 +156,7 @@ impl Default for ExecuteArgs { unstable_flags: Default::default(), optimization_passes: Default::default(), keep_debug_symbols: Default::default(), - dylint: Default::default(), + extra_lints: Default::default(), output_type: Default::default(), skip_wasm_validation: Default::default(), target: Default::default(), @@ -289,14 +300,8 @@ fn exec_cargo_for_onchain_target( "--target-dir={}", crate_metadata.target_directory.to_string_lossy() ); - - let mut args = vec![ - format!("--target={}", target.llvm_target()), - "-Zbuild-std=core,alloc".to_owned(), - "--no-default-features".to_owned(), - "--release".to_owned(), - target_dir, - ]; + let mut args = vec![target_dir, "--release".to_owned()]; + args.extend(onchain_cargo_options(target)); network.append_to_args(&mut args); let mut features = features.clone(); @@ -344,10 +349,13 @@ fn exec_cargo_for_onchain_target( env.push(("CARGO_ENCODED_RUSTFLAGS", Some(rustflags))); }; - let cargo = - util::cargo_cmd(command, &args, manifest_path.directory(), *verbosity, env); - - invoke_cargo_and_scan_for_error(cargo) + execute_cargo(util::cargo_cmd( + command, + &args, + manifest_path.directory(), + *verbosity, + env, + )) }; if unstable_flags.original_manifest { @@ -433,7 +441,7 @@ fn check_buffer_size_invoke_cargo_clean( "Detected a change in the configured buffer size. Rebuilding the project." .bold() ); - invoke_cargo_and_scan_for_error(cargo)?; + execute_cargo(cargo)?; } Err(_) => { verbose_eprintln!( @@ -443,7 +451,7 @@ fn check_buffer_size_invoke_cargo_clean( "Cannot find the previous size of the static buffer. Rebuilding the project." .bold() ); - invoke_cargo_and_scan_for_error(cargo)?; + execute_cargo(cargo)?; } } } @@ -452,59 +460,22 @@ fn check_buffer_size_invoke_cargo_clean( /// Executes the supplied cargo command, reading the output and scanning for known errors. /// Writes the captured stderr back to stderr and maintains the cargo tty progress bar. -fn invoke_cargo_and_scan_for_error(cargo: duct::Expression) -> Result<()> { - macro_rules! eprintln_red { - ($value:expr) => {{ - use colored::Colorize as _; - ::std::eprintln!("{}", $value.bright_red().bold()); - }}; +fn execute_cargo(cargo: duct::Expression) -> Result<()> { + match cargo.unchecked().run() { + Ok(out) if out.status.success() => Ok(()), + Ok(out) => anyhow::bail!(String::from_utf8_lossy(&out.stderr).to_string()), + Err(e) => anyhow::bail!("Cannot run `cargo` command: {:?}", e), } - - // unchecked: Even capture output on non exit return status - let cargo = util::cargo_tty_output(cargo).unchecked(); - - let missing_main_err = "error[E0601]".as_bytes(); - let mut err_buf = VecDeque::with_capacity(missing_main_err.len()); - - let mut reader = cargo.stderr_to_stdout().reader()?; - let mut buffer = [0u8; 1]; - - loop { - let bytes_read = io::Read::read(&mut reader, &mut buffer)?; - for byte in buffer[0..bytes_read].iter() { - err_buf.push_back(*byte); - if err_buf.len() > missing_main_err.len() { - let byte = err_buf.pop_front().expect("buffer is not empty"); - io::Write::write(&mut io::stderr(), &[byte])?; - } - } - if missing_main_err == err_buf.make_contiguous() { - eprintln_red!("\nExited with error: [E0601]"); - eprintln_red!( - "Your contract must be annotated with the `no_main` attribute.\n" - ); - eprintln_red!("Examples how to do this:"); - eprintln_red!(" - `#![cfg_attr(not(feature = \"std\"), no_std, no_main)]`"); - eprintln_red!(" - `#[no_main]`\n"); - return Err(anyhow::anyhow!("missing `no_main` attribute")) - } - if bytes_read == 0 { - // flush the remaining buffered bytes - io::Write::write(&mut io::stderr(), err_buf.make_contiguous())?; - break - } - buffer = [0u8; 1]; - } - Ok(()) } -/// Run linting steps which include `clippy` (mandatory) + `dylint` (optional). +/// Run linting that involves two steps: `clippy` and `dylint`. Both are mandatory as +/// they're part of the compilation process and implement security-critical features. fn lint( - dylint: bool, + extra_lints: bool, crate_metadata: &CrateMetadata, + target: &Target, verbosity: &Verbosity, ) -> Result<()> { - // mandatory: Always run clippy. verbose_eprintln!( verbosity, " {} {}", @@ -513,16 +484,19 @@ fn lint( ); exec_cargo_clippy(crate_metadata, *verbosity)?; - // optional: Dylint only on demand (for now). - if dylint { + // TODO (jubnzv): Dylint needs a custom toolchain installed by the user. Currently, + // it's required only for RiscV target. We're working on the toolchain integration + // and will make this step mandatory for all targets in future releases. + if extra_lints || matches!(target, Target::RiscV) { verbose_eprintln!( verbosity, " {} {}", "[==]".bold(), "Checking ink! linting rules".bright_green().bold() ); - exec_cargo_dylint(crate_metadata, *verbosity)?; + exec_cargo_dylint(extra_lints, crate_metadata, target, *verbosity)?; } + Ok(()) } @@ -537,7 +511,7 @@ fn exec_cargo_clippy(crate_metadata: &CrateMetadata, verbosity: Verbosity) -> Re "-Dclippy::arithmetic_side_effects", ]; // we execute clippy with the plain manifest no temp dir required - invoke_cargo_and_scan_for_error(util::cargo_cmd( + execute_cargo(util::cargo_cmd( "clippy", args, crate_metadata.manifest_path.directory(), @@ -546,11 +520,25 @@ fn exec_cargo_clippy(crate_metadata: &CrateMetadata, verbosity: Verbosity) -> Re )) } +/// Returns a list of cargo options used for on-chain builds +fn onchain_cargo_options(target: &Target) -> Vec { + vec![ + format!("--target={}", target.llvm_target()), + "-Zbuild-std=core,alloc".to_owned(), + "--no-default-features".to_owned(), + ] +} + /// Inject our custom lints into the manifest and execute `cargo dylint` . /// /// We create a temporary folder, extract the linting driver there and run /// `cargo dylint` with it. -fn exec_cargo_dylint(crate_metadata: &CrateMetadata, verbosity: Verbosity) -> Result<()> { +fn exec_cargo_dylint( + extra_lints: bool, + crate_metadata: &CrateMetadata, + target: &Target, + verbosity: Verbosity, +) -> Result<()> { check_dylint_requirements(crate_metadata.manifest_path.directory())?; // `dylint` is verbose by default, it doesn't have a `--verbose` argument, @@ -559,8 +547,20 @@ fn exec_cargo_dylint(crate_metadata: &CrateMetadata, verbosity: Verbosity) -> Re Verbosity::Default | Verbosity::Quiet => Verbosity::Quiet, }; + let mut args = if extra_lints { + vec![ + "--lib=ink_linting_mandatory".to_owned(), + "--lib=ink_linting".to_owned(), + ] + } else { + vec!["--lib=ink_linting_mandatory".to_owned()] + }; + args.push("--".to_owned()); + // Pass on-chain build options to ensure the linter expands all conditional `cfg_attr` + // macros, as it does for the release build. + args.extend(onchain_cargo_options(target)); + let target_dir = &crate_metadata.target_directory.to_string_lossy(); - let args = vec!["--lib=ink_linting"]; let env = vec![ // We need to set the `CARGO_TARGET_DIR` environment variable in // case `cargo dylint` is invoked. @@ -578,7 +578,7 @@ fn exec_cargo_dylint(crate_metadata: &CrateMetadata, verbosity: Verbosity) -> Re Workspace::new(&crate_metadata.cargo_meta, &crate_metadata.root_package.id)? .with_root_package_manifest(|manifest| { - manifest.with_dylint()?.with_empty_workspace(); + manifest.with_dylint()?; Ok(()) })? .using_temp(|manifest_path| { @@ -599,7 +599,8 @@ fn exec_cargo_dylint(crate_metadata: &CrateMetadata, verbosity: Verbosity) -> Re /// Checks if all requirements for `dylint` are installed. /// /// We require both `cargo-dylint` and `dylint-link` because the driver is being -/// built at runtime on demand. +/// built at runtime on demand. These must be built using a custom version of the +/// toolchain, as the linter utilizes the unstable rustc API. /// /// This function takes a `_working_dir` which is only used for unit tests. fn check_dylint_requirements(_working_dir: Option<&Path>) -> Result<()> { @@ -621,6 +622,29 @@ fn check_dylint_requirements(_working_dir: Option<&Path>) -> Result<()> { }) }; + // Check if the required toolchain is present and is installed with `rustup`. + if let Ok(output) = Command::new("rustup").arg("toolchain").arg("list").output() { + anyhow::ensure!( + String::from_utf8_lossy(&output.stdout).contains(linting::TOOLCHAIN_VERSION), + format!( + "Toolchain `{0}` was not found!\n\ + This specific version is required to provide additional source code analysis.\n\n + You can install it by executing `rustup install {0}`.", + linting::TOOLCHAIN_VERSION, + ) + .to_string() + .bright_yellow()); + } else { + anyhow::bail!(format!( + "Toolchain `{0}` was not found!\n\ + This specific version is required to provide additional source code analysis.\n\n + Install `rustup` according to https://rustup.rs/ and then run: `rustup install {0}`.", + linting::TOOLCHAIN_VERSION, + ) + .to_string() + .bright_yellow()); + } + // when testing this function we should never fall back to a `cargo` specified // in the env variable, as this would mess with the mocked binaries. #[cfg(not(test))] @@ -793,7 +817,7 @@ pub fn execute(args: ExecuteArgs) -> Result { build_artifact, unstable_flags, optimization_passes, - dylint, + extra_lints, output_type, target, .. @@ -838,7 +862,7 @@ pub fn execute(args: ExecuteArgs) -> Result { let (opt_result, metadata_result, dest_wasm) = match build_artifact { BuildArtifacts::CheckOnly => { // Check basically means only running our linter without building. - lint(*dylint, &crate_metadata, verbosity)?; + lint(*extra_lints, &crate_metadata, target, verbosity)?; (None, None, None) } BuildArtifacts::CodeOnly => { @@ -912,7 +936,7 @@ fn local_build( network, unstable_flags, keep_debug_symbols, - dylint, + extra_lints, skip_wasm_validation, target, max_memory_pages, @@ -921,7 +945,7 @@ fn local_build( // We always want to lint first so we don't suppress any warnings when a build is // skipped because of a matching fingerprint. - lint(*dylint, crate_metadata, verbosity)?; + lint(*extra_lints, crate_metadata, target, verbosity)?; let pre_fingerprint = Fingerprint::new(crate_metadata)?; diff --git a/crates/build/src/tests.rs b/crates/build/src/tests.rs index 40a16e3c8..119ea6e5d 100644 --- a/crates/build/src/tests.rs +++ b/crates/build/src/tests.rs @@ -76,7 +76,7 @@ build_tests!( build_with_json_output_works, building_contract_with_source_file_in_subfolder_must_work, building_contract_with_build_rs_must_work, - missing_cargo_dylint_installation_must_be_detected, + missing_linting_toolchain_installation_must_be_detected, generates_metadata, unchanged_contract_skips_optimization_and_metadata_steps, unchanged_contract_no_metadata_artifacts_generates_metadata @@ -87,7 +87,7 @@ fn build_code_only(manifest_path: &ManifestPath) -> Result<()> { manifest_path: manifest_path.clone(), build_mode: BuildMode::Release, build_artifact: BuildArtifacts::CodeOnly, - dylint: false, + extra_lints: false, ..Default::default() }; @@ -127,7 +127,7 @@ fn check_must_not_output_contract_artifacts_in_project_dir( let args = ExecuteArgs { manifest_path: manifest_path.clone(), build_artifact: BuildArtifacts::CheckOnly, - dylint: false, + extra_lints: false, ..Default::default() }; @@ -164,7 +164,7 @@ fn optimization_passes_from_cli_must_take_precedence_over_profile( unstable_flags: Default::default(), optimization_passes: Some(OptimizationPasses::Zero), keep_debug_symbols: false, - dylint: false, + extra_lints: false, output_type: OutputType::Json, skip_wasm_validation: false, target: Default::default(), @@ -207,7 +207,7 @@ fn optimization_passes_from_profile_must_be_used( // no optimization passes specified. optimization_passes: None, keep_debug_symbols: false, - dylint: false, + extra_lints: false, output_type: OutputType::Json, skip_wasm_validation: false, target: Default::default(), @@ -237,7 +237,7 @@ fn building_template_in_debug_mode_must_work(manifest_path: &ManifestPath) -> Re let args = ExecuteArgs { manifest_path: manifest_path.clone(), build_mode: BuildMode::Debug, - dylint: false, + extra_lints: false, ..Default::default() }; @@ -256,7 +256,7 @@ fn building_template_in_release_mode_must_work( let args = ExecuteArgs { manifest_path: manifest_path.clone(), build_mode: BuildMode::Release, - dylint: false, + extra_lints: false, ..Default::default() }; @@ -286,7 +286,7 @@ fn building_contract_with_source_file_in_subfolder_must_work( let args = ExecuteArgs { manifest_path: manifest_path.clone(), build_artifact: BuildArtifacts::CheckOnly, - dylint: false, + extra_lints: false, ..Default::default() }; @@ -307,12 +307,12 @@ fn building_contract_with_build_rs_must_work(manifest_path: &ManifestPath) -> Re let path = manifest_path.directory().expect("dir must exist"); let build_rs_path = path.join(Path::new("build.rs")); - fs::write(build_rs_path, "fn main() {}")?; + fs::write(build_rs_path, "#![cfg_attr(dylint_lib = \"ink_linting_mandatory\", allow(no_main))]\n\nfn main() {}")?; let args = ExecuteArgs { manifest_path: manifest_path.clone(), build_artifact: BuildArtifacts::CheckOnly, - dylint: false, + extra_lints: false, ..Default::default() }; @@ -330,7 +330,7 @@ fn keep_debug_symbols_in_debug_mode(manifest_path: &ManifestPath) -> Result<()> build_mode: BuildMode::Debug, build_artifact: BuildArtifacts::CodeOnly, keep_debug_symbols: true, - dylint: false, + extra_lints: false, ..Default::default() }; @@ -348,7 +348,7 @@ fn keep_debug_symbols_in_release_mode(manifest_path: &ManifestPath) -> Result<() build_mode: BuildMode::Release, build_artifact: BuildArtifacts::CodeOnly, keep_debug_symbols: true, - dylint: false, + extra_lints: false, ..Default::default() }; @@ -365,7 +365,7 @@ fn build_with_json_output_works(manifest_path: &ManifestPath) -> Result<()> { let args = ExecuteArgs { manifest_path: manifest_path.clone(), output_type: OutputType::Json, - dylint: false, + extra_lints: false, ..Default::default() }; @@ -378,7 +378,7 @@ fn build_with_json_output_works(manifest_path: &ManifestPath) -> Result<()> { } #[cfg(unix)] -fn missing_cargo_dylint_installation_must_be_detected( +fn missing_linting_toolchain_installation_must_be_detected( manifest_path: &ManifestPath, ) -> Result<()> { use super::util::tests::create_executable; @@ -386,22 +386,19 @@ fn missing_cargo_dylint_installation_must_be_detected( // given let manifest_dir = manifest_path.directory().unwrap(); - // mock existing `dylint-link` binary - let _tmp0 = create_executable(&manifest_dir.join("dylint-link"), "#!/bin/sh\nexit 0"); - - // mock a non-existing `cargo dylint` installation. - let _tmp1 = create_executable(&manifest_dir.join("cargo"), "#!/bin/sh\nexit 1"); + // mock non-existing `rustup` binary + let _tmp0 = create_executable(&manifest_dir.join("rustup"), "#!/bin/sh\nexit 1"); // when let args = ExecuteArgs { manifest_path: manifest_path.clone(), - dylint: true, + extra_lints: true, ..Default::default() }; let res = super::execute(args).map(|_| ()).unwrap_err(); // then - assert!(format!("{res:?}").contains("cargo-dylint was not found!")); + assert!(format!("{res:?}").contains("` was not found!")); Ok(()) } @@ -438,7 +435,7 @@ fn generates_metadata(manifest_path: &ManifestPath) -> Result<()> { fs::write(final_contract_wasm_path, "TEST FINAL WASM BLOB").unwrap(); let mut args = ExecuteArgs { - dylint: false, + extra_lints: false, ..Default::default() }; args.manifest_path = manifest_path.clone(); diff --git a/crates/build/src/workspace/manifest.rs b/crates/build/src/workspace/manifest.rs index c9ae6195e..0601f0c8f 100644 --- a/crates/build/src/workspace/manifest.rs +++ b/crates/build/src/workspace/manifest.rs @@ -314,11 +314,14 @@ impl Manifest { } pub fn with_dylint(&mut self) -> Result<&mut Self> { - let ink_dylint = { + let ink_dylint = |lib_name: &str| { let mut map = value::Table::new(); - map.insert("git".into(), "https://github.com/paritytech/ink/".into()); - map.insert("tag".into(), "v4.0.0-alpha.3".into()); - map.insert("pattern".into(), "linting/".into()); + map.insert("git".into(), crate::linting::GIT_URL.into()); + map.insert("rev".into(), crate::linting::GIT_REV.into()); + map.insert( + "pattern".into(), + value::Value::String(format!("linting/{}", lib_name)), + ); value::Value::Table(map) }; @@ -339,7 +342,7 @@ impl Manifest { .or_insert(value::Value::Array(Default::default())) .as_array_mut() .context("workspace.metadata.dylint.libraries section should be an array")? - .push(ink_dylint); + .extend(vec![ink_dylint("mandatory"), ink_dylint("extra")]); Ok(self) } diff --git a/crates/cargo-contract/src/cmd/build.rs b/crates/cargo-contract/src/cmd/build.rs index d0a86915d..f26cfb994 100644 --- a/crates/cargo-contract/src/cmd/build.rs +++ b/crates/cargo-contract/src/cmd/build.rs @@ -58,10 +58,10 @@ pub struct BuildCommand { /// Build offline #[clap(long = "offline")] build_offline: bool, - /// Performs ink! linting checks during the build process. + /// Performs extra ink! linting checks during the build process. /// - /// This only applies to ink! custom lints of which are there none at the moment. - /// Basic clippy lints which we are deem important are run anyways. + /// This only adds extra ink! linting checks. Basic clippy and ink! lints which we + /// are deem important are run anyways. #[clap(long)] lint: bool, /// Which build artifacts to generate. @@ -179,7 +179,7 @@ impl BuildCommand { unstable_flags, optimization_passes: self.optimization_passes, keep_debug_symbols: self.keep_debug_symbols, - dylint: self.lint, + extra_lints: self.lint, output_type, skip_wasm_validation: self.skip_wasm_validation, target: self.target, @@ -215,7 +215,7 @@ impl CheckCommand { unstable_flags: Default::default(), optimization_passes: Some(OptimizationPasses::Zero), keep_debug_symbols: false, - dylint: false, + extra_lints: false, output_type: OutputType::default(), skip_wasm_validation: false, target: Default::default(), diff --git a/crates/cargo-contract/src/cmd/verify.rs b/crates/cargo-contract/src/cmd/verify.rs index e9b9517e4..f60bbdd8d 100644 --- a/crates/cargo-contract/src/cmd/verify.rs +++ b/crates/cargo-contract/src/cmd/verify.rs @@ -144,7 +144,7 @@ impl VerifyCommand { optimization_passes: Some(build_info.wasm_opt_settings.optimization_passes), keep_debug_symbols: build_info.wasm_opt_settings.keep_debug_symbols, image: ImageVariant::from(metadata.image.clone()), - dylint: false, + extra_lints: false, ..Default::default() };