DsBinds: Use Typeable Module binding when desugaring CallStack evidence
Needs RevisionPublic

Authored by bgamari on Feb 27 2017, 10:47 AM.

Details

Summary

There's no reason to produce these every time we need to evidence.

Test Plan

Validate

bgamari created this revision.Feb 27 2017, 10:47 AM

Hmm, I'm not sure about this. This patch will force us to unpack the Module strings every time they're used, which would be memoized in the current code.

Furthermore, the unpacking should be shared between multiple SrcLocs in a module (at least as of D1259).

Seems like we're shoving work from compile- to run-time, which doesn't seem like the right move.

Hmm, I'm not sure about this. This patch will force us to unpack the Module strings every time they're used, which would be memoized in the current code.

Furthermore, the unpacking should be shared between multiple SrcLocs in a module (at least as of D1259).

Seems like we're shoving work from compile- to run-time, which doesn't seem like the right move.

But do we really expect string unpacking from CallStacks to be a bottleneck? In fact, I would have thought that we would generally *want* to unpack each time since we are more likely to deforest the intermediate list.

gridaphobe accepted this revision.Feb 27 2017, 12:28 PM

Good point, I suppose you're right. Might be worth a spot check with a simple example to make sure the deforestation happens?

This revision is now accepted and ready to land.Feb 27 2017, 12:28 PM
bgamari updated this revision to Diff 11372.Feb 27 2017, 2:41 PM

Make it compile

However, this still isn't quite working in the presence of TH as we don't generate a Module binding in this case. This will need to be fixed, I suppose.

bgamari updated this revision to Diff 11373.Feb 27 2017, 2:42 PM

Fix long lines

rwbarton added inline comments.
libraries/base/GHC/Stack/Types.hs
212

This breaks the API (can't match on SrcLoc { srcLocPackage = p } any more), is that okay?

gridaphobe added inline comments.Feb 27 2017, 3:10 PM
libraries/base/GHC/Stack/Types.hs
212

Good catch, I thought we had only exported an abstract type from GHC.Stack, but it turns out we export the field selectors there too.

Should we present only an abstract type to the world?

rwbarton added inline comments.Feb 27 2017, 3:11 PM
libraries/base/GHC/Stack/Types.hs
212

We could also potentially keep the same API by using functions like srcLocPackage and srcLocModule in the desugaring.

Good point, I suppose you're right. Might be worth a spot check with a simple example to make sure the deforestation happens?

Yes, a fair point.

libraries/base/GHC/Stack/Types.hs
212

This breaks the API (can't match on SrcLoc { srcLocPackage = p } any more), is that okay?

Yes, this is unfortunate. One option here would be to rename the data constructor and instead expose a pattern synonym, but it feels a bit yucky. As API breakage goes I think this is a pretty innocuous case; I suspect most people are just showing these things.

Moreover, we have been meaning to consolidate our Module and location types for a long time now; this is a good step in that direction.

We could also potentially keep the same API by using functions like srcLocPackage and srcLocModule in the desugaring.

I don't quite grok this. What do you mean?

rwbarton added inline comments.Feb 27 2017, 3:46 PM
libraries/base/GHC/Stack/Types.hs
212

Generate something like SrcLoc (modulePackageName trModule) (moduleModuleName trModule) startLine startCol endLine endCol.

bgamari added inline comments.Feb 27 2017, 4:01 PM
libraries/base/GHC/Stack/Types.hs
212

I suppose, although I was hoping to actually use the Module type in the name of Trac #10068.

rwbarton requested changes to this revision.Mar 5 2017, 1:55 PM
rwbarton added inline comments.
libraries/base/GHC/Stack/Types.hs
212

OK, but then the API having a random ' is not that great either.

libraries/deepseq
1

Is this supposed to be here?

This revision now requires changes to proceed.Mar 5 2017, 1:55 PM
austin resigned from this revision.Nov 9 2017, 9:43 AM