Merge pull request #3821 from roc-lang/fix-roc-run2

Don't monomorphize if roc foo.roc has errors

crates/cli/src/build.rs

@@ -5,7 +5,10 @@ use roc_build::{
 };
 use roc_builtins::bitcode;
 use roc_collections::VecMap;
-use roc_load::{EntryPoint, ExecutionMode, Expectations, LoadConfig, LoadingProblem, Threading};
+use roc_load::{
+    EntryPoint, ExecutionMode, Expectations, LoadConfig, LoadMonomorphizedError, LoadedModule,
+    LoadingProblem, Threading,
+};
 use roc_module::symbol::{Interns, ModuleId};
 use roc_mono::ir::OptLevel;
 use roc_reporting::report::RenderTarget;
@@ -35,6 +38,23 @@ pub struct BuiltFile {
     pub interns: Interns,
 }
 
+pub enum BuildOrdering {
+    /// Run up through typechecking first; continue building iff that is successful.
+    BuildIfChecks,
+    /// Always build the Roc binary, even if there are type errors.
+    AlwaysBuild,
+}
+
+#[derive(Debug)]
+#[allow(clippy::large_enum_variant)]
+pub enum BuildFileError<'a> {
+    LoadingProblem(LoadingProblem<'a>),
+    ErrorModule {
+        module: LoadedModule,
+        total_time: Duration,
+    },
+}
+
 #[allow(clippy::too_many_arguments)]
 pub fn build_file<'a>(
     arena: &'a Bump,
@@ -48,26 +68,44 @@ pub fn build_file<'a>(
     precompiled: bool,
     threading: Threading,
     wasm_dev_stack_bytes: Option<u32>,
-) -> Result<BuiltFile, LoadingProblem<'a>> {
+    order: BuildOrdering,
+) -> Result<BuiltFile, BuildFileError<'a>> {
     let compilation_start = Instant::now();
     let target_info = TargetInfo::from(target);
 
     // Step 1: compile the app and generate the .o file
     let subs_by_module = Default::default();
 
+    let exec_mode = match order {
+        BuildOrdering::BuildIfChecks => ExecutionMode::ExecutableIfCheck,
+        BuildOrdering::AlwaysBuild => ExecutionMode::Executable,
+    };
+
     let load_config = LoadConfig {
         target_info,
         // TODO: expose this from CLI?
         render: RenderTarget::ColorTerminal,
         threading,
-        exec_mode: ExecutionMode::Executable,
+        exec_mode,
     };
-    let loaded = roc_load::load_and_monomorphize(
+    let load_result = roc_load::load_and_monomorphize(
         arena,
         app_module_path.clone(),
         subs_by_module,
         load_config,
-    )?;
+    );
+    let loaded = match load_result {
+        Ok(loaded) => loaded,
+        Err(LoadMonomorphizedError::LoadingProblem(problem)) => {
+            return Err(BuildFileError::LoadingProblem(problem))
+        }
+        Err(LoadMonomorphizedError::ErrorModule(module)) => {
+            return Err(BuildFileError::ErrorModule {
+                module,
+                total_time: compilation_start.elapsed(),
+            })
+        }
+    };
 
     use target_lexicon::Architecture;
     let emit_wasm = matches!(target.architecture, Architecture::Wasm32);
@@ -159,9 +197,7 @@ pub fn build_file<'a>(
         .prefix("roc_app")
         .suffix(&format!(".{}", app_extension))
         .tempfile()
-        .map_err(|err| {
+        .map_err(|err| todo!("TODO Gracefully handle tempfile creation error {:?}", err))?;
-            todo!("TODO Gracefully handle tempfile creation error {:?}", err);
-        })?;
     let app_o_file = app_o_file.path();
     let buf = &mut String::with_capacity(1024);
 
@@ -327,12 +363,12 @@ pub fn build_file<'a>(
                     link_type
                 )
                 .map_err(|_| {
-                    todo!("gracefully handle `ld` failing to spawn.");
+                    todo!("gracefully handle `ld` failing to spawn.")
                 })?;
 
