Don't restart for removed or renamed modules (#290)

Ghcid crashes when modules are removed/renamed because all it can do to
the underlying GHCi session is `:reload`, and [a GHCi bug][1] means that
the GHCi session will crash when this happens.

Because ghciwatch tracks which files/modules are loaded in the session,
it can accurately `:unadd` files from the session when they're removed
on disk, without requiring the session be restared.

This should speed up development.

[1]: https://gitlab.haskell.org/ghc/ghc/-/issues/11596



I'm going to cut 1.0 with this while we're at it.

- [x] Labeled the PR with `patch`, `minor`, or `major` to request a
version bump when it's merged.
- [x] Updated the user manual in `docs/`.
- [x] Added integration / regression tests in `tests/`.

Author: 9999years

README.md

@@ -21,7 +21,7 @@ when source files change.
 
 - GHCi output is displayed to the user as soon as it's printed.
 - Ghciwatch can handle new modules, removed modules, or moved modules without a
-  hitch, so you don't need to manually restart it.
+  hitch
 - A variety of [lifecycle
   hooks](https://mercurytechnologies.github.io/ghciwatch/lifecycle-hooks.html)
   let you run Haskell code or shell commands on a variety of events.

docs/introduction.md

@@ -9,6 +9,7 @@ when source files change.
 
 - GHCi output is displayed to the user as soon as it's printed.
 - Ghciwatch can handle new modules, removed modules, or moved modules without a
+  hitch
 - A variety of [lifecycle hooks](lifecycle-hooks.md) let you run Haskell code
   or shell commands on a variety of events.
   - Run a test suite with [`--test-ghci
@@ -20,7 +21,6 @@ when source files change.
 - [Custom globs](cli.md#--reload-glob) can be supplied to reload or restart the
   GHCi session when non-Haskell files (like templates or database schema
   definitions) change.
-  hitch, so you don't need to manually restart it.
 - Ghciwatch can [clear the screen between reloads](cli.md#--clear).
 - Compilation errors can be written to a file with [`--error-file`](cli.md#--error-file), for
   compatibility with [ghcid's][ghcid] `--outputfile` option.

src/cli.rs

@@ -55,15 +55,15 @@ use crate::normal_path::NormalPath;
     override_usage = "ghciwatch [--command SHELL_COMMAND] [--watch PATH] [OPTIONS ...]"
 )]
 pub struct Opts {
-    /// A shell command which starts a `ghci` REPL, e.g. `ghci` or `cabal v2-repl` or similar.
+    /// A shell command which starts a GHCi REPL, e.g. `ghci` or `cabal v2-repl` or similar.
     ///
-    /// This is used to launch the underlying `ghci` session that `ghciwatch` controls.
+    /// This is used to launch the underlying GHCi session that `ghciwatch` controls.
     ///
     /// May contain quoted arguments which will be parsed in a `sh`-like manner.
     #[arg(long, value_name = "SHELL_COMMAND")]
     pub command: Option<ClonableCommand>,
 
-    /// A Haskell source file to load into a `ghci` REPL.
+    /// A Haskell source file to load into a GHCi REPL.
     ///
     /// Shortcut for `--command 'ghci PATH'`. Conflicts with `--command`.
     #[arg(value_name = "FILE", conflicts_with = "command")]
@@ -154,7 +154,7 @@ pub struct WatchOpts {
     #[arg(long = "watch", value_name = "PATH")]
     pub paths: Vec<NormalPath>,
 
-    /// Reload the `ghci` session when paths matching this glob change.
+    /// Reload the GHCi session when paths matching this glob change.
     ///
     /// By default, only changes to Haskell source files trigger reloads. If you'd like to exclude
     /// some files from that, you can add an ignore glob here, like `!src/my-special-dir/**/*.hs`.
@@ -169,13 +169,9 @@ pub struct WatchOpts {
     #[arg(long = "reload-glob")]
     pub reload_globs: Vec<String>,
 
-    /// Restart the `ghci` session when paths matching this glob change.
+    /// Restart the GHCi session when paths matching this glob change.
     ///
-    /// By default, only changes to `.cabal` or `.ghci` files or Haskell source files being
-    /// moved/removed will trigger restarts.
-    ///
-    /// Due to [a `ghci` bug][1], the `ghci` session must be restarted when Haskell modules are removed
-    /// or renamed.
+    /// By default, only changes to `.cabal` or `.ghci` files will trigger restarts.
     ///
     /// See `--reload-globs` for more details.
     ///

src/ghci/mod.rs

@@ -379,6 +379,7 @@ impl Ghci {
         let mut needs_restart = Vec::new();
         let mut needs_reload = Vec::new();
         let mut needs_add = Vec::new();
+        let mut needs_remove = Vec::new();
         for event in events {
             let path = event.as_path();
             let path = self.relative_path(path)?;
@@ -395,7 +396,7 @@ impl Ghci {
             );
 
             // Don't restart if we've explicitly ignored this path in a glob.
-            if (!restart_match.is_ignore()
+            if !restart_match.is_ignore()
                 // Restart on `.cabal` and `.ghci` files.
                 && (path
                     .extension()
@@ -406,31 +407,21 @@ impl Ghci {
                     .map(|name| name == ".ghci")
                     .unwrap_or(false)
                 // Restart on explicit restart globs.
-                || restart_match.is_whitelist()))
-                // Even if we've explicitly ignored this path in a glob, `ghci` can't cope with
-                // removed modules, so we need to restart when modules are removed or renamed.
-                //
-                // See: https://gitlab.haskell.org/ghc/ghc/-/issues/11596
-                //
-                // TODO: I should investigate if `:unadd` works for some classes of removed
-                // modules.
-                || (matches!(event, FileEvent::Remove(_))
-                    && path_is_haskell_source_file
-                    && self.targets.contains_source_path(&path))
+                || restart_match.is_whitelist())
             {
                 // Restart for this path.
                 tracing::debug!(%path, "Needs restart");
                 needs_restart.push(path);
-            } else if reload_match.is_whitelist() {
-                // Extra extensions are always reloaded, never added.
-                tracing::debug!(%path, "Needs reload");
-                needs_reload.push(path);
-            } else if !reload_match.is_ignore()
-                // Don't reload if we've explicitly ignored this path in a glob.
-                // Otherwise, reload when Haskell files are modified.
-                && matches!(event, FileEvent::Modify(_))
+            } else if reload_match.is_ignore() {
+                // Ignoring this path, continue.
+            } else if matches!(event, FileEvent::Remove(_))
                 && path_is_haskell_source_file
+                && self.targets.contains_source_path(&path)
             {
+                tracing::debug!(%path, "Needs remove");
+                needs_remove.push(path);
+            } else if matches!(event, FileEvent::Modify(_)) && path_is_haskell_source_file {
+                // Otherwise, reload when Haskell files are modified.
                 if self.targets.contains_source_path(&path) {
                     // We can `:reload` paths in the target set.
                     tracing::debug!(%path, "Needs reload");
@@ -440,13 +431,18 @@ impl Ghci {
                     tracing::debug!(%path, "Needs add");
                     needs_add.push(path);
                 }
+            } else if reload_match.is_whitelist() {
+                // Extra extensions are always reloaded, never added.
+                tracing::debug!(%path, "Needs reload");
+                needs_reload.push(path);
             }
         }
 
         Ok(ReloadActions {
             needs_restart,
             needs_reload,
             needs_add,
+            needs_remove,
         })
     }
 
@@ -480,20 +476,26 @@ impl Ghci {
 
         let mut log = CompilationLog::default();
 
-        if actions.needs_add_or_reload() {
+        if actions.needs_modify() {
             self.opts.clear();
             self.run_hooks(LifecycleEvent::Reload(hooks::When::Before), &mut log)
                 .await?;
         }
 
+        if !actions.needs_remove.is_empty() {
+            tracing::info!(
+                "Removing modules from ghci:\n{}",
+                format_bulleted_list(&actions.needs_remove)
+            );
+            self.remove_modules(&actions.needs_remove, &mut log).await?;
+        }
+
         if !actions.needs_add.is_empty() {
             tracing::info!(
                 "Adding modules to ghci:\n{}",
                 format_bulleted_list(&actions.needs_add)
             );
-            for path in &actions.needs_add {
-                self.add_module(path, &mut log).await?;
-            }
+            self.add_modules(&actions.needs_add, &mut log).await?;
         }
 
         if !actions.needs_reload.is_empty() {
@@ -506,7 +508,7 @@ impl Ghci {
                 .await?;
         }
 
-        if actions.needs_add_or_reload() {
+        if actions.needs_modify() {
             self.finish_compilation(
                 start_instant,
                 &mut log,
@@ -641,6 +643,21 @@ impl Ghci {
         Ok(())
     }
 
+    /// Remove all `eval_commands` for the given paths.
+    #[instrument(skip_all, level = "debug")]
+    async fn clear_eval_commands_for_paths(
+        &mut self,
+        paths: impl IntoIterator<Item = impl Borrow<NormalPath>>,
+    ) {
+        if !self.opts.enable_eval {
+            return;
+        }
+
+        for path in paths {
+            self.eval_commands.remove(path.borrow());
+        }
+    }
+
     /// Read and parse eval commands from the given `path`.
     #[instrument(level = "trace")]
     async fn parse_eval_commands(path: &Utf8Path) -> miette::Result<Vec<EvalCommand>> {
@@ -653,38 +670,32 @@ impl Ghci {
         Ok(commands)
     }
 
-    /// `:add` a module to the `ghci` session by path.
-    ///
-    /// Optionally returns a compilation result.
+    /// `:add` a module or modules to the `ghci` session by path.
     #[instrument(skip(self), level = "debug")]
-    async fn add_module(
+    async fn add_modules(
         &mut self,
-        path: &NormalPath,
+        paths: &[NormalPath],
         log: &mut CompilationLog,
     ) -> miette::Result<()> {
-        if self.targets.contains_source_path(path.absolute()) {
-            tracing::debug!(%path, "Skipping `:add`ing already-loaded path");
-            return Ok(());
-        }
+        let modules = self.targets.format_modules(&self.search_paths, paths)?;
 
         self.stdin
-            .add_module(&mut self.stdout, path.relative(), log)
+            .add_modules(&mut self.stdout, &modules, log)
             .await?;
 
-        self.targets
-            .insert_source_path(path.clone(), TargetKind::Path);
+        for path in paths {
+            self.targets
+                .insert_source_path(path.clone(), TargetKind::Path);
+        }
 
-        self.refresh_eval_commands_for_paths(std::iter::once(path))
-            .await?;
+        self.refresh_eval_commands_for_paths(paths).await?;
 
         Ok(())
     }
 
     /// `:add *` a module to the `ghci` session by path.
     ///
     /// This forces it to be interpreted.
-    ///
-    /// Optionally returns a compilation result.
     #[instrument(skip(self), level = "debug")]
     async fn interpret_module(
         &mut self,
@@ -707,6 +718,31 @@ impl Ghci {
         Ok(())
     }
 
+    /// `:unadd` a module or modules from the `ghci` session by path.
+    #[instrument(skip(self), level = "debug")]
+    async fn remove_modules(
+        &mut self,
+        paths: &[NormalPath],
+        log: &mut CompilationLog,
+    ) -> miette::Result<()> {
+        // Each `:unadd` implicitly reloads as well, so we have to `:unadd` all the modules in a
+        // single command so that GHCi doesn't try to load a bunch of removed modules after each
+        // one.
+        let modules = self.targets.format_modules(&self.search_paths, paths)?;
+
+        self.stdin
+            .remove_modules(&mut self.stdout, &modules, log)
+            .await?;
+
+        for path in paths {
+            self.targets.remove_source_path(path);
+            self.clear_eval_commands_for_paths(std::iter::once(path))
+                .await;
+        }
+
+        Ok(())
+    }
+
     /// Stop this `ghci` session and cancel the async tasks associated with it.
     #[instrument(skip_all, level = "debug")]
     async fn stop(&mut self) -> miette::Result<()> {
@@ -851,12 +887,14 @@ struct ReloadActions {
     needs_reload: Vec<NormalPath>,
     /// Paths to modules which need an `:add`.
     needs_add: Vec<NormalPath>,
+    /// Paths to modules which need an `:unadd`.
+    needs_remove: Vec<NormalPath>,
 }
 
 impl ReloadActions {
-    /// Do any modules need to be added or reloaded?
-    fn needs_add_or_reload(&self) -> bool {
-        !self.needs_add.is_empty() || !self.needs_reload.is_empty()
+    /// Do any modules need to be added, removed, or reloaded?
+    fn needs_modify(&self) -> bool {
+        !self.needs_add.is_empty() || !self.needs_reload.is_empty() || !self.needs_remove.is_empty()
     }
 
     /// Is a session restart needed?
@@ -868,7 +906,7 @@ impl ReloadActions {
     fn kind(&self) -> GhciReloadKind {
         if self.needs_restart() {
             GhciReloadKind::Restart
-        } else if self.needs_add_or_reload() {
+        } else if self.needs_modify() {
             GhciReloadKind::Reload
         } else {
             GhciReloadKind::None
@@ -881,7 +919,7 @@ impl ReloadActions {
 pub enum GhciReloadKind {
     /// Noop. No actions needed.
     None,
-    /// Reload and/or add modules. Can be interrupted.
+    /// Reload, add, and/or remove modules. Can be interrupted.
     Reload,
     /// Restart the whole session. Cannot be interrupted.
     Restart,

src/ghci/parse/module_set.rs

@@ -2,6 +2,7 @@ use std::borrow::Borrow;
 use std::cmp::Eq;
 use std::collections::hash_map::Keys;
 use std::collections::HashMap;
+use std::fmt::Display;
 use std::hash::Hash;
 use std::path::Path;
 
@@ -61,6 +62,17 @@ impl ModuleSet {
         }
     }
 
+    /// Remove a source path from this module set.
+    ///
+    /// Returns the target's kind, if it was present in the set.
+    pub fn remove_source_path<P>(&mut self, path: &P) -> Option<TargetKind>
+    where
+        NormalPath: Borrow<P>,
+        P: Hash + Eq + ?Sized,
+    {
+        self.modules.remove(path)
+    }
+
     /// Get the name used to refer to the given module path when importing it.
     ///
     /// If the module isn't imported, a path will be returned.
@@ -99,13 +111,32 @@ impl ModuleSet {
         }
     }
 
+    /// Format modules for adding or removing from a GHCi session.
+    ///
+    /// See [`ModuleSet::module_import_name`].
+    pub fn format_modules(
+        &self,
+        show_paths: &ShowPaths,
+        modules: &[NormalPath],
+    ) -> miette::Result<String> {
+        modules
+            .iter()
+            .map(|path| {
+                self.module_import_name(show_paths, path)
+                    .map(|module| module.name)
+            })
+            .collect::<Result<Vec<_>, _>>()
+            .map(|modules| modules.join(" "))
+    }
+
     /// Iterate over the source paths in this module set.
     pub fn iter(&self) -> Keys<'_, NormalPath, TargetKind> {
         self.modules.keys()
     }
 }
 
 /// Information about a module to be imported into a `ghci` session.
+#[derive(Debug, Clone)]
 pub struct ImportInfo {
     /// The name to refer to the module by.
     ///
@@ -117,3 +148,9 @@ pub struct ImportInfo {
     /// Whether the module is already loaded in the `ghci` session.
     pub loaded: bool,
 }
+
+impl Display for ImportInfo {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}", self.name)
+    }
+}