From 49a76e6365e6625e7751a58940fa8c54ab51f905 Mon Sep 17 00:00:00 2001 From: Rebecca Turner Date: Wed, 15 May 2024 16:25:36 -0700 Subject: [PATCH 1/2] Add regression test --- tests/eval.rs | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/tests/eval.rs b/tests/eval.rs index 8f9bfeaa..081dcca0 100644 --- a/tests/eval.rs +++ b/tests/eval.rs @@ -195,3 +195,58 @@ async fn can_eval_commands_in_non_interpreted_modules() { .await .expect("ghciwatch evals commands"); } + +/// Test that `ghciwatch` can eval commands in a module loaded at startup, and then test that it +/// can eval commands in that module a second time. +/// +/// See: +#[test] +async fn can_eval_commands_twice() { + let module_path = "src/MyModule.hs"; + let cmd = "-- $> example ++ example"; + let mut session = GhciWatchBuilder::new("tests/data/simple") + .with_arg("--enable-eval") + .before_start(move |path| async move { + Fs::new() + .append(path.join(module_path), format!("\n{cmd}\n")) + .await + }) + .start() + .await + .expect("ghciwatch starts"); + let module_path = session.path(module_path); + + // Adds the module succesfully. + let ok_reload = BaseMatcher::message("All good!") + // Evals the command. + .and(BaseMatcher::message( + r"MyModule.hs:\d+:\d+: example \+\+ example", + )) + // Reads eval output. + .and(BaseMatcher::message("Read line").with_field("line", "exampleexample")) + // Finishes the reload. + .and(BaseMatcher::reload_completes()) + .but_not( + BaseMatcher::message("Read stderr line") + .with_field("line", "defined in multiple files"), + ); + + session + .wait_until_ready() + .await + .expect("ghciwatch didn't start in time"); + + session.checkpoint(); + session.fs().touch(&module_path).await.unwrap(); + session + .assert_logged_or_wait(ok_reload.clone()) + .await + .expect("ghciwatch evals commands"); + + session.checkpoint(); + session.fs().touch(&module_path).await.unwrap(); + session + .assert_logged_or_wait(ok_reload) + .await + .expect("ghciwatch evals commands"); +} From 96a51a5a854ceb6bcff62f0d1da0e4a91dfc3cec Mon Sep 17 00:00:00 2001 From: Rebecca Turner Date: Wed, 15 May 2024 16:04:08 -0700 Subject: [PATCH 2/2] Fix "module defined in multiple files" AGAIN!!! In #214, we had a situation where modules were loaded: ghci> :show targets Foo Bar Baz And then an eval comment in (e.g.) `Foo` causes the module to be added and explicitly interpreted by path: ghci> :add *src/Foo.hs Then, we have `Foo` loaded by name (`Foo`) and by path (`src/Foo.hs`), which triggers the dreaded bug. At the time I proposed this fix, correctly: > 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. I threaded some extra information to the `:show targets` parser to track if modules were listed as names or paths, **but then at the end of `Ghci::interpret_module` I would always insert the module into the target set as a `TargetKind::Path`,** meaning that the *next* time the comment was evaluated, the module would be loaded as a path, causing the error. https://github.com/MercuryTechnologies/ghciwatch/blob/dbba61bbdec9a86f97051b12647e51b7be4fd484/src/ghci/mod.rs#L698-L699 This fixes the bug and adds an `assert!` to fail faster and more obviously if it happens again. --- src/ghci/mod.rs | 10 ++++---- src/ghci/parse/module_set.rs | 46 +++++++++++++++++++++++++++++------- 2 files changed, 42 insertions(+), 14 deletions(-) diff --git a/src/ghci/mod.rs b/src/ghci/mod.rs index 8cb9b637..188c2835 100644 --- a/src/ghci/mod.rs +++ b/src/ghci/mod.rs @@ -688,15 +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)?; + let module = self.targets.module_import_name(&self.search_paths, path)?; self.stdin - .interpret_module(&mut self.stdout, &import_name, log) + .interpret_module(&mut self.stdout, &module.name, log) .await?; - self.targets - .insert_source_path(path.clone(), TargetKind::Path); + if !module.loaded { + self.targets.insert_source_path(path.clone(), module.kind); + } self.refresh_eval_commands_for_paths(std::iter::once(path)) .await?; diff --git a/src/ghci/parse/module_set.rs b/src/ghci/parse/module_set.rs index 94c36793..bd32169b 100644 --- a/src/ghci/parse/module_set.rs +++ b/src/ghci/parse/module_set.rs @@ -52,7 +52,13 @@ impl ModuleSet { /// /// Returns whether the value was newly inserted. pub fn insert_source_path(&mut self, path: NormalPath, kind: TargetKind) -> bool { - self.modules.insert(path, kind).is_some() + match self.modules.insert(path, kind) { + Some(old_kind) => { + assert!(kind == old_kind, "`ghciwatch` failed to track how modules were imported in `ghci`; please report this as a bug"); + true + } + None => false, + } } /// Get the name used to refer to the given module path when importing it. @@ -68,18 +74,27 @@ impl ModuleSet { &self, show_paths: &ShowPaths, path: &NormalPath, - ) -> miette::Result<(String, TargetKind)> { + ) -> miette::Result { 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)) - } + Some(&kind) => match kind { + TargetKind::Path => Ok(ImportInfo { + name: path.relative().to_string(), + kind, + loaded: true, + }), + TargetKind::Module => Ok(ImportInfo { + name: show_paths.path_to_module(path)?, + kind, + loaded: true, + }), }, None => { let path = show_paths.make_relative(path)?; - Ok((path.into_relative().into_string(), TargetKind::Path)) + Ok(ImportInfo { + name: path.into_relative().into_string(), + kind: TargetKind::Path, + loaded: false, + }) } } } @@ -89,3 +104,16 @@ impl ModuleSet { self.modules.keys() } } + +/// Information about a module to be imported into a `ghci` session. +pub struct ImportInfo { + /// The name to refer to the module by. + /// + /// This may either be a dotted module name like `My.Cool.Module` or a path like + /// `src/My/Cool/Module.hs`. + pub name: String, + /// Whether the `name` is a name or path. + pub kind: TargetKind, + /// Whether the module is already loaded in the `ghci` session. + pub loaded: bool, +}