Hide the CallStack implicit parameter
ClosedPublic

Authored by gridaphobe on Jan 21 2016, 3:47 PM.

Details

Summary

The implicit parameter isn't actually very relevant to the CallStack machinery, so we hide the implementation details behind a constraint alias

type HasCallStack = (?callStack :: CallStack)

This has a few benefits:

  1. No need to enable ImplicitParams in user code.
  2. No need to remember the ?callStack naming convention.
  3. Gives us the option to change the implementation details in the future with less user-land breakage.

The revised CallStack API is exported from GHC.Stack and makes no mention of the implicit parameter.

Test Plan

./validate

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.
gridaphobe updated this revision to Diff 6347.Jan 21 2016, 3:47 PM
gridaphobe retitled this revision from to Hide the CallStack implicit parameter.
gridaphobe updated this object.
gridaphobe edited the test plan for this revision. (Show Details)
gridaphobe added a reviewer: simonpj.
gridaphobe set the repository for this revision to rGHC Glasgow Haskell Compiler.
gridaphobe added a project: GHC.
simonpj accepted this revision.Jan 21 2016, 5:40 PM
simonpj edited edge metadata.

good!

This revision is now accepted and ready to land.Jan 21 2016, 5:40 PM
bgamari accepted this revision.Jan 22 2016, 5:17 AM
bgamari edited edge metadata.

This looks quite nice. Let me know when I should have a look at the documentation.

libraries/base/GHC/Stack.hs
24–25

Is errorWithStackTrace really a "Profiling call stack"?

bgamari requested changes to this revision.Jan 25 2016, 11:12 AM
bgamari edited edge metadata.

Bumping out of merge queue while documentation is updated.

This revision now requires changes to proceed.Jan 25 2016, 11:12 AM
gridaphobe updated this revision to Diff 6428.Jan 25 2016, 1:05 PM
gridaphobe edited edge metadata.

A first draft of the revised CallStack docs, comments are very welcome.

Calling the feature "Implicit CallStack" doesn't make a whole lot of sense anymore since we're hiding the implicit parameter, so I'm just calling it "HasCallStack" after the constraint. I don't particularly like this as a feature name, but I don't have a better idea at the moment.

Looks good to me.

A few small things inline.

docs/users_guide/glasgow_exts.rst
12819

These code blocks should probably be syntax highlighted. For this use ::. For instance,

For example, we can define ::

    errorWithCallStack :: HasCallStack => String -> a
libraries/base/GHC/Stack.hs
24–25

I still don't understand why errorWithStackTrace is in the Profiling call stacks section.

gridaphobe updated this revision to Diff 6434.Jan 25 2016, 6:05 PM
gridaphobe edited edge metadata.

tweak code blocks

gridaphobe marked an inline comment as done.Jan 25 2016, 6:06 PM
gridaphobe added inline comments.
libraries/base/GHC/Stack.hs
24–26

Sorry, missed this. I agree it doesn't belong there anymore.

bgamari accepted this revision.Jan 26 2016, 4:51 AM
bgamari edited edge metadata.

This looks good to me. @gridaphobe, do you feel this is read to merge?

This revision is now accepted and ready to land.Jan 26 2016, 4:51 AM
gridaphobe updated this revision to Diff 6452.Jan 26 2016, 12:13 PM
gridaphobe edited edge metadata.
gridaphobe marked an inline comment as done.

rebase and make a few more tweaks. @bgamari, I'm happy with the docs now (main change was to add a step explaining when GHC infers HasCallStack constraints).

gridaphobe updated this object.Jan 26 2016, 12:13 PM
gridaphobe edited edge metadata.

These changes are only in the libarries and documenation.

Don't we need to edit the Notes in GHC's source code to connect the CallStack implicit parameter stuff e.g. isCallStackDict with the HasCallStack stuff that the user sees?

Perhaps including remarks that we might ulitmately change the implementation, so we should strongly discourage use of the underlying implicit parametrer.

