Fail instead of panic-ing when qAddTopDecls has conversion error
Needs ReviewPublic

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

Details

Reviewers
goldfire
bgamari
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.

mgsloan created this revision.Sun, Jul 1, 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.Mon, Jul 2, 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.Mon, Jul 2, 10:15 PM

Is there a test case to add? Thanks!

compiler/typecheck/TcSplice.hs
899

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.Mon, Jul 2, 10:15 PM
mgsloan updated this revision to Diff 17179.Tue, Jul 3, 11:34 PM
  • Better error message for addTopDecls conversion errors
mgsloan marked an inline comment as done.Tue, Jul 3, 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
899

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.