-            let exit_status = child.wait().map_err(|_| {
+            let exit_status = child
-                todo!("gracefully handle error after `ld` spawned");
+                .wait()
-            })?;
+                .map_err(|_| todo!("gracefully handle error after `ld` spawned"))?;
 
             if exit_status.success() {
                 problems

crates/cli/src/lib.rs

@@ -33,10 +33,13 @@ pub mod build;
 mod format;
 pub use format::format;
 
+use crate::build::{BuildFileError, BuildOrdering};
+
 const DEFAULT_ROC_FILENAME: &str = "main.roc";
 
 pub const CMD_BUILD: &str = "build";
 pub const CMD_RUN: &str = "run";
+pub const CMD_DEV: &str = "dev";
 pub const CMD_REPL: &str = "repl";
 pub const CMD_EDIT: &str = "edit";
 pub const CMD_DOCS: &str = "docs";
@@ -210,6 +213,19 @@ pub fn build_app<'a>() -> Command<'a> {
             .arg(roc_file_to_run.clone())
             .arg(args_for_app.clone())
         )
+        .subcommand(Command::new(CMD_DEV)
+            .about("`check` a .roc file, and then run it if there were no errors.")
+            .arg(flag_optimize.clone())
+            .arg(flag_max_threads.clone())
+            .arg(flag_opt_size.clone())
+            .arg(flag_dev.clone())
+            .arg(flag_debug.clone())
+            .arg(flag_time.clone())
+            .arg(flag_linker.clone())
+            .arg(flag_precompiled.clone())
+            .arg(roc_file_to_run.clone())
+            .arg(args_for_app.clone())
+        )
         .subcommand(Command::new(CMD_FORMAT)
             .about("Format a .roc file using standard Roc formatting")
             .arg(
@@ -521,6 +537,10 @@ pub fn build(
         .and_then(|s| s.parse::<u32>().ok())
         .map(|x| x * 1024);
 
+    let build_ordering = match config {
+        BuildAndRunIfNoErrors => BuildOrdering::BuildIfChecks,
+        _ => BuildOrdering::AlwaysBuild,
+    };
     let res_binary_path = build_file(
         &arena,
         &triple,
@@ -533,6 +553,7 @@ pub fn build(
         precompiled,
         threading,
         wasm_dev_stack_bytes,
+        build_ordering,
     );
 
     match res_binary_path {
@@ -633,55 +654,13 @@ pub fn build(
                     x
                 }
                 BuildAndRunIfNoErrors => {
-                    if problems.errors == 0 {
+                    debug_assert!(
-                        if problems.warnings > 0 {
+                        problems.errors == 0,
-                            println!(
+                        "if there are errors, they should have been returned as an error variant"
-                                "\x1B[32m0\x1B[39m errors and \x1B[33m{}\x1B[39m {} found in {} ms.\n\nRunning program…\n\n\x1B[36m{}\x1B[39m",
+                    );
-                                problems.warnings,
+                    if problems.warnings > 0 {
-                                if problems.warnings == 1 {
+                        println!(
-                                    "warning"
+                            "\x1B[32m0\x1B[39m errors and \x1B[33m{}\x1B[39m {} found in {} ms.\n\nRunning program…\n\n\x1B[36m{}\x1B[39m",
-                                } else {
-                                    "warnings"
-                                },
-                                total_time.as_millis(),
-                                "─".repeat(80)
-                            );
-                        }
-
-                        let args = matches.values_of_os(ARGS_FOR_APP).unwrap_or_default();
-
-                        let mut bytes = std::fs::read(&binary_path).unwrap();
-
-                        let x = roc_run(
-                            arena,
-                            opt_level,
-                            triple,
-                            args,
-                            &mut bytes,
-                            expectations,
-                            interns,
-                        );
-                        std::mem::forget(bytes);
-                        x
-                    } else {
-                        let mut output = format!(
-                            "\x1B[{}m{}\x1B[39m {} and \x1B[{}m{}\x1B[39m {} found in {} ms.\n\nYou can run the program anyway with \x1B[32mroc run",
-                            if problems.errors == 0 {
-                                32 // green
-                            } else {
-                                33 // yellow
-                            },
-                            problems.errors,
-                            if problems.errors == 1 {
-                                "error"
-                            } else {
-                                "errors"
-                            },
-                            if problems.warnings == 0 {
-                                32 // green
-                            } else {
-                                33 // yellow
-                            },
                             problems.warnings,
                             if problems.warnings == 1 {
                                 "warning"
@@ -689,22 +668,74 @@ pub fn build(
                                 "warnings"
                             },
                             total_time.as_millis(),
+                            "─".repeat(80)
                         );
-                        // If you're running "main.roc" then you can just do `roc run`
-                        // to re-run the program.
-                        if filename != DEFAULT_ROC_FILENAME {
-                            output.push(' ');
-                            output.push_str(&filename.to_string_lossy());
-                        }
-
-                        println!("{}\x1B[39m", output);
-
-                        Ok(problems.exit_code())
                     }
+
+                    let args = matches.values_of_os(ARGS_FOR_APP).unwrap_or_default();
+
+                    let mut bytes = std::fs::read(&binary_path).unwrap();
+
+                    let x = roc_run(
+                        arena,
+                        opt_level,
+                        triple,
+                        args,
+                        &mut bytes,
+                        expectations,
+                        interns,
+                    );
+                    std::mem::forget(bytes);
+                    x
                 }
             }
         }
-        Err(LoadingProblem::FormattedReport(report)) => {
+        Err(BuildFileError::ErrorModule {
+            mut module,
+            total_time,
+        }) => {
+            debug_assert!(module.total_problems() > 0);
+
+            let problems = roc_build::program::report_problems_typechecked(&mut module);
+
+            let mut output = format!(
+                "\x1B[{}m{}\x1B[39m {} and \x1B[{}m{}\x1B[39m {} found in {} ms.\n\nYou can run the program anyway with \x1B[32mroc run",
+                if problems.errors == 0 {
+                    32 // green
+                } else {
+                    33 // yellow
+                },
+                problems.errors,
+                if problems.errors == 1 {
+                    "error"
+                } else {
+                    "errors"
+                },
+                if problems.warnings == 0 {
+                    32 // green
+                } else {
+                    33 // yellow
+                },
+                problems.warnings,
+                if problems.warnings == 1 {
+                    "warning"
+                } else {
+                    "warnings"
+                },
+                total_time.as_millis(),
+            );
+            // If you're running "main.roc" then you can just do `roc run`
+            // to re-run the program.
+            if filename != DEFAULT_ROC_FILENAME {
+                output.push(' ');
+                output.push_str(&filename.to_string_lossy());
+            }
+
+            println!("{}\x1B[39m", output);
+
+            Ok(problems.exit_code())
+        }
+        Err(BuildFileError::LoadingProblem(LoadingProblem::FormattedReport(report))) => {
             print!("{}", report);
 
             Ok(1)

crates/cli/src/main.rs

@@ -1,9 +1,10 @@
 use roc_build::link::LinkType;
 use roc_cli::build::check_file;
 use roc_cli::{
-    build_app, format, test, BuildConfig, FormatMode, Target, CMD_BUILD, CMD_CHECK, CMD_DOCS,
+    build_app, format, test, BuildConfig, FormatMode, Target, CMD_BUILD, CMD_CHECK, CMD_DEV,
-    CMD_EDIT, CMD_FORMAT, CMD_GLUE, CMD_REPL, CMD_RUN, CMD_TEST, CMD_VERSION, DIRECTORY_OR_FILES,
+    CMD_DOCS, CMD_EDIT, CMD_FORMAT, CMD_GLUE, CMD_REPL, CMD_RUN, CMD_TEST, CMD_VERSION,
-    FLAG_CHECK, FLAG_LIB, FLAG_NO_LINK, FLAG_TARGET, FLAG_TIME, GLUE_FILE, ROC_FILE,
+    DIRECTORY_OR_FILES, FLAG_CHECK, FLAG_LIB, FLAG_NO_LINK, FLAG_TARGET, FLAG_TIME, GLUE_FILE,
+    ROC_FILE,
 };
 use roc_docs::generate_docs_html;
 use roc_error_macros::user_error;
@@ -64,6 +65,20 @@ fn main() -> io::Result<()> {
                 Ok(1)
             }
         }
+        Some((CMD_DEV, matches)) => {
+            if matches.is_present(ROC_FILE) {
+                build(
+                    matches,
+                    BuildConfig::BuildAndRunIfNoErrors,
+                    Triple::host(),
+                    LinkType::Executable,
+                )
+            } else {
+                eprintln!("What .roc file do you want to build? Specify it at the end of the `roc run` command.");
+
+                Ok(1)
+            }
+        }
         Some((CMD_GLUE, matches)) => {
             let input_path = Path::new(matches.value_of_os(ROC_FILE).unwrap());
             let output_path = Path::new(matches.value_of_os(GLUE_FILE).unwrap());

crates/compiler/load/src/lib.rs

@@ -54,6 +54,29 @@ pub fn load_single_threaded<'a>(
     )
 }
 
+#[derive(Debug)]
+#[allow(clippy::large_enum_variant)]
+pub enum LoadMonomorphizedError<'a> {
+    LoadingProblem(LoadingProblem<'a>),
+    /// Errors in the module that should be reported, without compiling the executable.
+    /// Relevant in check-and-then-build mode.
+    ErrorModule(LoadedModule),
+}
+
+impl<'a> From<LoadingProblem<'a>> for LoadMonomorphizedError<'a> {
+    fn from(problem: LoadingProblem<'a>) -> Self {
+        Self::LoadingProblem(problem)
+    }
+}
+
+// HACK only relevant because of some uses of `map_err` that decay into this error, but call `todo` -
+// rustc seems to be unhappy with that.
+impl<'a> From<()> for LoadMonomorphizedError<'a> {
+    fn from(_: ()) -> Self {
+        todo!()
+    }
+}
+
 #[allow(clippy::too_many_arguments)]
 pub fn load_and_monomorphize_from_str<'a>(
     arena: &'a Bump,
@@ -78,14 +101,14 @@ pub fn load_and_monomorphize(
     filename: PathBuf,
     exposed_types: ExposedByModule,
     load_config: LoadConfig,
-) -> Result<MonomorphizedModule<'_>, LoadingProblem<'_>> {
+) -> Result<MonomorphizedModule<'_>, LoadMonomorphizedError<'_>> {
     use LoadResult::*;
 
     let load_start = LoadStart::from_path(arena, filename, load_config.render)?;
 
     match load(arena, load_start, exposed_types, load_config)? {
         Monomorphized(module) => Ok(module),
-        TypeChecked(_) => unreachable!(""),
+        TypeChecked(module) => Err(LoadMonomorphizedError::ErrorModule(module)),
     }
 }
 

crates/compiler/load_internal/src/file.rs

@@ -130,13 +130,15 @@ pub enum ExecutionMode {
     Test,
     Check,
     Executable,
+    /// Like [`ExecutionMode::Executable`], but stops in the presence of type errors.
+    ExecutableIfCheck,
 }
 
 impl ExecutionMode {
     fn goal_phase(&self) -> Phase {
         match self {
             ExecutionMode::Test | ExecutionMode::Executable => Phase::MakeSpecializations,
-            ExecutionMode::Check => Phase::SolveTypes,
+            ExecutionMode::Check | ExecutionMode::ExecutableIfCheck => Phase::SolveTypes,
         }
     }
 }
@@ -168,6 +170,22 @@ struct ModuleCache<'a> {
     sources: MutMap<ModuleId, (PathBuf, &'a str)>,
 }
 
+impl<'a> ModuleCache<'a> {
+    pub fn total_problems(&self) -> usize {
+        let mut total = 0;
+
+        for problems in self.can_problems.values() {
+            total += problems.len();
+        }
+
+        for problems in self.type_problems.values() {
+            total += problems.len();
+        }
+
+        total
+    }
+}
+
 impl Default for ModuleCache<'_> {
     fn default() -> Self {
         let mut module_names = MutMap::default();
@@ -2379,12 +2397,35 @@ fn update<'a>(
                     .extend(solved_module.aliases.keys().copied());
             }
 
-            if is_host_exposed && state.goal_phase() == Phase::SolveTypes {
+            let finish_type_checking = is_host_exposed &&
+                (state.goal_phase() == Phase::SolveTypes)
+                // If we're running in check-and-then-build mode, only exit now there are errors.
+                && (!matches!(state.exec_mode, ExecutionMode::ExecutableIfCheck) || state.module_cache.total_problems() > 0);
+
+            if finish_type_checking {
                 debug_assert!(work.is_empty());
                 debug_assert!(state.dependencies.solved_all());
 
                 state.timings.insert(module_id, module_timing);
 
+                if matches!(state.exec_mode, ExecutionMode::ExecutableIfCheck) {
+                    // We there may outstanding modules in the typecheked cache whose ident IDs
+                    // aren't registered; transfer all of their idents over to the state, since
+                    // we're now done and ready to report errors.
+                    for (
+                        module_id,
+                        TypeCheckedModule {
+                            ident_ids,
+                            module_timing,
+                            ..
+                        },
+                    ) in state.module_cache.typechecked.drain()
+                    {
+                        state.constrained_ident_ids.insert(module_id, ident_ids);
+                        state.timings.insert(module_id, module_timing);
+                    }
+                }
+
                 let documentation = {
                     let mut empty = MutMap::default();
                     std::mem::swap(&mut empty, &mut state.module_cache.documentation);
@@ -2421,7 +2462,9 @@ fn update<'a>(
                     },
                 );
 
-                if state.goal_phase() > Phase::SolveTypes {
+                if state.goal_phase() > Phase::SolveTypes
+                    || matches!(state.exec_mode, ExecutionMode::ExecutableIfCheck)
+                {
                     let layout_cache = state
                         .layout_caches
                         .pop()
@@ -2446,6 +2489,25 @@ fn update<'a>(
                     state.timings.insert(module_id, module_timing);
                 }
 
+                let work = if is_host_exposed
+                    && matches!(state.exec_mode, ExecutionMode::ExecutableIfCheck)
+                {
+                    debug_assert!(
+                        work.is_empty(),
+                        "work left over after host exposed is checked"
+                    );
+
+                    // Update the goal phase to target full codegen.
+                    state.exec_mode = ExecutionMode::Executable;
+
+                    // Load the find + make specializations portion of the dependency graph.
+                    state
+                        .dependencies
+                        .load_find_and_make_specializations_after_check()
+                } else {
+                    work
+                };
+
                 start_tasks(arena, &mut state, work, injector, worker_listeners)?;
             }
 
@@ -2803,7 +2865,7 @@ fn finish_specialization(
     let entry_point = {
         match exec_mode {
             ExecutionMode::Test => EntryPoint::Test,
-            ExecutionMode::Executable => {
+            ExecutionMode::Executable | ExecutionMode::ExecutableIfCheck => {
                 let path_to_platform = {
                     use PlatformPath::*;
                     let package_name = match platform_path {
@@ -5000,7 +5062,9 @@ fn build_pending_specializations<'a>(
                 // skip expectations if we're not going to run them
                 match execution_mode {
                     ExecutionMode::Test => { /* fall through */ }
-                    ExecutionMode::Check | ExecutionMode::Executable => continue,
+                    ExecutionMode::Check
+                    | ExecutionMode::Executable
+                    | ExecutionMode::ExecutableIfCheck => continue,
                 }
 
                 // mark this symbol as a top-level thunk before any other work on the procs
@@ -5074,7 +5138,9 @@ fn build_pending_specializations<'a>(
                 // skip expectations if we're not going to run them
                 match execution_mode {
                     ExecutionMode::Test => { /* fall through */ }
-                    ExecutionMode::Check | ExecutionMode::Executable => continue,
+                    ExecutionMode::Check
+                    | ExecutionMode::Executable
+                    | ExecutionMode::ExecutableIfCheck => continue,
                 }
 
                 // mark this symbol as a top-level thunk before any other work on the procs

crates/compiler/load_internal/src/work.rs

@@ -166,11 +166,10 @@ impl<'a> Dependencies<'a> {
             }
         }
 
-        if goal_phase >= MakeSpecializations {
+        // Add "make specialization" dependents. Even if we're not targeting making
-            // Add make specialization dependents
+        // specializations right now, we may re-enter to do so later.
-            self.make_specializations_dependents
+        self.make_specializations_dependents
-                .add_succ(module_id, dependencies.iter().map(|dep| *dep.as_inner()));
+            .add_succ(module_id, dependencies.iter().map(|dep| *dep.as_inner()));
-        }
 
         // add dependencies for self
         // phase i + 1 of a file always depends on phase i being completed
@@ -374,6 +373,80 @@ impl<'a> Dependencies<'a> {
         }
     }
 
+    /// Loads the dependency graph to find and make specializations, and returns the next jobs to
+    /// be run.
+    ///
+    /// This should be used when the compiler wants to build or run a Roc executable if and only if
+    /// previous stages succeed; in such cases we load the dependency graph dynamically.
+    pub fn load_find_and_make_specializations_after_check(&mut self) -> MutSet<(ModuleId, Phase)> {
+        let mut output = MutSet::default();
+
+        let mut make_specializations_dependents = MakeSpecializationsDependents::default();
+        let default_make_specializations_dependents_len = make_specializations_dependents.0.len();
+        std::mem::swap(
+            &mut self.make_specializations_dependents,
+            &mut make_specializations_dependents,
+        );
+
+        for (&module, info) in make_specializations_dependents.0.iter_mut() {
+            debug_assert!(self.status.get_mut(&Job::Step(module, Phase::FindSpecializations)).is_none(), "should only have targeted solving types, but there is already a goal to find specializations");
+            debug_assert!(self.status.get_mut(&Job::Step(module, Phase::MakeSpecializations)).is_none(), "should only have targeted solving types, but there is already a goal to make specializations");
+            debug_assert!(
+                module == ModuleId::DERIVED_GEN || info.succ.contains(&ModuleId::DERIVED_GEN),
+                "derived module not accounted for in {:?}",
+                (module, info)
+            );
+
+            let mut has_find_specialization_dep = false;
+            for &module_dep in info.succ.iter() {
+                // The modules in `succ` are the modules for which specializations should be made
+                // after the current one. But, their specializations should be found before the
+                // current one.
+                if module_dep != ModuleId::DERIVED_GEN {
+                    // We never find specializations for DERIVED_GEN
+                    self.add_dependency(module, module_dep, Phase::FindSpecializations);
+                    has_find_specialization_dep = true;
+                }
+
+                self.add_dependency(module_dep, module, Phase::MakeSpecializations);
+                self.add_dependency(ModuleId::DERIVED_GEN, module, Phase::MakeSpecializations);
+
+                // `module_dep` can't make its specializations until the current module does.
+                info.has_pred = true;
+            }
+
+            if module != ModuleId::DERIVED_GEN {
+                self.add_to_status_for_phase(module, Phase::FindSpecializations);
+                self.add_dependency_help(
+                    module,
+                    module,
+                    Phase::MakeSpecializations,
+                    Phase::FindSpecializations,
+                );
+            }
+            self.add_to_status_for_phase(module, Phase::MakeSpecializations);
+
+            if !has_find_specialization_dep && module != ModuleId::DERIVED_GEN {
+                // We don't depend on any other modules having their specializations found first,
+                // so start finding specializations from this module.
+                output.insert((module, Phase::FindSpecializations));
+            }
+        }
+
+        std::mem::swap(
+            &mut self.make_specializations_dependents,
+            &mut make_specializations_dependents,
+        );
+        debug_assert_eq!(
+            make_specializations_dependents.0.len(),
+            default_make_specializations_dependents_len,
+            "more modules were added to the graph: {:?}",
+            make_specializations_dependents
+        );
+
+        output
+    }
+
     /// Load the entire "make specializations" dependency graph and start from the top.
     pub fn reload_make_specialization_pass(&mut self) -> MutSet<(ModuleId, Phase)> {
         let mut output = MutSet::default();