Expose source locations via Implicit Parameters of type GHC.Location.Location
ClosedPublic

Authored by gridaphobe on Dec 18 2014, 11:41 PM.

Details

Summary

IPs with this type will always be solved for the current source
location. If another IP of the same type is in scope, the two locations will be
appended, creating a call-stack. The Location type is kept abstract so users
cannot create them, but a Location can be turned into a list of SrcLocs, which
correspond to individual locations in a program. Each SrcLoc contains a
package/module/file name and start/end lines and columns.

The only thing missing from the SrcLoc in my opinion is the name of the
top-level definition it inhabits. I suspect that would also be useful, but it's
not clear to me how to extract the current top-level binder from within the
constraint solver. (Surely I'm just missing something here?)

I made the (perhaps controversial) decision to have GHC completely ignore
the names of Location IPs, meaning that in the following code:

bar :: (?myloc :: Location) => String
bar = foo

foo :: (?loc :: Location) => String
foo = show ?loc

if I call bar, the resulting call-stack will include locations for

  1. the use of ?loc inside foo,
  2. foos call-site inside bar, and
  3. bars call-site, wherever that may be.

This makes Location IPs very special indeed, and I'm happy to change it if the
dissonance is too great.

I've also left out any changes to base to make use of Location IPs, since there
were some concerns about a snowball effect. I think it would be reasonable to
mark this as an experimental feature for now (it is!), and defer using it in
base until we have more experience with it. It is, after all, quite easy to
define your own version of error, undefined, etc. that use Location IPs.

Test Plan

validate, new test-case is testsuite/tests/typecheck/should_run/IPLocation.hs

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes

Good start! I have added a few comments. I have put more global comments on Trac Trac #9049 itself.

Thanks

Simon

compiler/deSugar/DsBinds.hs
883

You only need the DataCon. You can get the TyCon with dataConTyCon. That means you can eliminate a couple of wired-in names.

899

Ditto for locationTyCon.

compiler/typecheck/TcEvidence.hs
739

The Module part is always the module being compiled, and the desugarer has access to that too. So get rid of it here, and make the desugarer get it out of the monad instead.

741

I don't understand the comment. Add a Note with an example?

compiler/typecheck/TcInteract.hs
1641

Can you give more comments? Presumably the _ in in the guard matches the string name of the implicit parameter: do say so and give an example!

1655

This is all a bit confusing. We call unDict (which calls toDict) and we call toDict directly. Are both necessary? Why not write out an example to elucidate. (in a Note of course)

1662

This duplicates fromDict in the HsIPVar case of tcExpr. Let's not duplicate code. Put it in Inst, I suggest.

gridaphobe updated this revision to Diff 2061.Jan 4 2015, 1:58 AM
gridaphobe edited edge metadata.
  • srcLocTyCon is no longer wired-in
  • elaborate on the need for coercing locations to dictionaries and vice-versa
  • let's call it a CallStack instead of a Location
  • minor change in test output

@simonpj Thanks for the comments! I've updated the diff with more comments, including a Note that explains the need for the coercion-dance between the Location (now CallStack) and the dictionary. Hopefully it will make more sense now!

I've left the locationTyConName (now callStackTyConName) wired-in because TcInteract.isCallStackIP needs it. (I suppose I could still get to the tyConName from the dataCon, but that's starting to feel clunky to me :/ I'll defer to your judgement here.)

simonmar added inline comments.
libraries/base/GHC/Location.hs
2 ↗(On Diff #2061)

Note that we also have GHC.Stack, which is the interface for programmatically obtaining the current cost-centre stack. The existence of both of these is likely to cause confusion, so I suggest (a) put CallStack and related operations in GHC.Stack and (b) have clear signposting in the documentation that explains the difference between the two.

It would be nice to make cost-centre stacks use SrcLoc too, in due course.

simonpj requested changes to this revision.Jan 5 2015, 8:08 AM
simonpj edited edge metadata.

More comments

compiler/typecheck/Inst.hs
651

This is only used for implicit parameters; it's not really as general as the comments here imply. Let's call it unwrapIP and wrapIP respectively. You can even put them in TcEvidence.

compiler/typecheck/TcEvidence.hs
847

Is this BNF helpful? We have the data type for EvCallStack already.

What we DO want is a little overview of the whole idea. Just summarise the key points of the ImplicitCallStack wiki page here, and give pointers to the key places where the plan is implemented.

850

Let's not do this dance. Once unwrapIP is in TcEvidence, the desugarer can call it just fine. I'd prefer if evidence terms had types that were, well, evidence types. Less to explain.

compiler/typecheck/TcInteract.hs
1635

Point to your overview Note from here.

1646

Crucial: you need isGiven ct here too. We only want to pick up a *given* Ct, not another wanted one!!

libraries/base/GHC/Location.hs
2 ↗(On Diff #2061)

I suggest that we put SrcLoc in GHC.SrcLoc. Then I'm ok with putting GHC.CallStack in GHC.Stack.

GHC.SrcLoc will in due course be used by Typable; but stacks will not.

This revision now requires changes to proceed.Jan 5 2015, 8:08 AM
gridaphobe added inline comments.Jan 5 2015, 10:20 AM
compiler/typecheck/Inst.hs
651

As implemented, the functions *should* work for any class that can be represented by a newtype (which IIRC means any single-method class), right? If so, wouldn't it make sense to keep the names generic, so they might be used elsewhere in time?

compiler/typecheck/TcInteract.hs
1669

@simonpj Is this panic OK? i.e. Is it possible that the ctLocSpan would be an UnhelpfulSrcSpan? I'd rather not push the eitherness into GHC.Location unless we have to.

gridaphobe updated this revision to Diff 2064.Jan 6 2015, 1:26 AM
gridaphobe edited edge metadata.

Improve the call-stack and undo the coercion-dance

  • split GHC.Location into GHC.SrcLoc and GHC.Stack
  • grab the function being called from the CtOrigin
  • only intercept IP constraints that arise from a function call or IP-use
  • make dsEvTerm unwrap the ip-dictionary on-the-fly.
  • rewrite the CallStack evidence note
  • fix errors and update test output

@simonpj I think this should address all of your comments apart from the unwrapIP/wrapIP business (I added a separate question about that). In particular, the updated IPLocation.stdout shows the new formatting of CallStacks which includes the function name.

I did notice a new issue though, we inadvertently "solve" the CallStack wanteds that arise from using :t in GHCi, which causes GHCi to print a misleading type, e.g.

ghci> :l IPLocation.hs
ghci> :t f1
f1 :: IO ()

I'm not sure how best to resolve this. We could have GHCi set a flag in the typechecker that disables the special casing of CallStacks, but that feels kludgy. We could also leave GHCi alone; the type it prints isn't incorrect, i.e. it's safe to call f1 in a context that doesn't define a CallStack IP as GHC will always provide the CallStack. But this also feels wrong.. Any thoughts?

gridaphobe updated this revision to Diff 2065.Jan 6 2015, 1:49 AM
gridaphobe edited edge metadata.
  • missing export
simonpj accepted this revision.Jan 6 2015, 5:08 AM
simonpj edited edge metadata.

Great! I've added more suggestions

@simonpj I think this should address all of your comments

You didn't add an overview Note, did you?

Also it would be really really good to update the wiki page so that it precisely reflects reality.

apart from the unwrapIP/wrapIP business (I added a separate question about that)

What question? Where?

In particular, the updated `IPLocation.stdout` shows the new formatting of `CallStack`s which includes the function name.

I like it, but see comments. I'd like a Name not a FastString in the evidence, so that if we want to put in qualified names we can easily do so.

I did notice a new issue though, we inadvertently "solve" the CallStack wanteds that arise from using :t in GHCi, which causes GHCi to print a misleading type, e.g

I think it's ok. After all, if you call f1 from GHCi, you'd expect it to solve the constraints, run the call, and display a suitable call stack if necessary. Use :info if you want the original type. But it would be worth noting this on the wiki page and the user manual.

compiler/typecheck/Inst.hs
651

I know. But it's not something we generally do. Let's keep it specialised for now, and put it in TcEvidence.

compiler/typecheck/TcEvidence.hs
739

Use EvCsEmpty, an empty stack, rather than duplicating.

740

Split into two.

EvCsPushCall Name RealSrcSpan EvTerm

for calls

EvCsTop OccName RealSrcSpan EvTerm

for occurrences of (?loc)
compiler/typecheck/TcInteract.hs
1669

Good question. It comes from tcl_loc.

Hmm. Looks to me as if tcl_loc could be a RealSrcSpan. Then CtLoc can be too. I'll do that, as a separate patch.

libraries/base/GHC/Stack.hsc
170

You are tragically missing a GOLDEN opportunity for a comment here. What is the String? What is the SrcLoc? What is a CallStack? Which order is it in (most recently called at top?).

Plus a pointer to the overview Note!

This revision is now accepted and ready to land.Jan 6 2015, 5:08 AM
simonpj requested changes to this revision.Jan 6 2015, 5:10 AM
simonpj edited edge metadata.

Actually I meant "request changes"!

This revision now requires changes to proceed.Jan 6 2015, 5:10 AM

I did the CtLoc thing, so it now contains a RealSrcSpan.

apart from the unwrapIP/wrapIP business (I added a separate question about that)

What question? Where?

It was an inline comment, but you responded to it :)

I did notice a new issue though, we inadvertently "solve" the CallStack wanteds that arise from using :t in GHCi, which causes GHCi to print a misleading type, e.g

I think it's ok. After all, if you call f1 from GHCi, you'd expect it to solve the constraints, run the call, and display a suitable call stack if necessary. Use :info if you want the original type. But it would be worth noting this on the wiki page and the user manual.

Ok, documenting the behavior seems like a good approach, I'll include that in the next revision.

compiler/typecheck/TcEvidence.hs
841

@simonpj I rewrote this Note to be the overview Note, the previous version was mostly just detailing the coercion-dance that we're not doing anymore.

In D578#17030, @simonpj wrote:

I did the CtLoc thing, so it now contains a RealSrcSpan.

Thanks!

gridaphobe updated this revision to Diff 2071.Jan 7 2015, 12:03 AM
gridaphobe edited edge metadata.

Use Names instead of Strings where possible and update the CallStack haddock to be more informative

  • specialize dictionary coercions to IP-dictionaries
  • split EvCallStack into 3 constructors
  • fix definition of evVarsOfTerm on EvCallStack
  • note quirky :type behavior in the manual

@simonpj let me know if the current version of the Note (Note [CallStack evidence terms]) is ok as an overview (it might still be too low-level?)

I'm not convinced by the addition of the EvCsEmpty constructor. It introduces the possibility of an empty stack (which we can guarantee will never happen!) What about

data EvCallStack
  = EvCsTop FastString RealSrcSpan       -- for occurrences of ?loc
  | EvCsPushCall Name RealSrcSpan EvTerm -- for call-sites

(By the way, the FastString is necessary in the ?loc case because that's what the CtOrigin gives us, but I think this is fine since IPs will always be unqualified.)

simonpj accepted this revision.Jan 7 2015, 9:59 AM
simonpj edited edge metadata.

Great. I'm pretty happy now.

Some minor comments.

Thanks!

Simon

compiler/deSugar/DsBinds.hs
880

I'd use an auxiliary function dsEvCallStack. (minor stylistic)

compiler/typecheck/TcEvidence.hs
738

Refer to Note!

740

Comment! (EvCsPushCall name loc stk) represents a call to function 'name' at location 'loc', in a calling context 'stk'.

741

Comment! (EvCsTop name loc stk) represents a use of implicit parameter ?name at location 'loc' in context 'stk'.

841

Yes this is good. Refer to the wiki page explicitly

Maybe the title can be `Note [Overview of implicit CallStacks]

Also refer back to the Note from all the places it refers to

886

Give an example of what this statement means

891

(defined in base library GHC.Stack)

897

What does this mean? Give an example

901

I think we decided to change this

912

Better to say: the result of desugaring an EvCallStack evidence term is a CoreExpr of type IP "some string" CallStack. The desugarer often needs to unwrap the IP box to get the CallStack inside; see DsBinds.dsEvCallStack.

Or something like that.

917

I don't understand this AT ALL!

1080

For both wrapIP and unwrapIP I suggest returning a Coercion not a TcCoercion. It's easy to convert the former into the latter (use the constructor TcCoercion), but a Coercion is just what we need in dsEvTerm.

1082

unwrapIP :: Type -> TcCoercion
That is, give it a Type as argument. Less work for caller.

-- Given (unwrapIP ty)
-- we expect ty to have form (IP "string" CallStack)
-- and return a coercion co :: ty ~ CallStack
1083

Also use unwrapIP in the HsIPVar case of TcExpr.tcExpr!

1092

Take arguments curried (ie two Type arguments).

libraries/base/GHC/Stack.hsc
184

personally I'd say "called at" rather than "called from".

This revision is now accepted and ready to land.Jan 7 2015, 9:59 AM
gridaphobe added inline comments.Jan 7 2015, 9:03 PM
compiler/typecheck/TcEvidence.hs
917

Oh you're right, that explanation is completely bogus.

The actual source of the constraint is the ambiguity check on showLocs type signature. I'm updating the Note!

gridaphobe updated this revision to Diff 2081.Jan 7 2015, 9:04 PM
gridaphobe edited edge metadata.

Revert to a more standard IP behavior and improve comments

  • only push call-sites onto stacks with the same IP name
  • simplify type of [un]wrapIP
gridaphobe updated this revision to Diff 2082.Jan 7 2015, 9:09 PM
  • missed a pointer to the Note
gridaphobe updated this revision to Diff 2084.Jan 7 2015, 9:18 PM
  • forgot to update the manual
gridaphobe added a comment.EditedJan 7 2015, 9:21 PM

@simonpj Ok, I went ahead and switched the behavior re: pushing onto an existing stack to the more intuitive version. I've also updated the Implementation Details on the wiki page to reflect the current revision.

Thanks for all of the comments and suggestions!

A couple more suggestions, but basically good to go

compiler/deSugar/DsBinds.hs
883

..in TcEvidence

compiler/typecheck/TcInteract.hs
610–630

actually, looking at this, it'd be neater to put the call-stack solving stuff *here*, and NOT in matchClassInst, wouldn't it? After all, the other cases look up in the interts...

1653

duplication with isCallStackIP. Better tp make the latter return Maybe EvTerm -> EvTerm, Or, I guess, have EvCsPush with an auxiliary type of EvCsStackItem.

gridaphobe updated this revision to Diff 2086.Jan 8 2015, 1:03 PM
  • move solving of CallStacks into TcInteract.interactDict
  • and the all-important release notes

@simonpj if there's nothing else, could I trouble you or @austin to land the patch for me?

gridaphobe updated this revision to Diff 2098.Jan 10 2015, 12:10 AM
  • appease ./validate
This revision was automatically updated to reflect the committed changes.

@gridaphobe why is the SrcLoc data constructor not exposed? It makes stuff like RecordWildCards unusable with it.

@gridaphobe why is the SrcLoc data constructor not exposed? It makes stuff like RecordWildCards unusable with it.

It looks like this is fixed in D861.