On Wed, Apr 13, 2011 at 11:44:33AM +0100, Simon Marlow wrote:

On 09/04/2011 04:32, Evan Laforge wrote:

...

I've found ghc's cyclic import error to be rather confusing, and I finally took the time to understand why. Here's an example:

Module imports form a cycle for modules: Cmd.Cmd (./Cmd/Cmd.hs) imports: Perform.Midi.Instrument Instrument.MidiDb Instrument.Db Perform.Midi.Instrument (./Perform/Midi/Instrument.hs) imports: Cmd.Cmd Instrument.MidiDb (./Instrument/MidiDb.hs) imports: Perform.Midi.Instrument Instrument.Db (./Instrument/Db.hs) imports: Instrument.Search Instrument.MidiDb Perform.Midi.Instrument Instrument.Search (./Instrument/Search.hs) imports: Instrument.MidiDb Perform.Midi.Instrument

It seems to be in a strange order and mentions extraneous modules. I would find this much easier to read:

Perform.Midi.Instrument -> Cmd.Cmd -> Instrument.MidiDb -> Perform.Midi.Instrument

So the algorithm that GHC uses is this:

- do a strongly connected component analysis - build until we hit a cycle - then, report the error for the cycle

Now, the modules in the cycle are a strongly connected component: every module is reachable from every other module by following imports. We report all the modules in the strongly connected component and their imports, but omit imports of modules outside the cycle.

...

Instead, the order goes Cmd.Cmd -> Instrument.MidiDb, and then goes backwards to Perform.Midi.Instrument -> Cmd.Cmd. Then it goes forward again to Instrument.MidiDb -> Perform.Midi.Instrument. So the order makes you jump around if you want to trace the import chain. The duplicated module that joins the cycle is not visually highlighted. Whats more, it further confuses the eye by merging in multiple loops. I suppose it could be useful to include additional loops, but I would find it easier to read if they were included on their own line, such as:

Cmd.Cmd -> Instrument.Db -> Instrument.Search -> Perform.Midi.Instrument -> Cmd.Cmd

However, I think probably the shortest loop is the most interesting one, and if there are multiple shortest loops, simply picking one randomly is likely to be sufficient.

Picking the shortest one sounds reasonable. However, there could be a single edge which if removed will break all the loops, and they might want to know which it is.

If you want to play with this, the code is very localised: it is a few lines in GhcMake.cyclicModuleError

http://hackage.haskell.org/trac/ghc/browser/compiler/main/GhcMake.hs#L1446

Hi Simon and Evan, Thanks for bringing up a problem, and providing useful information about it, in a way that is understandable enough for a newcomer to have an opinion about it! So, here is my newcomer's opinion: The error displays a strongly connected graph, with one or more cycles, but labels it a "cycle". As you both point out, having all the information about the strongly connected modules is very useful. Labeling it a cycle, however, gives the reader an expectation that is bound to be confounded. Perhaps the following change would be sufficient? --- tmp/GhcMake.hs~ 2011-04-14 09:46:02.177298318 -0700 +++ tmp/GhcMake.hs 2011-04-14 09:52:25.121290827 -0700 @@ -1460,7 +1460,8 @@ cyclicModuleErr :: [ModSummary] -> SDoc cyclicModuleErr ms - = hang (ptext (sLit "Module imports form a cycle for modules:")) + = hang (ptext (sLit "Module imports form a strongly connected graph, with one or more cycles, for these modules:")) 2 (vcat (map show_one ms)) where mods_in_cycle = map ms_mod_name ms -Bryan P.S. I'm not sure what the accepted method for formatting/wrapping string literals is, so I left it as an exercise. :)

