Fix "module defined in multiple files" by parsing GHC diagnostics #77
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
DUX-1290 Startup with failed modules
If modules fail to load when The correct approach is to simply Upstream bug report: https://gitlab.haskell.org/ghc/ghc/-/issues/13254 |
This was referenced Sep 11, 2023
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
Merged
9999years
restored the
rebeccat/dux-1290-startup-with-failed-modules
branch
9999years
force-pushed
the
rebeccat/split-reload-up
branch
from
c93dd22 to
0185729
Compare
9999years
force-pushed
the
rebeccat/dux-1290-startup-with-failed-modules
branch
from
4a11fe3 to
41cd86d
Compare
9999years
force-pushed
the
rebeccat/split-reload-up
branch
from
0185729 to
1e00821
Compare
9999years
force-pushed
the
rebeccat/dux-1290-startup-with-failed-modules
branch
from
41cd86d to
6f3348e
Compare
9999years
changed the title
GhciStderr::get_buffer
9999years
force-pushed
the
rebeccat/dux-1290-startup-with-failed-modules
branch
3 times, most recently
from
5ef7d41 to
7993fe5
Compare
9999years
force-pushed
the
rebeccat/split-reload-up
branch
from
1e00821 to
c0d5879
Compare
9999years
force-pushed
the
rebeccat/dux-1290-startup-with-failed-modules
branch
from
8577dd0 to
b053ce4
Compare
9999years
commented
Sep 11, 2023
Comment on lines
+285
to
+286
| if self.modules.contains_source_path(&path)? | ||
| || self.failed_modules.contains_source_path(&path)? |
There was a problem hiding this comment.
Here's the actual bug fix. Don't :add modules that failed to compile. The rest of the PR is about populating the failed_modules set correctly.
9999years
force-pushed
the
rebeccat/split-reload-up
branch
from
c0d5879 to
ae41dd3
Compare
9999years
force-pushed
the
rebeccat/dux-1290-startup-with-failed-modules
branch
from
b053ce4 to
91b6df9
Compare
9999years
force-pushed
the
rebeccat/split-reload-up
branch
from
ae41dd3 to
2366a42
Compare
9999years
force-pushed
the
rebeccat/dux-1290-startup-with-failed-modules
branch
from
91b6df9 to
8e4af91
Compare
9999years
force-pushed
the
rebeccat/split-reload-up
branch
from
2366a42 to
ecdc7ef
Compare
9999years
force-pushed
the
rebeccat/dux-1290-startup-with-failed-modules
branch
from
8e4af91 to
d0175be
Compare
Eventually we'll probably want this to be configurable with the `GhcidNgBuilder`.
This enables parsing diagnostics that are printed on stderr.
This moves some spammier logs to `trace` level, removes a few redundant spans, and provides some general cleanup.
This should help keep at least the "starting with failed modules" source of the "module defined in multiple files" bug gone.
9999years
force-pushed
the
rebeccat/dux-1290-startup-with-failed-modules
branch
from
d0175be to
5fc5cd9
Compare
Gabriella439
approved these changes
Sep 12, 2023
This was referenced Sep 19, 2023
Merged
Merged
Merged
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 parses GHC diagnostics and uses the output to avoid
:adding failed modules. See the bug upstream here: https://gitlab.haskell.org/ghc/ghc/-/issues/13254