Fix "modules defined in multiple files" bug when modules are renamed #125
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes a "modules defined in multiple files" bug when a module is renamed. The rename is interpreted as one module being deleted (which triggers a restart) and one module being added (which triggers an `:add`). The bug is that it would restart `ghci`, thereby loading the new module, and then attempt to `:add` the new module anyways, which would fail.
DUX-1404 Module defined in multiple files (again)
Repro:
|
9999years
commented
Oct 4, 2023
| @@ -380,6 +380,9 @@ impl Ghci { | |||
| tracing::info!(%command, "Running after-restart command"); | |||
| self.stdin.run_command(&mut self.stdout, command).await?; | |||
| } | |||
| // Once we restart, everything is freshly loaded. We don't need to add or | |||
| // reload any other modules. | |||
| return Ok(()); | |||
There was a problem hiding this comment.
This is the actual bug fix.
9999years
commented
Oct 4, 2023
Comment on lines
+557
to
+560
| if self.targets.contains_source_path(path.absolute())? { | ||
| tracing::debug!(%path, "Skipping `:add`ing already-loaded path"); | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
This is also sufficient to fix the bug. I'm adding it here in case I make this programming error in the future (calling Ghci::add on a file that's already part of the target set).
Gabriella439
approved these changes
Oct 4, 2023
9999years
deleted the
rebeccat/dux-1404-module-defined-in-multiple-files-again
branch
9999years
added a commit
that referenced
this pull request
Dec 5, 2023
#168 introduced another bug with eval commands. The actual bugfix here is relatively small, but I needed some architectural changes in the test harness (to support restarting the `ghciwatch` session) and in the `ghciwatch` codebase to make these changes. - Removes the system of `Mode`s introduced in #11 and gutted in #78. This grouped `ghci` output into buffers based on the "mode" `ghciwatch` was in when the output was produced (modules being compiled, tests being run, internal data being gathered, etc.). Over time this was used less and less. Now we parse diagnostics out of all compiler output and it's fine. - Also removes the `Ghci::process_ghc_messages` method introduced in #77. The idea was that methods like `Stdout::prompt` would return a list of parsed GHC messages/diagnostics, and those would be processed by `Ghci`. In practice, it was difficult to remember to pass the lists of messages to `process_ghc_messages` consistently. Now, each method that produces diagnostics takes a `&mut CompilationLog`. There's many places where messages are produced, but only a few where messages need to be written out (after reloads, after startup), so this is a lot more convenient. # Bug reproduction details There's two bugs here: `module '...' is not interpreted` and `module '...' defined in multiple files`. My attempted fix for the first bug in #168 caused the second bug. This PR fixes both. First, you load a `ghci` session: ``` $ ghci [1 of 4] Compiling MyLib ( src/MyLib.hs, interpreted ) [2 of 4] Compiling MyModule ( src/MyModule.hs, interpreted ) [3 of 4] Compiling Paths_my_simple_package ( /Users/wiggles/ghciwatch/tests/data/simple/dist-newstyle/build/aarch64-osx/ghc-9.6.3/my-simple-package-0.1.0.0/l/test-dev/build/test-dev/autogen/Paths_my_simple_package.hs, interpreted ) [4 of 4] Compiling TestMain ( test/TestMain.hs, interpreted ) Ok, four modules loaded. ghci> :quit ``` This interprets each module. When the GHC option [`-fwrite-if-simplified-core`](https://downloads.haskell.org/ghc/latest/docs/users_guide/phases.html#ghc-flag--fwrite-if-simplified-core) (introduced in GHC 9.4) is used, loading the `ghci` session additionally writes interface files for each module: > The interface file will contain all the bindings for a module. From this interface file we can restart code generation to produce byte-code. Then, you can load a _new_ `ghci` session, which will load the interface modules and use them to generate `ghci` byte-code directly. Critically, this means that the modules are not considered to be "interpreted", leading to this error message: ``` $ ghci Ok, four modules loaded. ghci> :module + *MyLib module 'MyLib' is not interpreted; try ':add *MyLib' first ``` ## How `ghciwatch` triggers the bug When `ghciwatch`'s `--enable-eval` flag is used, an eval command `runMyCommand` for a module `MyModule` is executed roughly like this: ``` > :module + *MyModule > runMyCommand > :module - *MyModule ``` `:module + *MyModule` brings the top-level definitions of `MyModule` into scope, so that eval commands can refer to definitions in the module they're defined in. @parsonsmatt noticed that if `ghci` loaded a module from one of these interface files, evaluating commands in that file would fail: ``` GHCi, version 9.6.2: https://www.haskell.org/ghc/ :? for help Loaded GHCi configuration from .ghci • ghci started in 5.52s • Adding modules to ghci: • test/Foo.hs Ok, 3140 modules loaded. • test/Foo.hs:25:7: runMyTests module 'Foo' is not interpreted; try ':add *Foo' first ``` I initially attempted to fix this in #168, which added an explicit `:add *Foo` before running the eval command, which forces the module to be interpreted. Unfortunately, using the module name instead of the module path could lead to the module being added twice, causing the dreaded `module defined in multiple files` bug (see #37, #77, #125).
9999years
added a commit
that referenced
this pull request
May 16, 2024
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. ## Prior art - #37 - #77 - #125 - #214
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a "modules defined in multiple files" bug when a module is renamed. The rename is interpreted as one module being deleted (which triggers a restart) and one module being added (which triggers an
:add).The bug is that it would restart
ghci, thereby loading the new module, and then attempt to:addthe new module anyways, which would fail.