Run typed splices in the zonker
Needs ReviewPublic

Authored by mpickering on Oct 30 2018, 11:27 AM.

Details

Reviewers
simonpj
goldfire
bgamari
Trac Issues
#15471
Summary

This fixes Trac #15471

In the typechecker we check that the splice has the right type but we
crucially don't zonk the generated expression. This is because we might
end up unifying type variables from outer scopes later on.

mpickering created this revision.Oct 30 2018, 11:27 AM
mpickering updated this revision to Diff 18538.Oct 31 2018, 8:09 AM
  • Rename the spliced expression in the right context
mpickering updated this revision to Diff 18539.Oct 31 2018, 8:13 AM
  • Add Simon's test as well
mpickering updated this revision to Diff 18540.Oct 31 2018, 9:33 AM
  • Add dummy dependency on GHC.Types
mpickering updated this revision to Diff 18541.Oct 31 2018, 9:49 AM
  • Revert "Add dummy dependency on GHC.Types"

The build works on hadrian but not on the old build system.

mpickering updated this revision to Diff 18638.Nov 9 2018, 2:42 PM
  • Simplify
  • Remove unused variable
mpickering updated this revision to Diff 18666.Nov 12 2018, 8:25 AM
  • Remove extra arguments
goldfire requested changes to this revision.Nov 12 2018, 8:44 AM

This looks like it works, but I think it can be refactored to be simpler. Thanks!

compiler/hsSyn/HsExpr.hs
46

Ew. Do we really need this? I don't think so -- see my comments in TcSplice.

2445

It would be nice to have a Note somewhere describing how this new system works.

2587

Unevaluated typed splice, perhaps?

compiler/typecheck/TcSplice.hs
498

I don't see the motivation for storing this as a delayed computation. I'd much rather see HsSplicedT store the bits and pieces necessary for the zonker just to call runTopSplice. I think that would be considerably simpler (and easier to debug, print, etc.).

531

Why does this need to be orig_expr? Can't it just be q_expr? That would mean we don't need to preserve orig_expr.

560

Instead of adding this parameter, better just for the caller to zonk if it likes to.

testsuite/tests/th/all.T
446

Oops!

This revision now requires changes to proceed.Nov 12 2018, 8:44 AM
mpickering marked an inline comment as done.Nov 12 2018, 12:09 PM

If you look at the first version of this patch, I did as you suggested and persisted all the original parts but I think it was more complicated. You still need to persist TcLclEnv and so end up with an import cycle and several {-# SOURCE #-} imports in the zonker.

See: https://phabricator.haskell.org/D5286?vs=on&id=18534&whitespace=ignore-most#toc

This way, all the code which deals with running the splice still exists in TcSplice. The TcRnTypes.hs-bootconceptually just contains TcM which will not change.

compiler/typecheck/TcSplice.hs
498

It was like this before but it meant importing a bunch of stuff into TcHsType which caused import cycles. Doing it this way means all the code to do with splices is in the same module rather than being split out into the zonker as well.

531

It needs to be orig_expr as this is used to print the message to users which describes what the splice expands to.

goldfire added inline comments.Nov 12 2018, 3:06 PM
compiler/typecheck/TcSplice.hs
498

Grumble. OK.

531

But, I think q_expr and orig_expr should pretty-print identically.

mpickering updated this revision to Diff 18674.Nov 12 2018, 4:27 PM
  • zonk after calling tcTopSpliceExpr
  • Remove merge artefact
  • Note and Richard's other comments
mpickering updated this revision to Diff 18678.Tue, Nov 13, 3:35 AM
  • Simplify
  • Remove unused variable
  • Remove extra arguments
  • zonk after calling tcTopSpliceExpr
  • Remove merge artefact
  • Note and Richard's other comments
  • Remove diff churn
mpickering updated this revision to Diff 18679.Tue, Nov 13, 3:39 AM
  • Remove blank line
  • Remove blank line
  • More formatting
mpickering planned changes to this revision.Tue, Nov 13, 5:52 AM

Still one problem with finalisers getting run in the wrong place.

mpickering updated this revision to Diff 19044.Fri, Dec 7, 8:18 AM
  • Simplify
  • Remove unused variable
  • Remove extra arguments
  • zonk after calling tcTopSpliceExpr
  • Note and Richard's other comments
  • Remove diff churn
  • Remove blank line
  • Remove blank line
  • More formatting
  • Store arguments to runTopSplice rather than whole TcM action
  • Run and zonk finalisers

I had a call with Simon and he suggested two changes which I have implemented now. The finaliser bug should also be fixed.

CI: https://gitlab.staging.haskell.org/mpickering/ghc/pipelines/105

mpickering updated this revision to Diff 19046.Fri, Dec 7, 8:57 AM
  • Try to fix build ordering

@goldfire, perhaps you could have one final look over this?