Better error messages

[bergus]

Closes #461, closes #507, closes #504 (via 9ec43a6)

• Better error messages for context fields
• Better error messages for template applications
• Better error messages for template parsing (there was a bug, many just dropped out in the middle of a template when it encountered invalid syntax)

[bergus]

I suppose using error here was not on purpose?

[bergus]

Hm, with every Context field now creating a message when it does not apply that does create an awful lot of debugging messages from the Compiler Alternative. Is <|> typically used directly by Users? If no, its only usage is in the Context monoid, and maybe we should drop the debug messages from there. Or at least filter out the ones that begin with "Tried field".

[jaspervdj]

This is great work, but I'm not sure about the change to Alternative.

compilerCatch y $ compilerThrow . (es++)

If you know run in verbose mode (-v), this will print a quadratic amount of messages, and then the error at the end will repeat them all once more. If we want to go this way, we probably want to remove the call to the logger there.

[bergus]

Yes, that's why I initially thought to drop the <|> call from Context's <> and implement it directly. Do you know whether any Users are calling <|> directly from the applications?

What I also considered is to change the CompilerError constructor to [(Verbosity, String)] -> CompilerResult a. This could allow us to create results that take the second alternative path, but only contain debug messages not exceptions. Not sure how feasible that would be, but we could

• print only the actual error when there's a mistake that caused an exception
• print the debug messages when there is no real error, which would show all the attempted fields (ending with "missing")
• fail with an error when an $if(…)$ field is "empty" because of an exception

The last one would be a breaking change, but possibly a good one; if not, we still could log these errors.
What do you think?

[bergus]

If you do not want to make a breaking change here,
to match the old behaviour, this should need to be

        handler Error es = do 
            logger <- compilerLogger <$> compilerAsk
            forM_ es $ \e -> compilerUnsafeIO $ Logger.debug logger $
                "Hakyll.Web.Template.applyTemplate: Error in 'if': " ++ e
            return ""

But I really like distinguishing exceptions (fail) from plain empty fields.

[bergus]

I've had a quick google search for field and fail usage scenarios, and found none that were offending. Or actually, this one.

[bergus]

While working on this, I found another bug: When the body of an $if(…)$ statement contained a field that threw an error, the else body ran. Ooops.
Now it correctly raised such errors.

[bergus]

Please see the update. I've changed CompilerError to include a verbosity (not multiple messages of different verbosities, as I considered initially), and the Alternative does drop the less important messages if both compilers fail.
I have removed the logging in verbose mode completely, as fields now either fail with a loud error or not at all. Notice that the -v output even got shorter than before, as I removed the missingField from the defaultContext - the applyTemplate' does add a missingField to every context anyway - and previously that duplication would have triggered the debug log with the first message in the Alternative.

[bergus]

Btw, when do you plan the next release? I'm close to shipping and need to decide whether to wait for an official version bump or just put my branches in as a beta.

[bergus]

I suppose that when dropping a list of messagse because there are more important ones one could log them to the debug screen. Not sure whether necessary.

[bergus]

Another question I'd like to get some feedback on is the order/formatting of the error messages. I like to use things like

-- helper to amend error messages
withErrorContext :: MonadError [String] m => String -> m a -> m a
withErrorContext msg = flip catchError (throwError . (msg:))

which basically prepends a string to the list of error messages if an error happens. This does give us some kind of call stack.
When logging the list of messages, should it be reversed so that the innermost error is at the top and the rest of lines gives the context? Or should the innermost error be printed as the last line?

[jaspervdj]

Hey, this PR is really starting to look god, I appreciate all the effort! Unfortunately I'm traveling to the states this weekend so I won't be able to look into it in detail until monday. I hope to merge it it then.

[bergus]

Thanks, I don't want to put any pressure on you :-)
I'm trying to use serverside builds now, and those don't work with a custom hakyll installation. Can you give me a very rough estimate when we can expect hakyll-4.9 (or 5.0?) on hackage?
I have to choose whether I'll need to use this approach to install hakyll from my branch, or just revert my files to be 4.8.3 compatible.

[jaspervdj]

I would prefer to keep the $if(..)$ behavior backwards-compatible. It is fairly common in Haskell libraries that empty is equivalent to fail msg apart from the produced message. Other than that, everything looks good to merge.

We can probably do a release later this week. If you're having trouble using custom versions of libraries, you can always consider using stack. I need to use patched libraries all the time at work and I've never had any trouble with it.

[bergus]

