Fix "module defined in multiple files" with eval commands (#214)

Our favorite bug is back! We have a situation where `ghciwatch` loads
all the modules at the start of the session, so they're all loaded:

```
ghci> :show targets
Foo
Bar
Baz
```

Then, an eval command in (for example) module `Foo` is executed. To
start with, the module is added and explicitly interpreted:

```
ghci> :add *src/Foo.hs
```

Unfortunately, this always uses the module path (see
[#171](#171)), so
if `ghciwatch` wasn't the one to load the module (e.g. it was loaded at
the start of the session), the module path and module name will conflict
and lead tot he dreaded "module is defined in multiple files".

See:
[https://gitlab.haskell.org/ghc/ghc/-/issues/13254#note_525037](https://gitlab.haskell.org/ghc/ghc/-/issues/13254#note_525037)

# Solution

I think we can fix this by keeping track of how each module is added to
the session — as a path or as a module name — and then only using that
form going forward.

src/ghci/mod.rs

@@ -80,6 +80,8 @@ use crate::shutdown::ShutdownHandle;
 use crate::CommandExt;
 use crate::StringCase;
 
+use self::parse::TargetKind;
+
 /// The `ghci` prompt we use. Should be unique enough, but maybe we can make it better with Unicode
 /// private-use-area codepoints or something in the future.
 pub const PROMPT: &str = "###~GHCIWATCH-PROMPT~###";
@@ -666,7 +668,8 @@ impl Ghci {
             .add_module(&mut self.stdout, path.relative(), log)
             .await?;
 
-        self.targets.insert_source_path(path.clone());
+        self.targets
+            .insert_source_path(path.clone(), TargetKind::Path);
 
         self.refresh_eval_commands_for_paths(std::iter::once(path))
             .await?;
@@ -685,11 +688,15 @@ impl Ghci {
         path: &NormalPath,
         log: &mut CompilationLog,
     ) -> miette::Result<()> {
+        let (import_name, _target_kind) =
+            self.targets.module_import_name(&self.search_paths, path)?;
+
         self.stdin
-            .interpret_module(&mut self.stdout, path.relative(), log)
+            .interpret_module(&mut self.stdout, &import_name, log)
             .await?;
 
-        self.targets.insert_source_path(path.clone());
+        self.targets
+            .insert_source_path(path.clone(), TargetKind::Path);
 
         self.refresh_eval_commands_for_paths(std::iter::once(path))
             .await?;

src/ghci/parse/mod.rs

@@ -8,6 +8,7 @@ mod module_and_files;
 mod module_set;
 mod show_paths;
 mod show_targets;
+mod target_kind;
 
 use haskell_grammar::module_name;
 use lines::rest_of_line;
@@ -26,3 +27,4 @@ pub use module_set::ModuleSet;
 pub use show_paths::parse_show_paths;
 pub use show_paths::ShowPaths;
 pub use show_targets::parse_show_targets;
+pub use target_kind::TargetKind;

src/ghci/parse/module_set.rs

@@ -1,37 +1,42 @@
 use std::borrow::Borrow;
 use std::cmp::Eq;
-use std::collections::hash_set::Iter;
-use std::collections::HashSet;
+use std::collections::hash_map::Keys;
+use std::collections::HashMap;
 use std::hash::Hash;
 use std::path::Path;
 
 use crate::normal_path::NormalPath;
 
+use super::ShowPaths;
+use super::TargetKind;
+
 /// A collection of source paths, retaining information about loaded modules in a `ghci`
 /// session.
 #[derive(Debug, Clone, Default, PartialEq, Eq)]
 pub struct ModuleSet {
-    set: HashSet<NormalPath>,
+    modules: HashMap<NormalPath, TargetKind>,
 }
 
 impl ModuleSet {
     /// Construct a `ModuleSet` from an iterator of module source paths.
     pub fn from_paths(
-        paths: impl IntoIterator<Item = impl AsRef<Path>>,
+        paths: impl IntoIterator<Item = (impl AsRef<Path>, TargetKind)>,
         current_dir: impl AsRef<Path>,
     ) -> miette::Result<Self> {
         let current_dir = current_dir.as_ref();
         Ok(Self {
-            set: paths
+            modules: paths
                 .into_iter()
-                .map(|path| NormalPath::new(path.as_ref(), current_dir))
+                .map(|(path, kind)| {
+                    NormalPath::new(path.as_ref(), current_dir).map(|path| (path, kind))
+                })
                 .collect::<Result<_, _>>()?,
         })
     }
 
     /// Get the number of modules in this set.
     pub fn len(&self) -> usize {
-        self.set.len()
+        self.modules.len()
     }
 
     /// Determine if a module with the given source path is contained in this module set.
@@ -40,18 +45,47 @@ impl ModuleSet {
         NormalPath: Borrow<P>,
         P: Hash + Eq + ?Sized,
     {
-        self.set.contains(path)
+        self.modules.contains_key(path)
     }
 
     /// Add a source path to this module set.
     ///
     /// Returns whether the value was newly inserted.
-    pub fn insert_source_path(&mut self, path: NormalPath) -> bool {
-        self.set.insert(path)
+    pub fn insert_source_path(&mut self, path: NormalPath, kind: TargetKind) -> bool {
+        self.modules.insert(path, kind).is_some()
+    }
+
+    /// 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.
+    ///
+    /// Otherwise, the form used to import the module originally will be used. Generally this is a
+    /// path if `ghciwatch` imported the module, and a module name if `ghci` imported the module on
+    /// startup.
+    ///
+    /// See: <https://gitlab.haskell.org/ghc/ghc/-/issues/13254#note_525037>
+    pub fn module_import_name(
+        &self,
+        show_paths: &ShowPaths,
+        path: &NormalPath,
+    ) -> miette::Result<(String, TargetKind)> {
+        match self.modules.get(path) {
+            Some(kind) => match kind {
+                TargetKind::Path => Ok((path.relative().to_string(), *kind)),
+                TargetKind::Module => {
+                    let module = show_paths.path_to_module(path)?;
+                    Ok((module, *kind))
+                }
+            },
+            None => {
+                let path = show_paths.make_relative(path)?;
+                Ok((path.into_relative().into_string(), TargetKind::Path))
+            }
+        }
     }
 
     /// Iterate over the source paths in this module set.
-    pub fn iter(&self) -> Iter<'_, NormalPath> {
-        self.set.iter()
+    pub fn iter(&self) -> Keys<'_, NormalPath, TargetKind> {
+        self.modules.keys()
     }
 }

src/ghci/parse/show_paths.rs

@@ -19,6 +19,7 @@ use crate::haskell_source_file::HASKELL_SOURCE_EXTENSIONS;
 use crate::normal_path::NormalPath;
 
 use super::lines::until_newline;
+use super::TargetKind;
 
 /// Parsed `:show paths` output.
 #[derive(Debug, Clone, PartialEq, Eq)]
@@ -36,13 +37,13 @@ impl ShowPaths {
     }
 
     /// Convert a target (from `:show targets` output) to a module source path.
-    pub fn target_to_path(&self, target: &str) -> miette::Result<Utf8PathBuf> {
+    pub fn target_to_path(&self, target: &str) -> miette::Result<(Utf8PathBuf, TargetKind)> {
         let target_path = Utf8Path::new(target);
         if is_haskell_source_file(target_path) {
             // The target is already a path.
             if let Some(path) = self.target_path_to_path(target_path) {
                 tracing::trace!(%path, %target, "Target is path");
-                return Ok(path);
+                return Ok((path, TargetKind::Path));
             }
         } else {
             // Else, split by `.` to get path components.
@@ -54,7 +55,7 @@ impl ShowPaths {
 
                 if let Some(path) = self.target_path_to_path(&path) {
                     tracing::trace!(%path, %target, "Found path for target");
-                    return Ok(path);
+                    return Ok((path, TargetKind::Module));
                 }
             }
         }

src/ghci/parse/show_targets.rs

@@ -5,12 +5,13 @@ use winnow::Parser;
 
 use super::lines::until_newline;
 use super::show_paths::ShowPaths;
+use super::TargetKind;
 
 /// Parse `:show targets` output into a set of module source paths.
 pub fn parse_show_targets(
     search_paths: &ShowPaths,
     input: &str,
-) -> miette::Result<Vec<Utf8PathBuf>> {
+) -> miette::Result<Vec<(Utf8PathBuf, TargetKind)>> {
     let targets: Vec<_> = repeat(0.., until_newline)
         .parse(input)
         .map_err(|err| miette!("{err}"))?;
@@ -43,6 +44,7 @@ mod tests {
                 indoc!(
                     "
                     src/MyLib.hs
+                    MyLib.hs
                     TestMain
                     MyLib
                     MyModule
@@ -51,10 +53,26 @@ mod tests {
             )
             .unwrap(),
             vec![
-                Utf8PathBuf::from("tests/data/simple/src/MyLib.hs"),
-                Utf8PathBuf::from("tests/data/simple/test/TestMain.hs"),
-                Utf8PathBuf::from("tests/data/simple/src/MyLib.hs"),
-                Utf8PathBuf::from("tests/data/simple/src/MyModule.hs"),
+                (
+                    Utf8PathBuf::from("tests/data/simple/src/MyLib.hs"),
+                    TargetKind::Path
+                ),
+                (
+                    Utf8PathBuf::from("tests/data/simple/src/MyLib.hs"),
+                    TargetKind::Path
+                ),
+                (
+                    Utf8PathBuf::from("tests/data/simple/test/TestMain.hs"),
+                    TargetKind::Module
+                ),
+                (
+                    Utf8PathBuf::from("tests/data/simple/src/MyLib.hs"),
+                    TargetKind::Module
+                ),
+                (
+                    Utf8PathBuf::from("tests/data/simple/src/MyModule.hs"),
+                    TargetKind::Module
+                ),
             ]
         );
     }

src/ghci/parse/target_kind.rs

@@ -0,0 +1,12 @@
+/// Entries in `:show targets` can be one of two types: module paths or module names (with `.` in
+/// place of path separators). Due to a `ghci` bug, the module can only be referred to as whichever
+/// form it was originally added as (see below), so we use this to track how we refer to modules.
+///
+/// See: <https://gitlab.haskell.org/ghc/ghc/-/issues/13254#note_525037>
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+pub enum TargetKind {
+    /// A target named by its source path.
+    Path,
+    /// A target named by its module name.
+    Module,
+}

src/ghci/stdin.rs

@@ -125,12 +125,12 @@ impl GhciStdin {
     pub async fn interpret_module(
         &mut self,
         stdout: &mut GhciStdout,
-        path: &Utf8Path,
+        module: &str,
         log: &mut CompilationLog,
     ) -> miette::Result<()> {
         // `:add *` forces the module to be interpreted, even if it was already loaded from
         // bytecode. This is necessary to access the module's top-level binds for the eval feature.
-        self.write_line(stdout, &format!(":add *{path}\n"), log)
+        self.write_line(stdout, &format!(":add *{module}\n"), log)
             .await
     }
 

src/normal_path.rs

@@ -61,6 +61,18 @@ impl NormalPath {
     pub fn relative(&self) -> &Utf8Path {
         self.relative.as_deref().unwrap_or_else(|| self.absolute())
     }
+
+    /// Get the absolute path, consuming this value.
+    pub fn into_absolute(self) -> Utf8PathBuf {
+        self.normal
+    }
+
+    /// Get the relative path, consuming this value.
+    ///
+    /// If no relative path is present, the absolute (normalized) path is used instead.
+    pub fn into_relative(self) -> Utf8PathBuf {
+        self.relative.unwrap_or(self.normal)
+    }
 }
 
 // Hash, Eq, and Ord delegate to the normalized path.

tests/eval.rs

@@ -30,9 +30,13 @@ async fn can_eval_commands() {
         .await
         .expect("ghciwatch didn't start in time");
 
-    let eval_message = BaseMatcher::message(r"MyModule.hs:\d+:\d+: example \+\+ example");
+    let defined_in_multiple_files =
+        BaseMatcher::message("Read stderr line").with_field("line", "defined in multiple files");
+
+    let eval_message = BaseMatcher::message(r"MyModule.hs:\d+:\d+: example \+\+ example")
+        .but_not(defined_in_multiple_files.clone());
     session
-        .assert_logged_or_wait(&eval_message)
+        .assert_logged_or_wait(eval_message.clone())
         .await
         .expect("ghciwatch evals commands");
     session
@@ -41,6 +45,14 @@ async fn can_eval_commands() {
         )
         .await
         .expect("ghciwatch evals commands");
+    session
+        .assert_logged_or_wait(
+            BaseMatcher::span_close()
+                .in_leaf_spans(["run_ghci", "initialize"])
+                .but_not(defined_in_multiple_files.clone()),
+        )
+        .await
+        .expect("ghciwatch finishes initializing");
 
     // Erase the command.
     session.fs().replace(module_path, cmd, "").await.unwrap();