Fix unparseable pretty-printing of promoted data cons
ClosedPublic

Authored by aherrmann on May 28 2018, 3:55 PM.

Details

Summary

Previously we would print code which would not round-trip:

> :set -XDataKinds
> :set -XPolyKinds
> data Proxy k = Proxy
> _ :: Proxy '[ 'True ]
error:
  Found hole: _ :: Proxy '['True]
> _ :: Proxy '['True]
error:
    Invalid type signature: _ :: ...
    Should be of form <variable> :: <type>
Test Plan

Validate with T14343

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.
aherrmann created this revision.May 28 2018, 3:55 PM

Part of the issue is resolved. But there is a remaining issue where I could use some pointers, see line comment.

This is my first submission to GHC. Please let me know if I'm doing something wrong.

compiler/iface/IfaceType.hs
976

The missing white space in the type-signature in the "Found hole" line seems resolved.
However, there is a remaining issue about a missing white space in the lines "In the expression" and "In an equation" as illustrated by the block comment.
It seems there is some additional post-processing happening. It is unclear to me where that happens. Trying to grep for the phrases did not reveal where those lines are being generated.

tdammers requested changes to this revision.May 29 2018, 2:43 AM
tdammers added a subscriber: tdammers.

Marking as "request changes", because this is clearly WIP.

compiler/iface/IfaceType.hs
1041

It's probably best to keep this discussion in phab comments or the ticket, rather than the code itself. If you really do want to keep all this in the code itself, I suggest moving it into a [Note].

This revision now requires changes to proceed.May 29 2018, 2:43 AM

Thanks so much for doing this, @aherrmann!

compiler/iface/IfaceType.hs
1041

I believe this is to be expected. Certain error messages in GHC will print out the source code exactly as you wrote it, so that is indeed a separate code path entirely from this, which simply tries to print a prettified version of the AST (without the original formatting).

In fact, I suspect the way you have it right now is correct! (Or are there test cases that are failing because of this?)

aherrmann updated this revision to Diff 16576.May 30 2018, 4:05 AM
  • Make T14343 and T14343b pass
aherrmann marked 2 inline comments as done.May 30 2018, 4:23 AM
aherrmann added inline comments.
compiler/iface/IfaceType.hs
976

Lifting the discussion into a line-comment instead of in the code as requested by @tdammers :

Replacing id by e.g. (char '$' <>) in the default case, yields the following test-outcome

--- ./printer/T14343.run/T14343.stderr.normalised       2018-05-27 23:03:43.052757776 +0200
+++ ./printer/T14343.run/T14343.comp.stderr.normalised  2018-05-27 23:03:43.052757776 +0200
@@ -13,13 +13,13 @@
            (and originally defined in ‘GHC.Err’))

 T14343.hs:11:9:
-     Found hole: _ :: Proxy '[ '[1]]
+     Found hole: _ :: Proxy '[ '[$1]]
      In the expression: _ :: Proxy '[ '[1]]
       In an equation for ‘test2’: test2 = _ :: Proxy '[ '[1]]
      Relevant bindings include
-        test2 :: Proxy '[ '[1]] (bound at T14343.hs:11:1)
+        test2 :: Proxy '[ '[$1]] (bound at T14343.hs:11:1)
       Valid substitutions include
-        test2 :: Proxy '[ '[1]] (defined at T14343.hs:11:1)
+        test2 :: Proxy '[ '[$1]] (defined at T14343.hs:11:1)
         Proxy :: forall k1 (k2 :: k1). Proxy k2 (defined at T14343.hs:8:16)
         undefined :: forall a. GHC.Stack.Types.HasCallStack => a
           (imported from ‘Prelude’ at T14343.hs:1:1
*** unexpected failure for T14343(normal)

Unexpected results from:
TEST="T14343"

Note, how the lines "In the expression" and "In an equation" don't contain the dollar sign, and have the correct space between bracket and single quote. And how only test2 is mismatching. Without the extra dollar sign, we get

--- ./printer/T14343.run/T14343.stderr.normalised       2018-05-27 23:04:11.500081779 +0200
+++ ./printer/T14343.run/T14343.comp.stderr.normalised  2018-05-27 23:04:11.500081779 +0200
@@ -14,8 +14,8 @@

 T14343.hs:11:9:
      Found hole: _ :: Proxy '[ '[1]]
-     In the expression: _ :: Proxy '[ '[1]]
-      In an equation for ‘test2’: test2 = _ :: Proxy '[ '[1]]
+     In the expression: _ :: Proxy '['[1]]
+      In an equation for ‘test2’: test2 = _ :: Proxy '['[1]]
      Relevant bindings include
         test2 :: Proxy '[ '[1]] (bound at T14343.hs:11:1)
       Valid substitutions include