Yeah, I'm right in the process of switching from a plain ghc invocation to stack :-)

I'm going to change the $if$ behaviour back and instead log errors to the debug view.
But regarding b/c, notice that 8ac7949 also breaks the Template constructor (which is no longer exported from Template.Internal) and replaces it by the template "smart constructor".
Not that it matters, I'm probably the only person ever using it. I only could find one usage of the module outside your source code via google: in this asian pdf - citing the source of hakyll-3.

[bergus]

Hi, have you had a chance to look at this?

[jaspervdj]

Sorry for the delayed response on my part.

I'm still very interested in getting a form of this merged in, but I'm not sure when I'll have time to look at it. :-(

It currently seems that we could solve this by putting something like [(Verbosity, String)] in CompilerWrite, and modifying the CompilerError constructor to :: CompilerWrite -> CompilerResult a.

[bergus]

OK, I'll try to get this working again after resolving the conflicts, and then see what I can do.
It's been a long time, which is a good way to evaluate whether I wrote maintainable code that I can still understand :-)

[bergus]

This essentially reverts 526cd35. Instead of exposing those primitives, readTemplate should have been used.

To benefit from error messages, I used the compileTemplateItem compiler in Feed.hs now, even though it requires an Item with a name. Not sure whether that is necessary, or whether we should simply assume that the embedded templates contain no errors.

Btw, could unsafeReadTemplateFile be deprecated? It seems it was originally created for Feed.hs, and is no longer used anywhere now.

[jaspervdj]

Yeah, we should be good to rip out unsafeReadTemplateFile

[bergus]

OK, I've reviewed this Verbosity thing. I guess using Logger.Verbosity is confusing indeed, as it doesn't directly have to do with logging.

All this hassle arises from the design of Template.Context which uses the Alternative instance of Compilers to "try" one field after the other and deciding $if(…)$s.
We would actually like to distinguish exceptions (IO errors, syntax errors, type errors, Haskell exceptions) from fields that could not be applied. However, currently both use CompilerError for signalling and collecting messages.
I can see two proper solutions for this:

• Introducing a new CompilerNoResult :: [String] -> CompilerResult a constructor which can be treated separately from errors in templates. It would get appropriate handling in the Alternative Compiler instance and might even be provided as the empty :: Compiler.
  Problem: No backcompat with code that uses fail in custom Contexts.
• Introducing a new MissingField :: ContextField constructor that can be returned in contexts to signify that nothing was found. Or similarly, changing the return type of the Context functions to Compiler (Maybe ContextField). Or rather, using Either [String] instead of Maybe, or respectively giving MissingField a [String] argument, so that we can collect helpful messages in there. (Directly logging them in the successfull compiler is not appropriate).
  It would then get appropriate handling in the Monoid Context.
  Problem: No backcompat with code that uses fail or empty :: Compiler in custom Contexts.

If you want to throw the backcompat concerns over board and release it as Hakyll 5.0, I'm all in :-)

My current approach resembles the first idea. Instead of using a separate constructor, I added a level to the CompilerError - either Message or Error (Debug isn't really used). The Alternative instance is a bit weird, but it basically says that when both attempts fail an error will take precedence over a normal message. If both failures are of the same level, their messages are concatenated.

For backcompat, even when the first alternative fails with an exception, the second one is still tried. In a proper solution, an exception would not be ignored - at least for the purposes of templates, not sure whether an error-suppressing alternative is useful elsewhere or whether catchError is enough for that.

some sort of Writer-like monoid in Compiler for logging? That way, we can just accumulate all the messages cleanly, and then at the end decide locally which ones to print etc. What do you think?

"at the end" sounds like "globally" to me? And I'm not sure whether this will work, filtering for level/verbosity in the end - we would want to locally decide which messages to keep. The template evaluation forms a tree, and if we just accumulate we would need to somehow represent that structure in the monoid?
Or maybe I just don't really understand what one can do with Writer, I have never used it before.

It currently seems that we could solve this by putting something like [(Verbosity, String)] in CompilerWrite, and modifying the CompilerError constructor to :: CompilerWrite -> CompilerResult a.

Not sure whether I grasp the scope of that change. Why use CompilerWrite here? What would the list of messages do in a CompilerDone? What would the dependencies and cache hits do in a CompilerError? I don't feel comfortable doing such an edit.

I will try to remove the Verbosity. Either by implementing data Reason = Error [String] | Message [String] and having CompilerError :: Reason -> CompilerResult a, or with that CompilerNoResult - which I quite like actually, even despite the backcompat fail semantics.

[bergus]

OK I tried

data CompilerResult a
    = CompilerDone a CompilerWrite
    | CompilerSnapshot Snapshot (Compiler a)
    | CompilerRequire (Identifier, Snapshot) (Compiler a)
    | CompilerNothing [String]
    | CompilerError [String]

but it's still really ugly:

compilerResult x = Compiler $ \_ -> return x
compilerThrow = compilerResult . CompilerError
compilerNothing = compilerResult . CompilerNothing

compilerHandle :: ([String] -> Compiler a) -> ([String] -> Compiler a) -> Compiler a -> Compiler a
compilerHandle f g (Compiler x) = Compiler $ \r -> do
    res <- x r
    case res of
        CompilerDone res' w  -> return (CompilerDone res' w)
        CompilerSnapshot s c -> return (CompilerSnapshot s (compilerHandle f g c))
        CompilerRequire i c  -> return (CompilerRequire i (compilerHandle f g c))
        CompilerNothing e    -> unCompiler (g e) r
        CompilerError e      -> unCompiler (f e) r

