review update

scarb/src/compiler/repository.rs

@@ -8,7 +8,6 @@ use itertools::Itertools;
 use smol_str::SmolStr;
 
 use crate::compiler::compilers::{LibCompiler, StarknetContractCompiler, TestCompiler};
-use crate::compiler::helpers::build_compiler_config;
 use crate::compiler::{CompilationUnit, Compiler};
 use crate::core::Workspace;
 
@@ -54,17 +53,6 @@ impl CompilerRepository {
         };
         compiler.compile(unit, db, ws)
     }
-
-    pub fn check(
-        &self,
-        unit: CompilationUnit,
-        db: &mut RootDatabase,
-        ws: &Workspace<'_>,
-    ) -> Result<()> {
-        let mut compiler_config = build_compiler_config(&unit, ws);
-        compiler_config.diagnostics_reporter.ensure(db)?;
-        Ok(())
-    }
 }
 
 impl fmt::Debug for CompilerRepository {

scarb/src/ops/compile.rs

@@ -1,13 +1,15 @@
 use anyhow::{anyhow, Result};
+use cairo_lang_compiler::db::RootDatabase;
 use cairo_lang_compiler::diagnostics::DiagnosticsError;
 use indoc::formatdoc;
 
 use scarb_ui::components::Status;
 use scarb_ui::HumanDuration;
 
 use crate::compiler::db::{build_scarb_root_database, has_starknet_plugin};
+use crate::compiler::helpers::build_compiler_config;
 use crate::compiler::CompilationUnit;
-use crate::core::{PackageId, TargetKind, Utf8PathWorkspaceExt, Workspace};
+use crate::core::{PackageId, PackageName, TargetKind, Utf8PathWorkspaceExt, Workspace};
 use crate::ops;
 
 #[derive(Debug)]
@@ -18,6 +20,25 @@ pub struct CompileOpts {
 
 #[tracing::instrument(skip_all, level = "debug")]
 pub fn compile(packages: Vec<PackageId>, opts: CompileOpts, ws: &Workspace<'_>) -> Result<()> {
+    process(packages, opts, ws, compile_unit, None)
+}
+
+#[tracing::instrument(skip_all, level = "debug")]
+pub fn check(packages: Vec<PackageId>, opts: CompileOpts, ws: &Workspace<'_>) -> Result<()> {
+    process(packages, opts, ws, check_unit, Some("checking"))
+}
+
+#[tracing::instrument(skip_all, level = "debug")]
+fn process<F>(
+    packages: Vec<PackageId>,
+    opts: CompileOpts,
+    ws: &Workspace<'_>,
+    mut operation: F,
+    operation_type: Option<&str>,
+) -> Result<()>
+where
+    F: FnMut(CompilationUnit, &Workspace<'_>) -> Result<()>,
+{
     let resolve = ops::resolve_workspace(ws)?;
 
     // Add test compilation units to build
@@ -46,14 +67,17 @@ pub fn compile(packages: Vec<PackageId>, opts: CompileOpts, ws: &Workspace<'_>)
         .collect::<Vec<_>>();
 
     for unit in compilation_units {
-        compile_unit(unit, ws)?;
+        operation(unit, ws)?;
     }
 
     let elapsed_time = HumanDuration(ws.config().elapsed_time());
-    ws.config().ui().print(Status::new(
-        "Finished",
-        &format!("release target(s) in {elapsed_time}"),
-    ));
+    let formatted_message = match operation_type {
+        Some(op) => format!("{op} release target(s) in {elapsed_time}"),
+        None => format!("release target(s) in {elapsed_time}"),
+    };
+    ws.config()
+        .ui()
+        .print(Status::new("Finished", &formatted_message));
 
     Ok(())
 }
@@ -67,6 +91,56 @@ fn compile_unit(unit: CompilationUnit, ws: &Workspace<'_>) -> Result<()> {
 
     let mut db = build_scarb_root_database(&unit, ws)?;
 
+    check_starknet_dependency(&unit, ws, &db, &package_name);
+
+    ws.config()
+        .compilers()
+        .compile(unit, &mut db, ws)
+        .map_err(|err| {
+            if !suppress_error(&err) {
+                ws.config().ui().anyhow(&err);
+            }
+
+            anyhow!("could not compile `{package_name}` due to previous error")
+        })?;
+
+    Ok(())
+}
+
+fn check_unit(unit: CompilationUnit, ws: &Workspace<'_>) -> Result<()> {
+    let package_name = unit.main_package_id.name.clone();
+
+    ws.config()
+        .ui()
+        .print(Status::new("Checking", &unit.name()));
+
+    let db = build_scarb_root_database(&unit, ws)?;
+
+    check_starknet_dependency(&unit, ws, &db, &package_name);
+
+    let mut compiler_config = build_compiler_config(&unit, ws);
+
+    compiler_config
+        .diagnostics_reporter
+        .ensure(&db)
+        .map_err(|err| {
+            let valid_error = err.into();
+            if !suppress_error(&valid_error) {
+                ws.config().ui().anyhow(&valid_error);
+            }
+
+            anyhow!("could not check `{package_name}` due to previous error")
+        })?;
+
+    Ok(())
+}
+
+fn check_starknet_dependency(
+    unit: &CompilationUnit,
+    ws: &Workspace<'_>,
+    db: &RootDatabase,
+    package_name: &PackageName,
+) {
     // NOTE: This is a special case that can be hit frequently by newcomers. Not specifying
     //   `starknet` dependency will error in 99% real-world Starknet contract projects.
     //   I think we can get away with emitting false positives for users who write raw contracts
@@ -85,21 +159,8 @@ fn compile_unit(unit: CompilationUnit, ws: &Workspace<'_>) -> Result<()> {
             cairo_version = crate::version::get().cairo.version,
         })
     }
-
-    ws.config()
-        .compilers()
-        .compile(unit, &mut db, ws)
-        .map_err(|err| {
-            if !suppress_error(&err) {
-                ws.config().ui().anyhow(&err);
-            }
-
-            anyhow!("could not compile `{package_name}` due to previous error")
-        })?;
-
-    Ok(())
 }
 
-pub fn suppress_error(err: &anyhow::Error) -> bool {
+fn suppress_error(err: &anyhow::Error) -> bool {
     matches!(err.downcast_ref(), Some(&DiagnosticsError))
 }

scarb/src/ops/mod.rs

@@ -3,7 +3,6 @@
 //! For datastructures describing the state, see [`crate::core`] module.
 
 pub use cache::*;
-pub use check::*;
 pub use clean::*;
 pub use compile::*;
 pub use fmt::*;
@@ -18,7 +17,6 @@ pub use subcommands::*;
 pub use workspace::*;
 
 mod cache;
-mod check;
 mod clean;
 mod compile;
 mod fmt;

scarb/tests/build_starknet_contract_allowed_libfuncs.rs

@@ -38,9 +38,9 @@ fn default_behaviour() {
         .lib_cairo(EXPERIMENTAL_LIBFUNC)
         .build(&t);
 
-    // TODO this warning can only be achieved by using `build`, not `check`
-    //  should we be worried about this?
     Scarb::quick_snapbox()
+        // NOTE: we cannot use `check` here, because without full compilation
+        // we cannot predict what libfuncs would be generated
         .arg("build")
         .current_dir(&t)
         .assert()
@@ -105,13 +105,16 @@ fn check_false() {
         .build(&t);
 
     Scarb::quick_snapbox()
-        .arg("check")
+        // NOTE: we cannot use `check` here, because without full compilation
+        // we cannot predict what libfuncs would be generated
+        // Also, `check` subcommand would not check if the libfuncs warning doesn't appear
+        .arg("build")
         .current_dir(&t)
         .assert()
         .success()
         .stdout_matches(indoc! {r#"
-        [..]  Checking hello v0.1.0 ([..])
-        [..]  Finished checking release target(s) in [..]
+        [..] Compiling hello v0.1.0 ([..])
+        [..]  Finished release target(s) in [..]
         "#});
 }
 
@@ -136,6 +139,8 @@ fn deny_true() {
         .build(&t);
 
     Scarb::quick_snapbox()
+        // NOTE: we cannot use `check` here, because without full compilation
+        // we cannot predict what libfuncs would be generated
         .arg("build")
         .current_dir(&t)
         .assert()
@@ -165,6 +170,8 @@ fn pass_named_list() {
         .build(&t);
 
     Scarb::quick_snapbox()
+        // NOTE: we cannot use `check` here, because without full compilation
+        // we cannot predict what libfuncs would be generated
         .arg("build")
         .current_dir(&t)
         .assert()
@@ -190,6 +197,8 @@ fn unknown_list_name() {
         .build(&t);
 
     Scarb::quick_snapbox()
+        // NOTE: we cannot use `check` here, because without full compilation
+        // we cannot predict what libfuncs would be generated
         .arg("build")
         .current_dir(&t)
         .assert()
@@ -224,6 +233,8 @@ fn list_path() {
         .build(&t);
 
     Scarb::quick_snapbox()
+        // NOTE: we cannot use `check` here, because without full compilation
+        // we cannot predict what libfuncs would be generated
         .arg("build")
         .current_dir(&t)
         .assert()
@@ -252,6 +263,8 @@ fn list_path_does_not_exist() {
         .build(&t);
 
     Scarb::quick_snapbox()
+        // NOTE: we cannot use `check` here, because without full compilation
+        // we cannot predict what libfuncs would be generated
         .arg("build")
         .current_dir(&t)
         .assert()