Opened 2 years ago

Closed 8 months ago

#9022 closed bug (fixed)

TH pretty printer and GHC parser semicolon placement mismatch

Reported by: roldugin Owned by: bollmann
Priority: normal Milestone:
Component: Template Haskell Version: 7.8.1
Keywords: newcomer Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: GHC rejects valid program Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D1898
Wiki Page:

Description

In GHC 7.8 TemplateHaskell pretty printer started inserting explicit braces and semicolons.

It puts semicolons at the end of the line as opposed to the beginning of the next line.

This causes GHC to fail parsing if we try to compile the pretty printed code.

$ cat Foo.hs
module Main where

import Language.Haskell.TH

main = putStrLn $ pprint foo

foo :: Dec
foo = barD
  where
       barD = FunD ( mkName "bar" )
                   [ Clause manyArgs (NormalB barBody) [] ]

       barBody = DoE [letxStmt, retxStmt]

       letxStmt = LetS [ ValD (VarP xName) (NormalB $ LitE $ IntegerL 5) [] ]

       retxStmt = NoBindS $ AppE returnVarE xVarE

       xName = mkName "x"

       returnVarE = VarE $ mkName "return"

       xVarE = VarE xName

       manyArgs = map argP [0..9]

       argP n = VarP $ mkName $ "arg" ++ show n

$ ghc-7.8.2 Foo.hs
[1 of 1] Compiling Main             ( Foo.hs, Foo.o )
Linking Foo ...

$ ./Foo | tee Bar.hs
bar arg0 arg1 arg2 arg3 arg4 arg5 arg6 arg7 arg8 arg9 = do {let x = 5;
                                                            return x}

$ ghc Bar.hs
[1 of 1] Compiling Main             ( Bar.hs, Bar.o )

Bar.hs:2:61: parse error on input `return'

I don't know if this is a problem with TH pretty printer or if GHC is supposed to parse semicolons wherever they are..

Attachments (1)

9022.patch (1.5 KB) - added by roldugin 23 months ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 23 months ago by thomie

  • Component changed from Compiler (Parser) to Test Suite
  • Milestone set to 7.10.1

Thank you for the report.

I confirm this problem does not occur in 7.6.3, and does occur with 7.8.1 and HEAD. I can not find anything about this in the 7.8.1 changelog. I did find a related change, but then for the GHC Api pretty printer.

commit 6f32f9b429814499cfde1367e59d3e46bffc4925
Author: simonpj@microsoft.com <unknown>
Date:   Thu Jul 23 15:24:11 2009 +0000

    Print explicit braces and semicolons in do-notation
    
    By printing explicit braces we make it more likely that pretty-printed
    code will be acceptable if fed back into GHC.
    
    See http://www.haskell.org/pipermail/glasgow-haskell-users/2009-July/017554.html

comment:2 Changed 23 months ago by thomie

  • Component changed from Test Suite to Template Haskell
  • Version changed from 7.8.2 to 7.8.1

comment:3 Changed 23 months ago by simonpj

I think the solution is not to be very delicate about how to place the semicolons for the do-notation. Rather, we should just use braces and semicolons for let-notation too. At least for lets in do-notation but perhaps for all.

Then you'd get

bar arg0 arg1 arg2 arg3 arg4 arg5 arg6 arg7 arg8 arg9 = do {let { x = 5 };
                                                            return x}

which should work fine.

Anyone want to do this? It would be consistent with the story for do-notation.

Simon

Changed 23 months ago by roldugin

comment:4 Changed 23 months ago by roldugin

There was a discussion in the mailing list and I fixed it based on advice I got there.

The fix worked for me but I haven't validated it.

I no longer have write access to the repo, so I've attached it above.

comment:5 Changed 23 months ago by simonpj

Did you test your fix?

let {x = 3} ; {y = 4} in x+y

is not valid Haskell, but is (I believe) what your patch will generate.

Simon

comment:6 Changed 23 months ago by roldugin

You're right!

let expressions should be fine:

> pprint <$> runQ [| let { x = 3; y = 4 } in x + y |]
"let {x_0 = 3; y_1 = 4}\n in x_0 GHC.Num.+ y_1"

But there is indeed a problem with let statements:

> pprint <$> runQ [| do let { x = 3; y = 4 } ; return (x + y) |]
"do {let {x_0 = 3}; {y_1 = 4}; GHC.Base.return (x_0 GHC.Num.+ y_1)}"

I think pretty printing LetS will need to use custom pprDecs like LetE does around line 149.

Unfortunately, I can't do it right now but I will when I get a minute.

George

comment:7 Changed 23 months ago by goldfire

  • Keywords newcomer added

comment:8 Changed 22 months ago by thoughtpolice

  • Milestone changed from 7.10.1 to 7.12.1

Moving to 7.12.1 milestone; if you feel this is an error and should be addressed sooner, please move it back to the 7.10.1 milestone.

comment:9 Changed 19 months ago by thomie

  • Owner set to roldugin

roldugin: assigning to you, since you were working on this.

comment:10 Changed 13 months ago by thoughtpolice

  • Milestone changed from 7.12.1 to 8.0.1

Milestone renamed

comment:11 Changed 13 months ago by thomie

  • Owner roldugin deleted

comment:12 Changed 8 months ago by thomie

  • Milestone 8.0.1 deleted

comment:13 Changed 8 months ago by bollmann

It seems that this issue was already fixed in ticket #10734. I just added the above test locally and it passes in the current head: https://git.haskell.org/ghc.git/commitdiff/00cbbab3362578df44851442408a8b91a2a769fa.

Shall I add the above testcase as part of the TH testsuite and then close the ticket? Or shall we just close this ticket right away?

comment:14 Changed 8 months ago by bollmann

  • Owner set to bollmann

comment:15 Changed 8 months ago by bollmann

  • Differential Rev(s) set to D1898
  • Status changed from new to patch

I just added the regression test mentioned above to the testsuite and pushed the code for review. However, I actually think, we should just close this ticket without adding the test (as the issue has already been fixed in #10734 and that ticket also added these sorts of tests). Let me know what you think.

Last edited 8 months ago by bollmann (previous) (diff)

comment:16 Changed 8 months ago by bollmann

  • Differential Rev(s) changed from D1898 to Phab:D1898

comment:17 Changed 8 months ago by Ben Gamari <ben@…>

In 2f9931e3/ghc:

add Template Haskell regression test for #9022.

The bug itself has already been fixed in #10734, so this
only adds another regression test (as given in the ticket).

Test Plan: ./validate

Reviewers: goldfire, austin, thomie, bgamari

Reviewed By: bgamari

Differential Revision: https://phabricator.haskell.org/D1898

GHC Trac Issues: #9022

comment:18 Changed 8 months ago by bollmann

  • Resolution set to fixed
  • Status changed from patch to closed
Note: See TracTickets for help on using tickets.