User-friendly error message when modules aren't listed

[seanhess]

Addresses #3695. When a user forgets to add their module to other-modules or exposed-modules, they would get a confusing "Multi Cradle: no prefixes matched" error.

This PR replaces the old error message with the following:

  Loading the module 'app/Test.hs' failed. It seems that it is not listed in your .cabal file!
  Perhaps you need to add `Test` to other-modules or exposed-modules
  For more information, visit:
  https://cabal.readthedocs.io/en/3.4/developing-packages.html#modules-included-in-the-package

This is my first PR for HLS, please let me know which conventions I've missed.

[fendor]

This looks like the perfect first step!

It would be great if we can include the cabal file in question in the error message. For cabal cradles, cradleErrorDependencies should contain the .cabal filepath if we were able to find it.

Further, I, personally, think we should show this error message only for cabal cradles (and perhaps stack cradles), so the error message still needs to be guarded. Look at the call site of renderCradleError, can we thread the CradleType or Cradle through such that we can call isCabalCradle?

As a word of warning, our tests are unfortunately rather flaky. Sometimes a test just fails. In that case, don't panic, we rerun the testsuite.

[seanhess]

The dependencies were empty in my test case, I'm afraid. This is all the information in the CradleError:

Should we show them the component(s) from the prefixes instead?

CradleError
  { cradleErrorDependencies = []
  , cradleErrorExitCode = ExitSuccess
  , cradleErrorStderr =
    [ "Multi Cradle: No prefixes matched"
    , "pwd: /Users/sean/Documents/code/hls-file-error"
    , "filepath: /Users/sean/Documents/code/hls-file-error/app/Sweet/Woot.hs"
    , "prefixes: ("app/Main.hs", Cabal {component = Just "hls-file-error:exe:hls-file-error"})"
    ]
  }

[seanhess]

It looks like parsing any information from the prefixes is going to be too complicated to be worth it. Here are all the possibilities from hie-bios:

instance Show (CradleType a) where
    show (Cabal comp) = "Cabal {component = " ++ show (cabalComponent comp) ++ "}"
    show (CabalMulti d a) = "CabalMulti {defaultCabal = " ++ show d ++ ", subCabalComponents = " ++ show a ++ "}"
    show (Stack comp) = "Stack {component = " ++ show (stackComponent comp) ++ ", stackYaml = " ++ show (stackYaml comp) ++ "}"
    show (StackMulti d a) = "StackMulti {defaultStack = " ++ show d ++ ", subStackComponents = "  ++ show a ++ "}"
    show Bios { call, depsCall } = "Bios {call = " ++ show call ++ ", depsCall = " ++ show depsCall ++ "}"
    show (Direct args) = "Direct {arguments = " ++ show args ++ "}"
    show None = "None"
    show (Multi a) = "Multi " ++ show a
    show (Other _ val) = "Other {originalYamlValue = " ++ show val ++ "}"

We can show them the output of each prefix directly, something like:

  Loading the module 'app/Test.hs' failed. It seems that it is not listed in your .cabal file!
  Perhaps you need to add `Test` to other-modules or exposed-modules
  For more information, visit:
  https://cabal.readthedocs.io/en/3.4/developing-packages.html#modules-included-in-the-package

  app/Main.hs - Cabal {component = Just "hls-file-error:exe:hls-file-error"})
  ... any other prefixes here ...

Which, while it isn't the most user-friendly, at least surfaces all the relevant information.

[fendor]

Oof, that is unintended behaviour that we have never realised before! We have to fix that in hie-bios!

You can do that if you'd like!

The code that eventually produces the error message is this: https://github.com/haskell/hie-bios/blob/master/src/HIE/Bios/Cradle.hs#L107

It is a bit tricky to follow, but essentially, we take a CabalMulti and wrap it in a MultiCradle again. However, we know at Cradle.hs#L107 already that it will be a cabal cradle.

After L107, override the runCradle action of the produced Cradle and look at how the cabalAction finds the cradle dependencies: https://github.com/haskell/hie-bios/blob/master/src/HIE/Bios/Cradle.hs#L702C26-L702C26. I think, we will be able to get a better error message that way.

[seanhess]

I'd be happy to take a look at it, if you can help me figure out how to test hie-bios locally. I got it compiling. Is there a way to run hie-bios on my error test project and get it to generate this error? My naive attempts didn't work.

Even better, I would like to get haskell-language-server to compile against a local hie-bios, but when I tried this on the latest master for both, they didn't compile. Any guidance? I can post the error here if that would help

[fendor]

Is there a way to run hie-bios on my error test project and get it to generate this error? My naive attempts didn't work.

Install the hie-bios binary and run hie-bios debug ./app/A.hs. This should show some logs and produce the final error message.

Even better, I would like to get haskell-language-server to compile against a local hie-bios, but when I tried this on the latest master for both, they didn't compile. Any guidance? I can post the error here if that would help