Following Bryan's suggestion I've improved GHC's error message when there's a module cycle: Module imports form a cycle: module `Foo4' imports `Foo' which imports `Foo2' which imports `Foo3' which imports `Foo4' Simon | -----Original Message----- | From: glasgow-haskell-users-bounces@haskell.org [mailto:glasgow-haskell-users- | bounces@haskell.org] On Behalf Of Bryan Richter | Sent: 14 April 2011 18:24 | To: glasgow-haskell-users@haskell.org | Subject: Re: ghc cyclic import error confusing | | On Wed, Apr 13, 2011 at 11:44:33AM +0100, Simon Marlow wrote: | > On 09/04/2011 04:32, Evan Laforge wrote: | > >I've found ghc's cyclic import error to be rather confusing, and I | > >finally took the time to understand why. Here's an example: | > > | > >Module imports form a cycle for modules: | > > Cmd.Cmd (./Cmd/Cmd.hs) | > > imports: Perform.Midi.Instrument Instrument.MidiDb Instrument.Db | > > Perform.Midi.Instrument (./Perform/Midi/Instrument.hs) | > > imports: Cmd.Cmd | > > Instrument.MidiDb (./Instrument/MidiDb.hs) | > > imports: Perform.Midi.Instrument | > > Instrument.Db (./Instrument/Db.hs) | > > imports: Instrument.Search Instrument.MidiDb | > > Perform.Midi.Instrument | > > Instrument.Search (./Instrument/Search.hs) | > > imports: Instrument.MidiDb Perform.Midi.Instrument | > > | > >It seems to be in a strange order and mentions extraneous modules. I | > >would find this much easier to read: | > > | > >Perform.Midi.Instrument -> Cmd.Cmd -> Instrument.MidiDb -> | > >Perform.Midi.Instrument | > | > So the algorithm that GHC uses is this: | > | > - do a strongly connected component analysis | > - build until we hit a cycle | > - then, report the error for the cycle | > | > Now, the modules in the cycle are a strongly connected component: | > every module is reachable from every other module by following | > imports. We report all the modules in the strongly connected | > component and their imports, but omit imports of modules outside the | > cycle. | > | > >Instead, the order goes Cmd.Cmd -> Instrument.MidiDb, and then goes | > >backwards to Perform.Midi.Instrument -> Cmd.Cmd. Then it goes | > >forward again to Instrument.MidiDb -> Perform.Midi.Instrument. So | > >the order makes you jump around if you want to trace the import | > >chain. The duplicated module that joins the cycle is not visually highlighted. | > >Whats more, it further confuses the eye by merging in multiple loops. | > >I suppose it could be useful to include additional loops, but I would | > >find it easier to read if they were included on their own line, such | > >as: | > > | > >Cmd.Cmd -> Instrument.Db -> Instrument.Search -> | > >Perform.Midi.Instrument -> Cmd.Cmd | > > | > >However, I think probably the shortest loop is the most interesting | > >one, and if there are multiple shortest loops, simply picking one | > >randomly is likely to be sufficient. | > | > Picking the shortest one sounds reasonable. However, there could be a | > single edge which if removed will break all the loops, and they might | > want to know which it is. | > | > If you want to play with this, the code is very localised: it is a few | > lines in GhcMake.cyclicModuleError | > | > http://hackage.haskell.org/trac/ghc/browser/compiler/main/GhcMake.hs#L | > 1446 | | Hi Simon and Evan, | | Thanks for bringing up a problem, and providing useful information about it, in a way | that is understandable enough for a newcomer to have an opinion about it! | | So, here is my newcomer's opinion: The error displays a strongly connected graph, | with one or more cycles, but labels it a "cycle". As you both point out, having all | the information about the strongly connected modules is very useful. Labeling it a | cycle, however, gives the reader an expectation that is bound to be confounded. | | Perhaps the following change would be sufficient? | | | | --- tmp/GhcMake.hs~ 2011-04-14 09:46:02.177298318 -0700 | +++ tmp/GhcMake.hs 2011-04-14 09:52:25.121290827 -0700 | @@ -1460,7 +1460,8 @@ | | cyclicModuleErr :: [ModSummary] -> SDoc cyclicModuleErr ms | - = hang (ptext (sLit "Module imports form a cycle for modules:")) | + = hang (ptext (sLit "Module imports form a strongly connected graph, | + with one or more cycles, for these modules:")) | 2 (vcat (map show_one ms)) | where | mods_in_cycle = map ms_mod_name ms | | | | | | -Bryan | | P.S. I'm not sure what the accepted method for formatting/wrapping string literals | is, so I left it as an exercise. :)