Fix handling of ApplicativeDo in TH AST quotes
ClosedPublic

Authored by mgsloan on Jun 30 2018, 3:35 AM.

Details

Summary

See https://ghc.haskell.org/trac/ghc/ticket/14471

Also fixes a parenthesization bug in pprStmt when ret_stripped is True

Test Plan

tests added to testsuite

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.
mgsloan created this revision.Jun 30 2018, 3:35 AM
mgsloan edited the summary of this revision. (Show Details)Jun 30 2018, 3:36 AM
goldfire accepted this revision.Jul 2 2018, 10:19 PM

Oof. This is ugly. It suggests we need a better AST for statements. But I don't see a better way to accommodate TH here.

Thanks!

This revision is now accepted and ready to land.Jul 2 2018, 10:19 PM

@goldfire Welcome, thanks for reviewing! I don't think that this suggests we should have a better AST for TH, because I don't think ApplicativeDo structures should be visible to TH. The logic here is modeled closely after the ppr logic.

It is certainly true that this isn't pretty. Perhaps it would instead be possible to turn off the ApplicativeDo transformation while handling TH AST quotes? Then DsMeta could just panic on the ApplicativeDo stuff because it should never be present.

@goldfire Welcome, thanks for reviewing! I don't think that this suggests we should have a better AST for TH, because I don't think ApplicativeDo structures should be visible to TH. The logic here is modeled closely after the ppr logic.

I meant a better GHC AST -- not the TH AST. Sorry for the confusion!

It is certainly true that this isn't pretty. Perhaps it would instead be possible to turn off the ApplicativeDo transformation while handling TH AST quotes? Then DsMeta could just panic on the ApplicativeDo stuff because it should never be present.

That's a compelling idea. To be honest, though, I'm not sure of the ramifications of that decision, not being familiar enough with ApplicativeDo. Are there programs that would type-check that shouldn't? Programs that are rejected that shouldn't?

That's a compelling idea. To be honest, though, I'm not sure of the ramifications of that decision, not being familiar enough with ApplicativeDo. Are there programs that would type-check that shouldn't? Programs that are rejected that shouldn't?

Yeah, I wish I'd thought of this in the first place! I'm giving it a try now and it seems like a much simpler change. I will update this diff soon with the simpler approach. Not applying the ApplicativeDo transformation when renaming TH brackets should have exactly the same behavior as the change here, and I think it is the right thing to do for TH AST brackets.

ApplicativeDo just uses the applicative operators to implement do notation, when possible. There are no programs that would typecheck that shouldn't, or programs that are rejected that shouldn't.

It is worth exploring whether doE gets ApplicativeDo applied to it, but I think that's a separate issue from this.

mgsloan updated this revision to Diff 17230.Jul 7 2018, 9:23 PM

Instead of undoing ApplicativeDo in DsMeta, don't apply the transform within TH brackets

Glad I gave this approach a whirl, I think this is much cleaner 👍

@goldfire Please take a look when you have a spare cycle

goldfire accepted this revision.Jul 8 2018, 6:16 PM

This certainly is an easier way of fixing the problem. But I'm still a bit worried, now that I've thought more about it.

What does it mean when we have [| expr |]? Specifically, how do we treat expr with respect to any extensions we have enabled?

To be more concrete, suppose we have

{-# LANGUAGE ApplicativeDo, TemplateHaskell #-}

module Quote where

quote = [| do ... |]   -- subject to ApplicativeDo transform
{-# LANGUAGE TemplateHaskell #-}  -- NB: NoApplicativeDo

import Quote

action = $quote

Should action use applicative combinators or monadic ones? I think the answer has to be "monadic ones", treating a quote just as a shorthand for a little slice of AST.

But TH does not respect this notion currently. Witness this example:

{-# LANGUAGE RebindableSyntax, TemplateHaskell #-}

module Quote2 where

quote2 = [| if 5>3 then 'x' else 'y' |]

This module fails, because ifThenElse and fromInteger are not in scope. This is wrong for several reasons:

  1. Template Haskell is fine, actually, with things that aren't in scope. If something isn't in scope, TH just uses UnboundVarE.
  1. If we bring some ifThenElse and fromInteger into scope (regardless of what they actually are), the quote doesn't use them at all. (It does label the (>) operator as unbound, though, because RebindableSyntax implies NoImplicitPrelude.)

So maybe my bottom line here is that TH does respect the "it's just a slice of AST" viewpoint, but then implements this idea buggily in the presence of RebindableSyntax when a quote is made.

Which all goes to say that I approve this patch. And I found a bug, to be reported separately.

compiler/hsSyn/HsExpr.hs
2234 ↗(On Diff #17230)

I think this change is just a holdover from other refactoring. If so, then please undo, so git blame, etc., works better.

compiler/rename/RnExpr.hs
736

This needs more of a comment describing what the problem is. Could you put a Note in about this? It's really quite far from obvious how ApplicativeDo interacts with TH!

737

Well, that was easy.

bgamari retitled this revision from Fix handling of ApplicativeDo in TH AST quotes (Trac #14471) to Fix handling of ApplicativeDo in TH AST quotes.Jul 12 2018, 10:27 AM
bgamari updated the Trac tickets for this revision.
bgamari added inline comments.Jul 12 2018, 10:30 AM
compiler/hsSyn/HsExpr.hs
2234 ↗(On Diff #17230)

Indeed; we end up with redundant parens with this change. I'll revert when I merge.

This revision was automatically updated to reflect the committed changes.