instance Alternative Compiler where
    empty   = compilerNothing []
    x <|> y = compilerHandle (\rxs ->
          compilerHandle (\rys ->
            compilerNothing $ rxs ++ rys
          ) (\eys ->
            log rxs >> compilerThrow eys
          ) y
        ) (\exs ->
          compilerHandle (\rys ->
            log rys >> compilerThrow exs
          ) (\eys ->
            compilerNothing $ exs ++ eys
          ) y
        ) x
      where
        log = compilerDebugLog . map
            ("Hakyll.Core.Compiler.Internal: Alternative fail suppressed: " ++)

[jaspervdj]

I would also favor the first approach you presented, and the code doesn't look too bad IMO.

But I'm a bit unsure about the Alternative instance -- it looks like x <|> empty might change CompilerError to CompilerNothing or the other way around? Wouldn't that violate the laws?

I also think CompilerEmpty is a better name than CompilerNothing.

As far as backwards compatibility goes, I care most about keeping everything working. If users get a slightly less good error message because they are using e.g. fail rather than empty (or something else entirely), that's fine.

[bergus]

code doesn't look too bad IMO.

I did like the Alternative implementation with the Verbosity much better than this compilerHandle thingy with two callbacks. Also CompilerNothing needed to be added almost everywhere as a repetition of the CompilerError case. Admittedly Verbosity was a horrible choice, I went with a new data type designed specifically for this now. See commit below.

But I'm a bit unsure about the Alternative instance -- it looks like x <|> empty might change CompilerError to CompilerNothing or the other way around? Wouldn't that violate the laws?

The idea is that CompilerError always takes precedence over CompilerNothing. x <|> empty = x should hold - the Nothing [] gets dropped, and there shouldn't even be a debug log entry from no messages.

I also think CompilerEmpty is a better name than CompilerNothing.

That name I wasn't happy about either :-) Good suggestion - I had the same idea for renaming the NoField.

[bergus]

I guess unReason might have followed the naming conventions, but it seemed kinda unreasonable :-D

[jaspervdj]

😁

[bergus]

I don't remember why I did pass no messages to the the handler here - to make it distinguishable?
Now catchError will also catch NoCompilationResults, not just CompilationFailures, which seems reasonable - they are errors, at least outside of template applications.
However, c `catchError` throwError will change the error reason type, which I am not so happy about. Maybe we should export a helper function that allows altering (adding to) the messages without changing the type. Bifunctor comes to my mind, but our messages are string lists not arbitrary types.

[bergus]

I could have made Reason a Semigroup or Monoid instance for this to make the code even cleaner, but that doesn't allow to put the discarded messages in the debug.

[bergus]

I guess this one should also be re-exported from Core.Compiler, as it's useful for people writing custom Contexts with error messages.

[jaspervdj]

The separate datatype makes the code much more obvious, nice job!

I think compilerNoResult needs a Haddock to indicate why it's there. Should we also do fail = compilerNoResult? I'm not fully convinced about the name yet. Maybe something like compilerFailBranch?

Other than that, I think all the code looks good now! :-)

[bergus]

Should we also do fail = compilerNoResult?

No, fail should stay reserved for unrecoverable errors.

I'm not fully convinced about the name yet. Maybe something like compilerFailBranch?

Me neither. compilerFailBranch sounds good! Haddock documentation coming.

[jaspervdj]

This was merged in #722