Opened 8 years ago

Closed 6 months ago

Last modified 3 months ago

#3384 closed task (fixed)

Add HsSyn prettyprinter tests

Reported by: igloo Owned by:
Priority: normal Milestone: 8.2.1
Component: Test Suite Version:
Keywords: Cc: alanz, mpickering, Shayan-Najd
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D2752
Wiki Page:

Description

Add HsSyn prettyprinter tests. See #1966 for some discussion.

Change History (20)

comment:1 Changed 3 years ago by kseo

Type of failure: None/Unknown

I'd like to work on this task as my first patch to GHC. I think the idea suggested by dfranke in #1966 is a good starting point.

Take some large Haskell projects and:

  1. Parse them
  2. Pretty-print them
  3. Reparse the output
  4. Pretty-print again

and verify that step 3 succeeds and the outputs of steps 2 and 4 match.

comment:2 Changed 3 years ago by kseo

Owner: set to kseo

comment:3 Changed 3 years ago by thomie

Version: 6.10.4

kseo: did you manage to get something going? Please ask questions on irc if you have any, maybe somebody can help you out.

comment:4 Changed 2 years ago by thomie

Cc: alanz added
Owner: kseo deleted

comment:5 Changed 2 years ago by alanz

This looks interesting.

No attempt to preserve comments/layout, but ensure it can be round tripped.

comment:6 Changed 2 years ago by mpickering

Cc: mpickering added

comment:7 Changed 7 months ago by alanz

Updating https://github.com/alanz/ghc-exactprint/blob/no-annotations/tests/Test/NoAnnotations.hs#L119 to be

  let !printed = pragmaStr ++ "\n" ++ (showSDoc_ $ GHC.ppr parsedOrig)

i.e. to simply ppr the ParsedSource and compare the resulting re-parsed AST.

The initial results are for ghc-8.0.1, of the 823 test cases in ghc-exactprint, the straight GHC ppr fails to roundtrip 106 where it cannot be reparsed, and another 84 where the re-parsed AST does not match the original.

comment:8 Changed 6 months ago by alanz

Capturing some email on ghc-devs: From @alanz

I am currently working on #3384, with the intent of ensuring that

parse (ppr (parse source)) == parse source

I have hit the issue where

foo :: (Int)

has the parens reflected in the original parsed AST, but the types pretty printer goes to a lot of trouble to remove any parens not required to interpret the meaning of the type.

So the question is, should the default ppr faithfully reproduce the source that was parsed to generate the given AST, or simplify it?

From a round-tripping perspective I prefer the former, but there are other use cases where the current behaviour is preferred.

If the original is preferred, it can perhaps be enabled via a flag to the pretty printer, but before doing that I want to see if it actually matters.

comment:9 Changed 6 months ago by alanz

Email response from Richard Eisenberg

I'm afraid I've lost track of where this discussion took place (does someone else remember seeing it?), but I've advocated for faithful reproduction in the past. In particular, the pretty-printers for HsSyn should, in my opinion, never add or remove parentheses.

There is a problem here, though: sometimes HsSyn is produced within GHC (for example, in the implementation of deriving, or in splicing TH, among a few other spots). This HsSyn can get away with missing parentheses, because it's built as an unambiguous AST. But if the pretty-printer is always faithful, then pretty-printing such an AST will produce an unparsable string. Perhaps the answer is that the generated code should be generous with parentheses -- essentially moving the paren-adding code from today's pretty-printer to the generation sites.

Bottom line here: I fully support this direction of travel, but it's not without bumps in the road.

comment:10 Changed 6 months ago by alanz

The current AST does not allow faithful reproduction of a context with extra parens around it. So

foo :: (Show a) => a -> String

and

foo :: ((Show a)) => a -> String

produce the same AST fragment. ghc-exactprint works around this by using the API Annotations.

comment:11 Changed 6 months ago by simonpj

Let me just say: the current status quo of HsSyn is not a matter of careful design, and I'm quite open to moving it around. For example, we should fix comment:10 by fixing HsSyn!

If I don't comment much it's because I'm happy to see progress here, but busy; not because I'm resisting it :-).

Simon

comment:12 Changed 6 months ago by Shayan-Najd

Cc: Shayan-Najd added

comment:13 Changed 6 months ago by alanz

Milestone: 8.2.1

comment:14 Changed 6 months ago by alanz

Differential Rev(s): Phab:D2752

comment:15 Changed 6 months ago by alanz

Status: newpatch

comment:16 Changed 6 months ago by alanz

This is passing all its tests on phab now, please review.

comment:17 Changed 6 months ago by Alan Zimmerman <alan.zimm@…>

In 499e438/ghc:

Add HsSyn prettyprinter tests

Summary:
Add prettyprinter tests, which take a file, parse it, pretty print it,
re-parse the pretty printed version and then compare the original and
new ASTs (ignoring locations)

Updates haddock submodule to match the AST changes.

There are three issues outstanding

1. Extra parens around a context are not reproduced. This will require an
   AST change and will be done in a separate patch.

2. Currently if an `HsTickPragma` is found, this is not pretty-printed,
   to prevent noise in the output.

   I am not sure what the desired behaviour in this case is, so have left
   it as before. Test Ppr047 is marked as expected fail for this.

3. Apart from in a context, the ParsedSource AST keeps all the parens from
   the original source.  Something is happening in the renamer to remove the
   parens around visible type application, causing T12530 to fail, as the
   dumped splice decl is after the renamer.

   This needs to be fixed by keeping the parens, but I do not know where they
   are being removed.  I have amended the test to pass, by removing the parens
   in the expected output.

Test Plan: ./validate

Reviewers: goldfire, mpickering, simonpj, bgamari, austin

Reviewed By: simonpj, bgamari

Subscribers: simonpj, goldfire, thomie, mpickering

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

GHC Trac Issues: #3384

comment:18 Changed 6 months ago by alanz

Resolution: fixed
Status: patchclosed

comment:19 Changed 5 months ago by Tamar Christina <tamar@…>

In d88efb7/ghc:

Fix Pretty printer tests on Windows

Summary:
D2752 added some tests which escapes string literals. This means newlines are converted
before they get normalized by the IO functions. So on Windows \r\n would be in the output
while \n was expected.

Test Plan: make test -C testsuite/tests/printer

Reviewers: austin, bgamari, alanz

Reviewed By: alanz

Subscribers: thomie

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

GHC Trac Issues: #3384

comment:20 Changed 3 months ago by Alan Zimmerman <alan.zimm@…>

In 258c719/ghc:

TH-spliced class instances are pretty-printed incorrectly post-#3384

Summary:
The HsSyn prettyprinter tests patch 499e43824bda967546ebf95ee33ec1f84a114a7c
broke the pretty-printing of Template Haskell-spliced class instances.

Test Plan: ./validate

Reviewers: RyanGlScott, austin, goldfire, bgamari

Reviewed By: RyanGlScott, bgamari

Subscribers: thomie

Differential Revision: https://phabricator.haskell.org/D3043
Note: See TracTickets for help on using tickets.