Fail instead of panic-ing when qAddTopDecls has conversion error
ClosedPublic

Authored by mgsloan on Jul 1 2018, 3:07 AM.

Details

Summary

See https://ghc.haskell.org/trac/ghc/ticket/14627 for an example where GHC
panics when using qAddTopDecls on [d| f = Bool |]. Instead, it should be a
normal error message, and that's what this change is for. It does not entirely
resolve Trac#14627, since "Illegal variable name: 'bool'" isn't a very good
error for this cirumstance.

Test Plan

Manually tested.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mgsloan created this revision.Jul 1 2018, 3:07 AM

Hmm, indeed the error is still a bit perplexing. In particular, why are we exposing the user to the name qAddTopDecls; this is an implementation detail, no?

mgsloan updated this revision to Diff 17157.Jul 2 2018, 5:52 PM
  • Improve message text when qAddTopDecls encounters a conversion error

@bgamari "qAddTopDecls" isn't an implementation detail, since it's exposed by the TH API as a method of the Quasi typeclass. addTopDecls is just a wrapper that forces the monad to be Q.

Indeed, even when this does not panic, the error message for this particular issue is bad. That problem is resolved by https://phabricator.haskell.org/D4923

goldfire requested changes to this revision.Jul 2 2018, 10:15 PM

Is there a test case to add? Thanks!

compiler/typecheck/TcSplice.hs
898

While this is certainly an improvement over the status quo, I don't find this message particularly easy for users to understand. (What's "conversion"?) I also wouldn't explicitly mention qAddTopDecls, because most users will just be calling addTopDecls.

This revision now requires changes to proceed.Jul 2 2018, 10:15 PM
mgsloan updated this revision to Diff 17179.Jul 3 2018, 11:34 PM
  • Better error message for addTopDecls conversion errors
mgsloan marked an inline comment as done.Jul 3 2018, 11:35 PM

Thanks for reviewing, Richard! I think I have made a change which addresses your inline comment.

There is a test-case in https://phabricator.haskell.org/D4923 which depends on this diff being merged. Three possible ways forward:

  1. Just merge it and rely on the test from D4923
  2. Add another testcase independent of D4923
  3. Close this diff and include the changes with D4923

Please let me know which you'd prefer!

compiler/typecheck/TcSplice.hs
898

Good point! I've changed it to "Error in a declaration passed to addTopDecls:" which I think is a better message.

The test case in D4923 doesn't print the error message you've added here -- that's the test I want to see. So I think this means that we need another test in this diff. Thanks.

mgsloan updated this revision to Diff 17393.Jul 19 2018, 5:58 AM
mgsloan marked an inline comment as done.
  • Add a test for addTopDecls not panicing

@goldfire Ah, right you are! Not sure quite why I didn't previously see something as obvious as that, I guess I must have forgotten that the solution there no longer encountered the error during conversion.

I've added a test for it. Is this ready for merge?

goldfire accepted this revision.Jul 19 2018, 8:50 AM

Yes, thanks!

This revision is now accepted and ready to land.Jul 19 2018, 8:50 AM
This revision was automatically updated to reflect the committed changes.