It is possible, that we changed something, and now it doesn't compile any more... I haven't looked into it yet, I am afraid.

Perhaps you want to git checkout the latest release of hie-bios and work off that? There haven't been many changes, so if you get something working, it is trivial to get it merged upstream.

[seanhess]

I pushed changes to limit to isCabalCradle, and to show the prefixes. I'll look in to messing with hie-bios now

[seanhess]

I think I'm in over my head. Here's the issue I'm running in to:

src/Hie/Implicit/Cradle.hs:67:39: error: [GHC-83865]
    • Couldn't match type ‘Maybe FilePath -> CabalType’
                     with ‘CabalType’
      Expected: Name -> Component -> (FilePath, CabalType)
        Actual: Name
                -> Component -> (FilePath, Maybe FilePath -> CabalType)
    • In the second argument of ‘build’, namely ‘cabalComponent'’
      In the expression:
        build (CabalMulti mempty) cabalComponent' cabalPkgs
      In an equation for ‘cabal’:
          cabal = build (CabalMulti mempty) cabalComponent' cabalPkgs
   |
67 |     cabal = build (CabalMulti mempty) cabalComponent' cabalPkgs
   |                                       ^^^^^^^^^^^^^^^

I found this related comment in flake.nix. I have VERY limited exposure to nix, but I think I'm getting the error for the same reason mentioned in the comment:

    # smunix: github:haskell/hie-bios defines
    #   'CabalType :: Maybe String -> Maybe FilePath -> CabalType'
    # while the original githcom:Avi-D-coder/hie-bios still has this:
    #   'CabalType :: Maybe String -> CabalType'
    # We need a patched version of implicit-hie-cradle that works with hls, so I've created
    # the repository below. Obviously, this is not sustainable as it adds more technical debt.
    # We need a better strategy to streamline changes required by HLS from other hie-bios related
    # packages.
    # See details here: https://github.com/Avi-D-coder/implicit-hie-cradle/compare/master...smunix:implicit-hie-cradle:smunix-patch-hls-1?expand=1

[fendor]

Yeah, that was something we changed. So, essentially, clone implicit-hie pass in Nothing for this new parameter and point the cabal.project in hls to your patched implicit-hie version. You can refer to relative paths in cabal.project, so for example: packages: ../hie-bios ../implicit-hie ..., where your forks for hie-bios and implicit-hie are in the same directory as hls.

However, checking out an older hie-bios (the latest released tag) is likely quicker.

[seanhess]

@fendor I think I'm not going to have time to address the hie-bios changes ATM. Would you consider this PR without them? What else should change? Final error output is:

  Loading the module 'app/Test.hs' failed. It seems that it is not listed in your .cabal file!
  Perhaps you need to add `Test` to other-modules or exposed-modules
  For more information, visit:
  https://cabal.readthedocs.io/en/3.4/developing-packages.html#modules-included-in-the-package

  app/Main.hs - Cabal {component = Just "hls-file-error:exe:hls-file-error"})
  ...

[fendor]

@seanhess Yes, we will take the PR as is. Our HSoC student @VeryMilkyJoe can take it over to push it over the finish line, maybe try to fix the hie-bios issue in the process as well.

Regarding the error message, I would ask you to write down what kind of information you would have needed to understand/be able to resolve the issue you encountered in #3693? Is the given information enough?

[seanhess]

Great! Yes for me personally, the PR is at the point that satisfies everything I needed from it and would have prevented me from getting lost and needing to ask for help. Also I’m happy to “finish” the PR if we are sticking to the haskell-language-server repository. I just don’t have time to dig in to hie-bios.

…

On Thu, Jul 13, 2023 at 1:55 AM fendor ***@***.***> wrote: @seanhess <https://github.com/seanhess> Yes, we will take the PR as is. Our HSoC student @VeryMilkyJoe <https://github.com/VeryMilkyJoe> can take it over to push it over the finish line, maybe try to fix the hie-bios issue in the process as well. Regarding the error message, I would ask you to write down what kind of information you would have needed to understand/be able to resolve the issue you encountered in #3693 <#3693>? Is the given information enough? — Reply to this email directly, view it on GitHub <#3711 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAD66LDILQNMYKV7656FW3XP6SYTANCNFSM6AAAAAA2GR5INE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[seanhess]

Hey @fendor, anything short of cracking open hie-bios I can do to help get this merged?

[fendor]

Hey fendor, anything short of cracking open hie-bios I can do to help get this merged?

I have it on my to-do list for the next weeks to improve the hie-bios error messages, and also get this merged.
@VeryMilkyJoe will also need it for their work for the hls-cabal-plugin.

From my perspective, you don't need to do anything, it is a matter of plugging everything together, which requires some work and coordination.

[fendor]

LGTM! First step to help new users. Many more improvements are yet to follow.