From 29e54353e1d3a67c3965ec271ae1c978a790988f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Mon, 14 Aug 2023 16:38:19 +0200 Subject: [PATCH] Disallow unchecked arithmetic (#1190) * Move away from using `overflow-checks` * Remove unchecked math from test * Simplify logic * Install clippy on the CI * Fix CI * fmt * update stale docs * Don't run clippy while building * Run clippy as a mandatory extra pass * Fix riscv build * Remove no longer needed arguments from check * Fix cargo output parsing * Fix doctest * Fix clippy * Remove build steps * Move linting before building * Disable warnings for metadata generation * Add comment about warning supression. * fmt --- .github/workflows/ci.yml | 4 +- crates/build/README.md | 2 +- crates/build/src/args.rs | 49 +------- crates/build/src/docker.rs | 46 ++----- crates/build/src/lib.rs | 166 +++++++++++++------------ crates/build/src/metadata.rs | 25 ++-- crates/build/src/tests.rs | 26 ++-- crates/build/src/workspace/manifest.rs | 14 ++- crates/build/src/workspace/profile.rs | 10 +- crates/cargo-contract/src/cmd/build.rs | 23 ++-- crates/cargo-contract/src/main.rs | 3 - crates/cargo-contract/tests/encode.rs | 2 +- 12 files changed, 146 insertions(+), 224 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 38d8090ef..0e70ffd92 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -139,7 +139,7 @@ jobs: toolchain: stable default: true target: wasm32-unknown-unknown - components: rust-src + components: rust-src, clippy - name: Cache uses: Swatinem/rust-cache@v2 @@ -183,7 +183,7 @@ jobs: toolchain: stable default: true target: wasm32-unknown-unknown - components: rust-src + components: rust-src, clippy - name: Cache uses: Swatinem/rust-cache@v2 diff --git a/crates/build/README.md b/crates/build/README.md index b8feec1ad..d789dfb0b 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, - lint: false, + dylint: false, output_type: OutputType::Json, skip_wasm_validation: false, target: Target::Wasm, diff --git a/crates/build/src/args.rs b/crates/build/src/args.rs index 869476af5..d41641bc5 100644 --- a/crates/build/src/args.rs +++ b/crates/build/src/args.rs @@ -120,52 +120,13 @@ impl BuildArtifacts { /// Used as output on the cli. pub fn steps(&self) -> usize { match self { - BuildArtifacts::All => 4, - BuildArtifacts::CodeOnly => 3, + BuildArtifacts::All => 5, + BuildArtifacts::CodeOnly => 4, BuildArtifacts::CheckOnly => 1, } } } -/// Track and display the current and total number of steps. -#[derive(Debug, Clone, Copy)] -pub struct BuildSteps { - pub current_step: usize, - pub total_steps: Option, -} - -impl BuildSteps { - pub fn new() -> Self { - Self { - current_step: 1, - total_steps: None, - } - } - - pub fn increment_current(&mut self) { - self.current_step += 1; - } - - pub fn set_total_steps(&mut self, steps: usize) { - self.total_steps = Some(steps) - } -} - -impl Default for BuildSteps { - fn default() -> Self { - Self::new() - } -} - -impl fmt::Display for BuildSteps { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let total_steps = self - .total_steps - .map_or("*".to_string(), |steps| steps.to_string()); - write!(f, "[{}/{}]", self.current_step, total_steps) - } -} - /// The list of targets that ink! supports. #[derive( Eq, @@ -199,10 +160,10 @@ impl Target { } /// Target specific flags to be set to `CARGO_ENCODED_RUSTFLAGS` while building. - pub fn rustflags(&self) -> &'static str { + pub fn rustflags(&self) -> Option<&'static str> { match self { - Self::Wasm => "-C\x1flink-arg=-zstack-size=65536\x1f-C\x1flink-arg=--import-memory\x1f-Clinker-plugin-lto\x1f-C\x1ftarget-cpu=mvp", - Self::RiscV => "-Clinker-plugin-lto", + Self::Wasm => Some("-Clink-arg=-zstack-size=65536\x1f-Clink-arg=--import-memory\x1f-Ctarget-cpu=mvp"), + Self::RiscV => None, } } diff --git a/crates/build/src/docker.rs b/crates/build/src/docker.rs index 34dd9b246..ad72b6de7 100644 --- a/crates/build/src/docker.rs +++ b/crates/build/src/docker.rs @@ -86,9 +86,7 @@ use tokio_stream::{ use crate::{ verbose_eprintln, - BuildArtifacts, BuildResult, - BuildSteps, CrateMetadata, ExecuteArgs, Verbosity, @@ -119,7 +117,6 @@ pub fn docker_build(args: ExecuteArgs) -> Result { verbosity, output_type, target, - build_artifact, image, .. } = args; @@ -127,13 +124,6 @@ pub fn docker_build(args: ExecuteArgs) -> Result { .enable_all() .build()? .block_on(async { - let mut build_steps = BuildSteps::new(); - - build_steps.set_total_steps(3); - if build_artifact == BuildArtifacts::CodeOnly { - build_steps.set_total_steps(2); - } - let crate_metadata = CrateMetadata::collect(&manifest_path, target)?; let host_folder = std::env::current_dir()?; let args = compose_build_args()?; @@ -159,23 +149,19 @@ pub fn docker_build(args: ExecuteArgs) -> Result { &crate_metadata.contract_artifact_name, &host_folder, &verbosity, - &mut build_steps, ) .await?; - let mut build_result = - run_build(&client, &container, &verbosity, &mut build_steps).await?; + let mut build_result = run_build(&client, &container, &verbosity).await?; update_build_result(&host_folder, &mut build_result)?; - update_metadata(&build_result, &verbosity, &mut build_steps, &image, &client) - .await?; + update_metadata(&build_result, &verbosity, &image, &client).await?; - build_steps.increment_current(); verbose_eprintln!( verbosity, " {} {}", - format!("{build_steps}").bold(), + "[==]".bold(), "Displaying results".bright_cyan().bold(), ); @@ -228,7 +214,6 @@ fn update_build_result(host_folder: &Path, build_result: &mut BuildResult) -> Re async fn update_metadata( build_result: &BuildResult, verbosity: &Verbosity, - build_steps: &mut BuildSteps, build_image: &str, client: &Docker, ) -> Result<()> { @@ -250,13 +235,7 @@ async fn update_metadata( metadata.image = Some(image_tag); - crate::metadata::write_metadata( - metadata_artifacts, - metadata, - build_steps, - verbosity, - true, - )?; + crate::metadata::write_metadata(metadata_artifacts, metadata, verbosity, true)?; } Ok(()) } @@ -287,7 +266,6 @@ async fn create_container( contract_name: &str, host_folder: &Path, verbosity: &Verbosity, - build_steps: &mut BuildSteps, ) -> Result { let entrypoint = vec!["cargo".to_string(), "contract".to_string()]; @@ -378,8 +356,7 @@ async fn create_container( } ) { // no such image locally, so pull and try again - pull_image(client, build_image.to_string(), verbosity, build_steps) - .await?; + pull_image(client, build_image.to_string(), verbosity).await?; client .create_container(options, config) .await @@ -397,7 +374,6 @@ async fn run_build( client: &Docker, container_name: &str, verbosity: &Verbosity, - build_steps: &mut BuildSteps, ) -> Result { client .start_container::(container_name, None) @@ -418,7 +394,7 @@ async fn run_build( verbose_eprintln!( verbosity, " {} {}", - format!("{build_steps}").bold(), + "[==]".bold(), format!("Started the build inside the container: {}", container_name) .bright_cyan() .bold(), @@ -487,12 +463,7 @@ fn compose_build_args() -> Result> { } /// Pulls the docker image from the registry. -async fn pull_image( - client: &Docker, - image: String, - verbosity: &Verbosity, - build_steps: &mut BuildSteps, -) -> Result<()> { +async fn pull_image(client: &Docker, image: String, verbosity: &Verbosity) -> Result<()> { let mut pull_image_stream = client.create_image( Some(CreateImageOptions { from_image: image, @@ -505,12 +476,11 @@ async fn pull_image( verbose_eprintln!( verbosity, " {} {}", - format!("{build_steps}").bold(), + "[==]".bold(), "Image does not exist. Pulling one from the registry" .bright_cyan() .bold() ); - build_steps.increment_current(); if verbosity.is_verbose() { show_pull_progress(pull_image_stream).await? diff --git a/crates/build/src/lib.rs b/crates/build/src/lib.rs index 9999ded5e..9999ace10 100644 --- a/crates/build/src/lib.rs +++ b/crates/build/src/lib.rs @@ -38,7 +38,6 @@ pub use self::{ args::{ BuildArtifacts, BuildMode, - BuildSteps, Features, Network, OutputType, @@ -122,7 +121,7 @@ pub struct ExecuteArgs { pub unstable_flags: UnstableFlags, pub optimization_passes: Option, pub keep_debug_symbols: bool, - pub lint: bool, + pub dylint: bool, pub output_type: OutputType, pub skip_wasm_validation: bool, pub target: Target, @@ -142,7 +141,7 @@ impl Default for ExecuteArgs { unstable_flags: Default::default(), optimization_passes: Default::default(), keep_debug_symbols: Default::default(), - lint: Default::default(), + dylint: Default::default(), output_type: Default::default(), skip_wasm_validation: Default::default(), target: Default::default(), @@ -319,8 +318,23 @@ fn exec_cargo_for_onchain_target( ); } + // merge target specific flags with the common flags (defined here) + // We want to disable warnings here as they will be duplicates of the clippy pass. + // However, if we want to do so with either `--cap-lints allow` or `-A + // warnings` the build will fail. It seems that the cross compilation + // depends on some warning to be enabled. Until we figure that out we need + // to live with duplicated warnings. For the metadata build we can disable + // warnings. + let rustflags = { + let common_flags = "-Clinker-plugin-lto"; + if let Some(target_flags) = target.rustflags() { + format!("{}\x1f{}", common_flags, target_flags) + } else { + common_flags.to_string() + } + }; + // the linker needs our linker script as file - let rustflags = target.rustflags(); if matches!(target, Target::RiscV) { fs::create_dir_all(&crate_metadata.target_directory)?; let path = crate_metadata @@ -333,7 +347,7 @@ fn exec_cargo_for_onchain_target( Some(format!("{rustflags}\x1f-Clink-arg=-T{path}",)), )); } else { - env.push(("CARGO_ENCODED_RUSTFLAGS", Some(rustflags.to_string()))); + env.push(("CARGO_ENCODED_RUSTFLAGS", Some(rustflags))); }; let cargo = @@ -376,7 +390,8 @@ fn invoke_cargo_and_scan_for_error(cargo: duct::Expression) -> Result<()> { }}; } - let cargo = util::cargo_tty_output(cargo); + // 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()); @@ -413,8 +428,55 @@ fn invoke_cargo_and_scan_for_error(cargo: duct::Expression) -> Result<()> { Ok(()) } -/// Executes `cargo dylint` with the ink! linting driver that is built during -/// the `build.rs`. +/// Run linting steps which include `clippy` (mandatory) + `dylint` (optional). +fn lint( + dylint: bool, + crate_metadata: &CrateMetadata, + verbosity: &Verbosity, +) -> Result<()> { + // mandatory: Always run clippy. + verbose_eprintln!( + verbosity, + " {} {}", + "[==]".bold(), + "Checking clippy linting rules".bright_green().bold() + ); + exec_cargo_clippy(crate_metadata, *verbosity)?; + + // optional: Dylint only on demand (for now). + if dylint { + verbose_eprintln!( + verbosity, + " {} {}", + "[==]".bold(), + "Checking ink! linting rules".bright_green().bold() + ); + exec_cargo_dylint(crate_metadata, *verbosity)?; + } + Ok(()) +} + +/// Run cargo clippy on the unmodified manifest. +fn exec_cargo_clippy(crate_metadata: &CrateMetadata, verbosity: Verbosity) -> Result<()> { + let args = [ + "--all-features", + // customize clippy lints after the "--" + "--", + // this is a hard error because we want to guarantee that implicit overflows + // never happen + "-Dclippy::arithmetic_side_effects", + ]; + // we execute clippy with the plain manifest no temp dir required + invoke_cargo_and_scan_for_error(util::cargo_cmd( + "clippy", + args, + crate_metadata.manifest_path.directory(), + verbosity, + vec![], + )) +} + +/// 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. @@ -668,7 +730,7 @@ fn assert_compatible_ink_dependencies( /// Checks whether the supplied `ink_version` already contains the debug feature. /// /// This feature was introduced in `3.0.0-rc4` with `ink_env/ink-debug`. -pub fn assert_debug_mode_supported(ink_version: &Version) -> anyhow::Result<()> { +pub fn assert_debug_mode_supported(ink_version: &Version) -> Result<()> { tracing::debug!("Contract version: {:?}", ink_version); let minimum_version = Version::parse("3.0.0-rc4").expect("parsing version failed"); if ink_version < &minimum_version { @@ -693,7 +755,7 @@ pub fn execute(args: ExecuteArgs) -> Result { build_artifact, unstable_flags, optimization_passes, - lint, + dylint, output_type, target, .. @@ -734,42 +796,19 @@ pub fn execute(args: ExecuteArgs) -> Result { let (opt_result, metadata_result, dest_wasm) = match build_artifact { BuildArtifacts::CheckOnly => { - let mut build_steps = BuildSteps::new(); - maybe_lint( - &mut build_steps, - *build_artifact, - *lint, - &crate_metadata, - verbosity, - )?; - - verbose_eprintln!( - verbosity, - " {} {}", - format!("{build_steps}").bold(), - "Executing `cargo check`".bright_green().bold() - ); - exec_cargo_for_onchain_target( - &crate_metadata, - "check", - features, - &BuildMode::Release, - network, - verbosity, - unstable_flags, - target, - )?; + // Check basically means only running our linter without building. + lint(*dylint, &crate_metadata, verbosity)?; (None, None, None) } BuildArtifacts::CodeOnly => { // when building only the code metadata will become stale clean_metadata(); - let (opt_result, _, dest_wasm, _) = + let (opt_result, _, dest_wasm) = local_build(&crate_metadata, &optimization_passes, &args)?; (opt_result, None, Some(dest_wasm)) } BuildArtifacts::All => { - let (opt_result, build_info, dest_wasm, build_steps) = + let (opt_result, build_info, dest_wasm) = local_build(&crate_metadata, &optimization_passes, &args).map_err( |e| { // build error -> bundle is stale @@ -798,7 +837,6 @@ pub fn execute(args: ExecuteArgs) -> Result { features, *network, *verbosity, - build_steps, unstable_flags, build_info, )?; @@ -825,32 +863,33 @@ fn local_build( crate_metadata: &CrateMetadata, optimization_passes: &OptimizationPasses, args: &ExecuteArgs, -) -> Result<(Option, BuildInfo, PathBuf, BuildSteps)> { +) -> Result<(Option, BuildInfo, PathBuf)> { let ExecuteArgs { verbosity, features, build_mode, network, - build_artifact, unstable_flags, keep_debug_symbols, - lint, + dylint, skip_wasm_validation, target, max_memory_pages, .. } = args; - let mut build_steps = BuildSteps::new(); + // 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)?; + let pre_fingerprint = Fingerprint::new(crate_metadata)?; verbose_eprintln!( verbosity, " {} {}", - format!("{build_steps}").bold(), + "[==]".bold(), "Building cargo project".bright_green().bold() ); - build_steps.increment_current(); exec_cargo_for_onchain_target( crate_metadata, "build", @@ -906,24 +945,15 @@ fn local_build( crate_metadata.original_code.display(), pre_fingerprint ); - return Ok((None, build_info, dest_code_path, build_steps)) + return Ok((None, build_info, dest_code_path)) } - maybe_lint( - &mut build_steps, - *build_artifact, - *lint, - crate_metadata, - verbosity, - )?; - verbose_eprintln!( verbosity, " {} {}", - format!("{build_steps}").bold(), + "[==]".bold(), "Post processing code".bright_green().bold() ); - build_steps.increment_current(); // remove build artifacts so we don't have anything stale lingering around for t in Target::iter() { @@ -963,35 +993,9 @@ fn local_build( Some(optimization_result), build_info, crate_metadata.dest_code.clone(), - build_steps, )) } -pub fn maybe_lint( - steps: &mut BuildSteps, - build_artifact: BuildArtifacts, - lint: bool, - crate_metadata: &CrateMetadata, - verbosity: &Verbosity, -) -> Result<()> { - let total_steps = build_artifact.steps(); - if lint { - steps.set_total_steps(total_steps + 1); - verbose_eprintln!( - verbosity, - " {} {}", - format!("{steps}").bold(), - "Checking ink! linting rules".bright_green().bold() - ); - steps.increment_current(); - exec_cargo_dylint(crate_metadata, *verbosity)?; - Ok(()) - } else { - steps.set_total_steps(total_steps); - Ok(()) - } -} - /// Unique fingerprint for a file to detect whether it has changed. #[derive(Debug, Eq, PartialEq)] struct Fingerprint { diff --git a/crates/build/src/metadata.rs b/crates/build/src/metadata.rs index 5266ccf96..e686221f7 100644 --- a/crates/build/src/metadata.rs +++ b/crates/build/src/metadata.rs @@ -24,7 +24,6 @@ use crate::{ Workspace, }, BuildMode, - BuildSteps, Features, Lto, Network, @@ -118,7 +117,6 @@ pub fn execute( features: &Features, network: Network, verbosity: Verbosity, - mut build_steps: BuildSteps, unstable_options: &UnstableFlags, build_info: BuildInfo, ) -> Result<()> { @@ -133,8 +131,8 @@ pub fn execute( verbose_eprintln!( verbosity, " {} {}", - format!("{build_steps}").bold(), - "Generating metadata".bright_green().bold() + "[==]".bold(), + "Generating metadata".bright_green().bold(), ); let target_dir = crate_metadata .target_directory @@ -156,7 +154,10 @@ pub fn execute( args, crate_metadata.manifest_path.directory(), verbosity, - vec![], + vec![( + "CARGO_ENCODED_RUSTFLAGS", + Some("--cap-lints=allow".to_string()), + )], ); let output = cmd.stdout_capture().run()?; @@ -164,13 +165,7 @@ pub fn execute( serde_json::from_slice(&output.stdout)?; let metadata = ContractMetadata::new(source, contract, None, user, ink_meta); - write_metadata( - metadata_artifacts, - metadata, - &mut build_steps, - &verbosity, - false, - )?; + write_metadata(metadata_artifacts, metadata, &verbosity, false)?; Ok(()) }; @@ -199,7 +194,6 @@ pub fn execute( pub fn write_metadata( metadata_artifacts: &MetadataArtifacts, metadata: ContractMetadata, - build_steps: &mut BuildSteps, verbosity: &Verbosity, overwrite: bool, ) -> Result<()> { @@ -208,21 +202,20 @@ pub fn write_metadata( metadata.remove_source_wasm_attribute(); let contents = serde_json::to_string_pretty(&metadata)?; fs::write(&metadata_artifacts.dest_metadata, contents)?; - build_steps.increment_current(); } if overwrite { verbose_eprintln!( verbosity, " {} {}", - format!("{build_steps}").bold(), + "[==]".bold(), "Updating paths".bright_cyan().bold() ); } else { verbose_eprintln!( verbosity, " {} {}", - format!("{build_steps}").bold(), + "[==]".bold(), "Generating bundle".bright_green().bold() ); } diff --git a/crates/build/src/tests.rs b/crates/build/src/tests.rs index 26a64de4e..40a16e3c8 100644 --- a/crates/build/src/tests.rs +++ b/crates/build/src/tests.rs @@ -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, - lint: false, + dylint: 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, - lint: false, + dylint: 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, - lint: false, + dylint: 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, - lint: false, + dylint: 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, - lint: false, + dylint: 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, - lint: false, + dylint: 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, - lint: false, + dylint: false, ..Default::default() }; @@ -312,7 +312,7 @@ fn building_contract_with_build_rs_must_work(manifest_path: &ManifestPath) -> Re let args = ExecuteArgs { manifest_path: manifest_path.clone(), build_artifact: BuildArtifacts::CheckOnly, - lint: false, + dylint: 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, - lint: false, + dylint: 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, - lint: false, + dylint: 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, - lint: false, + dylint: false, ..Default::default() }; @@ -395,7 +395,7 @@ fn missing_cargo_dylint_installation_must_be_detected( // when let args = ExecuteArgs { manifest_path: manifest_path.clone(), - lint: true, + dylint: true, ..Default::default() }; let res = super::execute(args).map(|_| ()).unwrap_err(); @@ -438,7 +438,7 @@ fn generates_metadata(manifest_path: &ManifestPath) -> Result<()> { fs::write(final_contract_wasm_path, "TEST FINAL WASM BLOB").unwrap(); let mut args = ExecuteArgs { - lint: false, + dylint: 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 83dbbc39f..7c23701f3 100644 --- a/crates/build/src/workspace/manifest.rs +++ b/crates/build/src/workspace/manifest.rs @@ -134,12 +134,20 @@ impl Manifest { pub fn new(manifest_path: ManifestPath) -> Result { let toml = fs::read_to_string(&manifest_path).context("Loading Cargo.toml")?; let toml: value::Table = toml::from_str(&toml)?; - - Ok(Manifest { + let mut manifest = Manifest { path: manifest_path, toml, metadata_package: false, - }) + }; + let profile = manifest.profile_release_table_mut()?; + if profile + .get("overflow-checks") + .and_then(|val| val.as_bool()) + .unwrap_or(false) + { + anyhow::bail!("Overflow checks must be disabled. Cargo contract makes sure that no unchecked arithmetic is used.") + } + Ok(manifest) } /// Get the name of the package. diff --git a/crates/build/src/workspace/profile.rs b/crates/build/src/workspace/profile.rs index e9e22f324..ccd2f6a80 100644 --- a/crates/build/src/workspace/profile.rs +++ b/crates/build/src/workspace/profile.rs @@ -26,7 +26,6 @@ pub struct Profile { pub lto: Option, // `None` means use rustc default. pub codegen_units: Option, - pub overflow_checks: Option, pub panic: Option, } @@ -37,7 +36,6 @@ impl Profile { opt_level: Some(OptLevel::Z), lto: Some(Lto::Fat), codegen_units: Some(1), - overflow_checks: Some(true), panic: Some(PanicStrategy::Abort), } } @@ -70,7 +68,6 @@ impl Profile { ); set_value_if_vacant("lto", self.lto.map(Lto::to_toml_value), profile); set_value_if_vacant("codegen-units", self.codegen_units, profile); - set_value_if_vacant("overflow-checks", self.overflow_checks, profile); set_value_if_vacant( "panic", self.panic.map(PanicStrategy::to_toml_value), @@ -157,11 +154,10 @@ mod tests { // no `[profile.release]` section specified let manifest_toml = ""; - let mut expected = toml::value::Table::new(); + let mut expected = value::Table::new(); expected.insert("opt-level".into(), value::Value::String("z".into())); expected.insert("lto".into(), value::Value::String("fat".into())); expected.insert("codegen-units".into(), value::Value::Integer(1)); - expected.insert("overflow-checks".into(), value::Value::Boolean(true)); expected.insert("panic".into(), value::Value::String("abort".into())); let mut manifest_profile = toml::from_str(manifest_toml).unwrap(); @@ -179,14 +175,12 @@ mod tests { panic = "unwind" lto = false opt-level = 3 - overflow-checks = false codegen-units = 256 "#; - let mut expected = toml::value::Table::new(); + let mut expected = value::Table::new(); expected.insert("opt-level".into(), value::Value::Integer(3)); expected.insert("lto".into(), value::Value::Boolean(false)); expected.insert("codegen-units".into(), value::Value::Integer(256)); - expected.insert("overflow-checks".into(), value::Value::Boolean(false)); expected.insert("panic".into(), value::Value::String("unwind".into())); let mut manifest_profile = toml::from_str(manifest_toml).unwrap(); diff --git a/crates/cargo-contract/src/cmd/build.rs b/crates/cargo-contract/src/cmd/build.rs index 75192d197..d0a86915d 100644 --- a/crates/cargo-contract/src/cmd/build.rs +++ b/crates/cargo-contract/src/cmd/build.rs @@ -58,7 +58,10 @@ pub struct BuildCommand { /// Build offline #[clap(long = "offline")] build_offline: bool, - /// Performs linting checks during the build process + /// Performs 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. #[clap(long)] lint: bool, /// Which build artifacts to generate. @@ -176,7 +179,7 @@ impl BuildCommand { unstable_flags, optimization_passes: self.optimization_passes, keep_debug_symbols: self.keep_debug_symbols, - lint: self.lint, + dylint: self.lint, output_type, skip_wasm_validation: self.skip_wasm_validation, target: self.target, @@ -195,35 +198,27 @@ pub struct CheckCommand { manifest_path: Option, #[clap(flatten)] verbosity: VerbosityFlags, - #[clap(flatten)] - features: Features, - #[clap(flatten)] - unstable_options: UnstableOptions, - #[clap(long, default_value = "wasm")] - target: Target, } impl CheckCommand { pub fn exec(&self) -> Result { let manifest_path = ManifestPath::try_from(self.manifest_path.as_ref())?; - let unstable_flags: UnstableFlags = - TryFrom::<&UnstableOptions>::try_from(&self.unstable_options)?; let verbosity: Verbosity = TryFrom::<&VerbosityFlags>::try_from(&self.verbosity)?; let args = ExecuteArgs { manifest_path, verbosity, build_mode: BuildMode::Debug, - features: self.features.clone(), + features: Default::default(), network: Network::default(), build_artifact: BuildArtifacts::CheckOnly, - unstable_flags, + unstable_flags: Default::default(), optimization_passes: Some(OptimizationPasses::Zero), keep_debug_symbols: false, - lint: false, + dylint: false, output_type: OutputType::default(), skip_wasm_validation: false, - target: self.target, + target: Default::default(), max_memory_pages: 0, image: ImageVariant::Default, }; diff --git a/crates/cargo-contract/src/main.rs b/crates/cargo-contract/src/main.rs index 42ff3c498..f7ffb2c7a 100644 --- a/crates/cargo-contract/src/main.rs +++ b/crates/cargo-contract/src/main.rs @@ -175,9 +175,6 @@ fn exec(cmd: Command) -> Result<()> { res.dest_wasm.is_none(), "no dest_wasm must be on the generation result" ); - if res.verbosity.is_verbose() { - println!("\nYour contract's code was built successfully.") - } Ok(()) } Command::Upload(upload) => { diff --git a/crates/cargo-contract/tests/encode.rs b/crates/cargo-contract/tests/encode.rs index 362cfc4a6..252be51ea 100644 --- a/crates/cargo-contract/tests/encode.rs +++ b/crates/cargo-contract/tests/encode.rs @@ -49,7 +49,7 @@ fn encode_works() { #[ink(message, selector = 0xBABABABA)] pub fn inc(&mut self, by: i32) { - self.value += by; + self.value.saturating_add(by); } #[ink(message, selector = 0xCACACACA)]