Can we improve import errors?

[joneshf]

I'm doing some file renames for dhall-bhat, and the error message for missing imports can get a little confusing. For example:

↳ ./NonEmptyList/applicative
  ↳ ./NonEmptyList/functor
    ↳ ./NonEmptyList/traversable
      ↳ ./List/traversable
        ↳ ./List/Foldable

Error: Missing file /home/joneshf/programming/FormationAI/dhall-bhat/List/Foldable

The error message does give all of the information necessary to figure out the problem, but there's some hard things about it:

• The problem is in ./List/traversable, but the last line–that says what the error is–doesn't mention ./List/traversable. You have to parse the import chain to find the second to last import to know where the error occurred.
• There is no source location. This one is especially hard if there are multiple imports of the same file as you have to replace them all, but you might miss one and get really confused when you get the same error again. You might question if you did the correct thing, or if the Dhall program is reporting properly, or whatever else.
• There is no explanation available. One of the great things about Dhall is the --explain flag. For the most part, when I get stuck on something, I can add the --explain flag, and get unstuck. The examples and explanations are top-notch.

Is there some way we can improve these issues? Do we have enough information to add locations and whatnot to the error? I'm happy to submit a PR to help out.

[Gabriella439]

@joneshf: Yeah, this should be doable.

The only reason that the MissingFile exception doesn't have source information is because it is thrown from the exprFromUncachedImport function here:

dhall-haskell/src/Dhall/Import.hs

Line 596 in b5c005e

else throwMissingImport (MissingFile path)

... and that function is only provided an Import argument without any source context. However, that context could be added by first defining a function of type:

annotateEmbed :: Expr s a -> Expr s (Maybe s, a)

... which in the common case would specialize to:

annotateEmbed :: Expr Src Import -> Expr Src (Maybe Src, Import)

... and then changing exprFromImport (and everything downstream of it) to have this type:

exprFromImport :: Maybe Src -> Import -> StateT (Status IO) IO (Expr Src Import)

... and then we'd have the source information we needed to annotate where the missing file import came from.

There's also no reason why we can't add support for the --explain flag, too. The way you do that is to change the detailed function here:

dhall-haskell/src/Dhall.hs

Line 542 in b5c005e

detailed :: IO a -> IO a

... to catch MissingFile exceptions and to wrap them in a more detailed exception type. The only reason I didn't do this already is because I wasn't sure what additional information would be worth providing for missing files.

I can also do these changes for you, but you're welcome to take a stab at it before I get to it if you want.

[joneshf]

I haven't forgotten about this issue 🙂 (though I did forget to respond).

Thanks for the explanation! I think I'll probably hop on it the next time I get hit by it.

[ocharles]

I believe this duplicates #226. This has a bit more advice for progress, so perhaps #226 should be closed.

[Profpatsch]

Ping. I’m trying to write a flycheck plugin for emacs, so I’d like to show the user all files that can’t be imported.

One intermediate solution would be to try import, catch import errors and then iterate over the AST to try and resolve each import separately.

[Gabriella439]

First change to add source locations to missing imports is up here: #812

[Gabriella439]

@Profpatsch: I don't plan to get to import error messages that list all failed imports, for two main reasons:

• I have higher priority improvements I'd like to address (like performance)
• The interpreter also doesn't support more than one error for type-checking and I'm also not ready to implement that

If somebody adds support for that I'll accept the patch, though

[Gabriella439]

@joneshf: So I've gotten about as far as I think I will get on this at the moment (once #815 is merged)

Additional improvements will require me to overhaul Dhall's exception system because it's a bit of a mess at this point, which is the root cause of some of the issues you described, such as:

• Not being able to easily use --explain to elaborate on "Missing file" errors
• Not being able to easily trim the last import if the exception is an import resolution failure

[Profpatsch]

I have higher priority improvements I'd like to address (like performance)

Very valid.

Additional improvements will require me to overhaul Dhall's exception system

The multiple error messages changes is gonna block on that, too. For dhall-flycheck I could go ahead with my previous idea:

One intermediate solution would be to try import, catch import errors and then iterate over the AST to try and resolve each import separately.

Though to be honest the time spent with import errors is minimal in most cases, so it’s a heavy investment for a pretty small gain. If the exceptions were to be replaced by return values the change should be trivial though.

[joneshf]

Sounds good. Should we close this issue in exchange for whatever exception overhaul will come out or leave it open to track that this pain point still exists?

[Gabriella439]

@joneshf: It's fine to leave this issue open for now

[Profpatsch]

Updating dhall-flycheck to latest dhall, and I was bitten by errors being exceptions again.

It’s just not possible to correctly wrap dhall at the moment and being sure that errors are handled exhaustively. Or even get much information out of them (e.g. for inline errors that should appear in the file which threw an import error).

I would very much appreciate if the switch away from exceptions towards structured errors (via Either or Validation) could be done.

[Profpatsch]

Also errors like PrettyHttpException don’t carry a source code position even though they totally could.

[sjakobi]

@Profpatsch PRs to improve the situation would be very welcome! :)

[Profpatsch]

I’m afraid I don’t have time to deep-dive at the moment (it’s a rather fundamental change of the main functions), I just wanted to bump the topic to mark interest.

[Profpatsch]

I notice that the pretty-printed error message contains an import chain, though I’m not sure where it gets it from. It’s not in the errors that are thrown by Import.loadWith. as far as I can see.

~/vuizvui/result/bin/dhall-flycheck ./log2.dhall
dhall-flycheck: 
↳ ./Reference.dhall
  ↳ ./ri.dhall

Error: Missing file ./ri.dhall

1│           ./ri.dhall

/home/philip/dotfiles-private/log/Reference.dhall:1:11

[Profpatsch]

For reference, this is how I catch errors at the moment:
https://github.com/Profpatsch/dhall-flycheck/blob/3d11abbebf80eef2df98ce90d92f9453a0309637/src/Dhall/Flycheck.hs#L256-L294

Note that this list has to be kept in sync with the actual (open) error list on every update (no type errors), otherwise the checker randomly crashes. And I really don’t want to catch all exceptions, because that’s a big no-go in the presence of signals and async errors (see the relevant section in Control.Exception)

[joneshf]

Could a first step be to have a single exception type?

data ImportException
  = ImportResolutionDisabled Dhall.Import.ImportResolutionDisabled
  | PrettyHttpException Dhall.Import.PrettyHttpException
  ...

You ought to be able to exhaustively check the exceptions, and it ought to be less invasive than both changing the types and also changing the error handling mechanism. Would that work?

[Profpatsch]

Would that work?

It would solve the problem of not being forced to be exhaustive, yes, but not the problem of having no good error positions for most of these errors. Maybe I’m missing a way to read them from some evaluation context.