@@ -27,8 +27,8 @@

 T14343.hs:12:9:
      Found hole: _ :: Proxy '[ '("Symbol", 1)]
-     In the expression: _ :: Proxy '[ '("Symbol", 1)]
-      In an equation for ‘test3’: test3 = _ :: Proxy '[ '("Symbol", 1)]
+     In the expression: _ :: Proxy '['("Symbol", 1)]
+      In an equation for ‘test3’: test3 = _ :: Proxy '['("Symbol", 1)]
      Relevant bindings include
         test3 :: Proxy '[ '("Symbol", 1)] (bound at T14343.hs:12:1)
       Valid substitutions include
*** unexpected failure for T14343(normal)

Unexpected results from:
TEST="T14343"

Now the lines "In the expression" and "In an equation" lack the expected space between breacket and single quote. It seems that these lines are generated by separate code paths and maybe are subject to some form of post processing.

1041

Or are there test cases that are failing because of this?

Running make test I get 33 unexpected failures and 1 unexpected stat failure after these changes, and repeating the failing tests before these changes I get 33 unexpected failures.
I'm not sure what the unexpected stat failure is about:

Unexpected stat failures:
   perf/should_run/T14052.run  T14052 [stat not good enough] (ghci)

Apart from that it seems the answer is no.

testsuite/tests/printer/T14343.stderr
18

@RyanGlScott

In fact, I suspect the way you have it right now is correct!

In this form these new test-cases pass. Note, how the spaces are inconsistent between the type signatures in "Found hold", and "In the expression" and "In an equation".

RyanGlScott added inline comments.May 30 2018, 5:19 AM
compiler/iface/IfaceType.hs
1041

I get 33 unexpected failures.

OK. It's not clear from your description whether these changes are simply the results of slight error message fluctuations due to pretty-printer changes (which are to be expected) or something more serious. When you run those 33 failing tests, do the new results look OK? If so, run make accept TEST="<test1> <test2> ..." to have GHC's test suite accept the new changes.

bgamari requested changes to this revision.May 30 2018, 4:17 PM
bgamari retitled this revision from Fix bad pretty-printing (14343) to Fix bad pretty-printing.
bgamari edited the summary of this revision. (Show Details)
bgamari updated the Trac tickets for this revision.
bgamari retitled this revision from Fix bad pretty-printing to [WIP\ Fix bad pretty-printing.
bgamari retitled this revision from [WIP\ Fix bad pretty-printing to [WIP] Fix bad pretty-printing.

Do any of the existing tests' output change? If so it would be very helpful for review if you could commit the new output. If not feel free to request review again.

This revision now requires changes to proceed.May 30 2018, 4:18 PM
aherrmann updated this revision to Diff 16595.May 30 2018, 5:01 PM
aherrmann marked an inline comment as done.
  • make accept TEST="T15067 T12711"
bgamari requested changes to this revision.May 30 2018, 5:49 PM

Ahh, yes, this output looks much better.

The patch looks good to m; just one small request.

compiler/iface/IfaceType.hs
1149

I would suggest that we lift this out to the top-level to eliminate the duplication.

This revision now requires changes to proceed.May 30 2018, 5:49 PM
aherrmann updated this revision to Diff 16682.Jun 3 2018, 10:35 AM
  • Adapt T13035 and T9872b
  • Lift pprSpaceIfPromotedTyCon to top-level
aherrmann marked 4 inline comments as done.Jun 3 2018, 10:44 AM
aherrmann added inline comments.
compiler/iface/IfaceType.hs
1041

@RyanGlScott
Sorry, yes that was not very clear.

I found four test-cases that were affected by this change, and where the new results look okay. I've changed their expected outcome in this differential accordingly.

RyanGlScott accepted this revision.Jun 3 2018, 4:42 PM

This looks great. Thanks, @aherrmann!

bgamari accepted this revision.Jun 7 2018, 9:50 AM
bgamari retitled this revision from [WIP] Fix bad pretty-printing to Fix unparseable pretty-printing of promoted data cons.
bgamari edited the summary of this revision. (Show Details)
bgamari edited the summary of this revision. (Show Details)
bgamari edited the test plan for this revision. (Show Details)

Looks good. Thanks @aherrmann!

This revision was not accepted when it landed; it landed in state Needs Review.Jun 7 2018, 7:52 PM
This revision was automatically updated to reflect the committed changes.