From 77ef9298ba07b43c0ddbdce612c42336ec11cf16 Mon Sep 17 00:00:00 2001
From: Rebecca Turner <rbt@sent.as>
Date: Mon, 15 Apr 2024 13:04:40 -0700
Subject: [PATCH] Fix "module defined in multiple files" with eval commands
(#214)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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](https://github.com/MercuryTechnologies/ghciwatch/pull/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 | 13 ++++++--
src/ghci/parse/mod.rs | 2 ++
src/ghci/parse/module_set.rs | 58 +++++++++++++++++++++++++++-------
src/ghci/parse/show_paths.rs | 7 ++--
src/ghci/parse/show_targets.rs | 28 +++++++++++++---
src/ghci/parse/target_kind.rs | 12 +++++++
src/ghci/stdin.rs | 4 +--
src/normal_path.rs | 12 +++++++
tests/eval.rs | 16 ++++++++--
9 files changed, 125 insertions(+), 27 deletions(-)
create mode 100644 src/ghci/parse/target_kind.rs
diff --git a/src/ghci/mod.rs b/src/ghci/mod.rs
index d5b1be0a..6f61bf66 100644
--- a/src/ghci/mod.rs
+++ b/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?;
diff --git a/src/ghci/parse/mod.rs b/src/ghci/parse/mod.rs
index 178a1516..13c89e57 100644
--- a/src/ghci/parse/mod.rs
+++ b/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;
diff --git a/src/ghci/parse/module_set.rs b/src/ghci/parse/module_set.rs
index b353294b..94c36793 100644
--- a/src/ghci/parse/module_set.rs
+++ b/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()
}
}
diff --git a/src/ghci/parse/show_paths.rs b/src/ghci/parse/show_paths.rs
index 83cb10c3..49a5abd6 100644
--- a/src/ghci/parse/show_paths.rs
+++ b/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));
}
}
}
diff --git a/src/ghci/parse/show_targets.rs b/src/ghci/parse/show_targets.rs
index 75ac8e32..8dcc6f2d 100644
--- a/src/ghci/parse/show_targets.rs
+++ b/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
+ ),
]
);
}
diff --git a/src/ghci/parse/target_kind.rs b/src/ghci/parse/target_kind.rs
new file mode 100644
index 00000000..118edd9e
--- /dev/null
+++ b/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,
+}
diff --git a/src/ghci/stdin.rs b/src/ghci/stdin.rs
index 099d20f2..dad957a2 100644
--- a/src/ghci/stdin.rs
+++ b/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
}
diff --git a/src/normal_path.rs b/src/normal_path.rs
index 2b63c079..9cfad3f4 100644
--- a/src/normal_path.rs
+++ b/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.
diff --git a/tests/eval.rs b/tests/eval.rs
index e88baf28..8f9bfeaa 100644
--- a/tests/eval.rs
+++ b/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();