Generally great though -- thank you.

bgamari requested changes to this revision.Jan 27 2016, 5:19 AM
bgamari edited edge metadata.

Also, it seems like there's a good number of testsuite testcases whose expected output needs to be updated,

Unexpected results from:
TEST="T5628 T2120 T11193 SafeLang09 T457 T5587 T5625 T10845 IPLocation T10529b T10529c T10529a T5976 T5358 TH_exn2 T8987 conc021 sigof02m sigof02 annfail12 T10501 T5557 ffi008 fptrfail01 break017 break011 break009 arr004 arr007 arr008 arr003 tough2 assert readFloat strun002 cgrun045 T5626 cgrun051 cgrun016 cgrun059 stm060 tough T7411 ghc-e005"

OVERALL SUMMARY for test run started at Wed Jan 27 06:11:53 2016 EST
 0:05:59 spent to go through
    5024 total tests, which gave rise to
   15891 test cases, of which
   10821 were skipped

      71 had missing libraries
    4843 expected passes
     107 expected failures

       0 caused framework failures
       0 unexpected passes
      49 unexpected failures
       0 unexpected stat failures

These look like they are mostly of the form,

--- ../../libraries/hpc/tests/simple/tixs/T10529a.stderr.normalised	2016-01-27 06:17:23.302832344 -0500
+++ ../../libraries/hpc/tests/simple/tixs/T10529a.run.stderr.normalised	2016-01-27 06:17:23.302832344 -0500
@@ -1,3 +1,3 @@
 hpc: can not find NonExistingModule in ./.hpc
-CallStack (from ImplicitParams):
+CallStack (from HasCallStack):
   error, called at libraries/hpc/Trace/Hpc/Mix.hs:<line>:<column> in <package-id>:Trace.Hpc.Mix
This revision now requires changes to proceed.Jan 27 2016, 5:19 AM

@simonpj I warned about the underlying implicit parameter in the haddocks for HasCallStack and CallStack. I didn't mention it in the user's guide because I think the only way you would discover the IP is by browsing the haddocks or source. But I can certainly copy the warning to the user's guide if you think it would be beneficial.

@bgamari silly me, I forgot that harbormaster isn't validating patches at the moment. Working though the tests now.

gridaphobe updated this revision to Diff 6480.Jan 27 2016, 4:16 PM
gridaphobe edited edge metadata.

fix the testsuite, tie-in HasCallStack in "Note [Overview of implicit CallStacks]", and remove setCallStack as it does not work as intended (the intended function can't be written in Haskell).

@simonpj I don't think it makes sense to reframe the Note in terms of HasCallStack; from the perspective of GHC, HasCallStack is just an alias, the interesting bit is the underlying implicit parameter. I've referenced the alias so there's a connection between the user-facing docs and the internal docs, and I'll make a bigger pass over the Note if/when we remove the implicit parameter. Does that make sense?

simonpj accepted this revision.Jan 27 2016, 5:27 PM
simonpj edited edge metadata.

Yes I just want a connection between the GHC source code and the library code.

compiler/typecheck/TcEvidence.hs
531

Give the alias right here! It's really important to understand what it is.

Say where it is defined (GHC.Stack.Types I think).

531–532

Regular, but special. In particular the *name* chosen is not important but the *type* is important.

gridaphobe updated this revision to Diff 6482.Jan 27 2016, 6:07 PM
gridaphobe edited edge metadata.

give the HasCallStack alias in the Note, add the API warning to the user's guide as well

bgamari accepted this revision.Feb 1 2016, 7:01 AM
bgamari edited edge metadata.

Looks good to me. Thanks @gridaphobe!

This revision is now accepted and ready to land.Feb 1 2016, 7:01 AM
This revision was automatically updated to reflect the committed changes.

I’m feeling a bit vain right now and want to add, just for the record, that I had the idea for this type synonym: https://www.mail-archive.com/ghc-devs@haskell.org/msg10805.html