Opened 11 years ago

Closed 4 years ago

#2530 closed bug (fixed)

deriving Show adds extra parens for constructor with record syntax

Reported by: spl Owned by: bgamari
Priority: highest Milestone: 8.0.1
Component: Compiler Version: 6.8.3
Keywords: Cc: leather@…, ndmitchell@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D669, Phab:D2027
Wiki Page:

Description (last modified by bgamari)

Since record construction binds more tightly than function application, no parentheses are needed when passing a constructor with record syntax to a function constructor. As a result, we can pass Just A {x = 5} as shown below.

module Main where

data A = A {x :: Int} deriving (Show)

main :: IO ()
main = print $ Just A {x = 5}

But we get the result in the GHCi session below:

*Main> main
Just (A {x = 5})

I tried looking through TcGenDeriv, but didn't figure out quite where the parenifying was done. nested_compose_Expr? show_thingies?

I just wanted to make a ticket for general knowledge. Of course, this is not a real bug. Perhaps you could reclassify it as a feature. Either way, it would be nice to have.

Attachments (1)

T2530.hs (102 bytes) - added by morabbin 7 years ago.

Download all attachments as: .zip

Change History (42)

comment:1 Changed 11 years ago by simonpj

difficulty: Unknown

True. But I hate the fact that record construction binds more tightly than application. There's no harm in the extra parens, and I think they make it easier to read. So can we close this?

Simon

comment:2 Changed 11 years ago by NeilMitchell

Hugs does the opposite, which is a bit of a shame, as it means Hugs and GHC versions of a program do not produce the same output. Perhaps the Haskell' report should legislate on the "suggested" printing of a record.

comment:3 Changed 11 years ago by spl

I agree with Neil in that it is a shame it isn't standardized. I found it, because I'm writing a generic version of Show, and I'd like my output to be equivalent to GHC's for an added comfort level of testing.

I'm fine with being overruled. As I said, it is simply a "nice-to-have."

comment:4 Changed 11 years ago by igloo

I agree with Simon (and I think it's a bug in the language that you don't need parens there).

I also agree with Neil, that we should have consistency between implementations, and that the report should say what the output should be.

comment:5 Changed 11 years ago by igloo

Milestone: 6.10 branch

comment:6 Changed 11 years ago by simonpj

Milestone: 6.10 branch6.12 branch

I'm not sure what the resolution is here.

  • Hugs and GHC produce different output, but both are legal.
  • Which output you like is something of a matter of taste. My own taste prefers GHC's output.
  • If there was a strong lobby in favour of matching output, then we could change GHC or change Hugs.

This isn't a terribly big deal, and the easiest thing to do is, well, nothing. So if you want a change, can you lead a discussion that converges on a particular course of action? Meanwhile, I'll move this off the 6.10 branch. But if the discussion converges quickly we can easily put it in 6.10.

Simon

comment:7 Changed 11 years ago by NeilMitchell

Cc: ndmitchell@… added

I do often use x{...} as a single term without brackets, but if I couldn't that wouldn't be too much of as hassle. Given that printing should produce unambiguous terms (both for the compiler and the human reader), adding the brackets seems a sensible choice.

I am strongly in favour of Hugs and GHC agreeing on this, and indeed on everything. As soon as they don't, we're going to be in the situation where peoples programs behave differently, where test cases need two sets of "matching output" etc. As a regular user of both systems, the current compatibility is wonderful, but perfect compatibility (for a subset of Haskell) would be better.

comment:8 Changed 10 years ago by spl

Architecture: x86Unknown/Multiple
Operating System: MacOS XUnknown/Multiple
Type of failure: None/Unknown

In the spirit of BugSweep, this bug should probably be closed easily. The two options proposed are:

  1. Do nothing. This suits SPJ's style.
  2. Change by removing parentheses. This agrees with Hugs and NM.

I think it would be nice to do 2, but I don't feel that strongly about it. There do not seem to be many interested parties in this issue, and it is quite minor. Perhaps the default action should be to do nothing

comment:9 Changed 10 years ago by igloo

I'm interested in the report and all the implementations agreeing on what the answer is. I think that the discussion to determine what that answer should be would be better held on a mailing list, and we can resolve this ticket once that is done.

comment:10 Changed 10 years ago by spl

For reference, there have been a few responses on this Café thread.

comment:11 Changed 10 years ago by igloo

Milestone: 6.12 branch6.12.3

comment:12 Changed 9 years ago by igloo

Milestone: 6.12.36.14.1
Priority: normallow

comment:13 Changed 9 years ago by igloo

http://hackage.haskell.org/trac/haskell-prime/wiki/StricterLabelledFieldSyntax was rejected, so removing the parens is probably the right thing to do.

comment:14 Changed 9 years ago by igloo

Milestone: 7.0.17.0.2

comment:15 Changed 9 years ago by igloo

Milestone: 7.0.27.2.1

comment:16 Changed 8 years ago by igloo

Milestone: 7.2.17.4.1

comment:17 Changed 8 years ago by igloo

Milestone: 7.4.17.6.1
Priority: lowlowest

comment:18 Changed 7 years ago by igloo

Milestone: 7.6.17.6.2

comment:19 Changed 7 years ago by morabbin

I don't think that the fact that the GHC variant wasn't voted suprem justifies making what is really just a cosmetic change. The case for change is pretty weak, since the GHC variant (with parens) is not at all ambiguous (clarity is next to godliness, etc,), and is semantically equivalent to the Hugs/Haskell 98 variant. Close as wontfix, I say!

Changed 7 years ago by morabbin

Attachment: T2530.hs added

comment:20 Changed 7 years ago by igloo

Currently we do give the wrong answer, though, even if it's unlikely that anyone would care for anything other than making testcases.

comment:21 Changed 5 years ago by thoughtpolice

Milestone: 7.6.27.10.1

Moving to 7.10.1.

comment:22 Changed 5 years ago by thoughtpolice

Milestone: 7.10.17.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:23 Changed 5 years ago by thoughtpolice

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:24 in reply to:  13 Changed 5 years ago by thomie

Differential Rev(s): Phab:D669
Status: newpatch

The 2010 report, section 11.4, mentions: "The result of show is a syntactically correct Haskell expression ... Parenthesis are only added where needed ..."

comment:25 Changed 5 years ago by Austin Seipp <austin@…>

In 5692643c9d17e746327588cd6157a923642b7975/ghc:

Show record construction/update without parens

Summary:
The 2010 report mentions:
"The result of `show` is a syntactically correct Haskell expression ...
Parenthesis are only added where needed, //ignoring associativity//".

Reviewers: austin

Reviewed By: austin

Subscribers: thomie

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

GHC Trac Issues: #2530

comment:26 Changed 5 years ago by thoughtpolice

Resolution: fixed
Status: patchclosed

Merged, thanks!

comment:27 Changed 4 years ago by nomeata

JFTR if you come across this bug and are worried: A quick test shows that read . show still works fine, even if read and show come from different versions of GHC, in all combinations.

comment:28 Changed 4 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

comment:29 Changed 4 years ago by hvr

Fyi, this broke a few testsuites of mine based on golden-tests based on the Show-serialisation. Not sure yet how I can fix this other than using a duplicated set of golden files with the old and new Show instance serialisation...

comment:30 Changed 4 years ago by hvr

Resolution: fixed
Status: closednew

Btw, this also affects how GHCi displays results:

GHCi, version 8.0.0.20160204: http://www.haskell.org/ghc/  :? for help
Loaded GHCi configuration from /home/hvr/.ghci

λ:1> data T = C { f1, f2 :: () } deriving Show
data T = C {f1 :: (), f2 :: ()}
f1 :: T -> ()
f2 :: T -> ()

λ:2> Just (C () ())
Just C {f1 = (), f2 = ()}
it :: Maybe T

λ:3> 

comment:31 Changed 4 years ago by hvr

Owner: set to bgamari
Priority: lowesthighest

Temporarily setting this to high prio and assigning to Ben for judgement call

comment:32 Changed 4 years ago by hvr

Fwiw, Changing GHCi's output after so many years is possibly disrupting to literature, as GHCi's output would look different from examples printed in books and tutorials.

comment:33 Changed 4 years ago by RyanGlScott

Well, if people find this objectionable, I suppose we also need to review #10104, since that also resulted in a change in Show output.

comment:34 Changed 4 years ago by rwbarton

I assume hvr is joking, but you can never be sure...

comment:35 in reply to:  33 Changed 4 years ago by hvr

Replying to RyanGlScott:

Well, if people find this objectionable, I suppose we also need to review #10104, since that also resulted in a change in Show output.

There's a big difference though: #2530 wants to fix a deliberate deviation (i.e. many people here consider {} binding tighter than function application to be a design-flaw) from the Haskell Report. Those additional brackets are harmless and don't harm the expectation of 'Show' resulting in an expression that parses back to the original.

On the other hand, #10104 covers an area outside the Haskell Report (-XMagicHash) and more importantly fixes the actual problem that show (MkT 3#) results in MkT 3 which can't be parsed back to the original due to a kind-error.

comment:36 Changed 4 years ago by bgamari

I would be fine with retaining the old behavior, keeping the parens, for now. It's pretty widely believed that the binding of record syntax specified in the report was a mistake. Let's allow the Haskell Prime committee to discuss this matter before we commit to changing Show. While it's almost certainly not practical to change the binding of record syntax, the next report could specify a more lax semantics for Show.

To be clear, I'm merely requesting that the committee add this matter to their list of matters to address; their final decision is of course in their hands. If there is a good chance that the committee will consider such a change, then let's revert the commit in comment:25 for now and reconsider after the committee has made their decision.

If it's unlikely that the next report will loosen Show, then we should decide now whether we want retain and document the old behavior as an infelicity of GHC's implementation, or whether we want to come into compliance with the Report. I'm slightly leaning towards the former for the sake of stability, although I'm happy to hear arguments in the other direction.

comment:37 Changed 4 years ago by RyanGlScott

I don't have particularly strong feelings on this matter, and I'd be okay with reverting this for now, since multiple GHC devs have expressed their displeasure in this change.

This change does minimize the number of parentheses outputted in a way that agrees with the Haskell Report, but as Edward Kmett notes in a Haskell Café thread, we're already not using a minimal amount of parentheses as it is due to Show not taking fixity into account when parenthesizing (e.g., you can omit parentheses when chaining several infixl constructors). I wouldn't feel terrible about adding extra parentheses in this particular case.

comment:38 Changed 4 years ago by bgamari

Description: modified (diff)
Differential Rev(s): Phab:D669Phab:D669, Phab:D2027
Status: newpatch

Phab:D2027 reverts to the previous behavior including redundant parens as described in comment:36.

comment:39 Changed 4 years ago by Ben Gamari <ben@…>

In 1448f8ab/ghc:

Show: Restore redundant parentheses around records

As discussed in #2530 we are going to continue to produce parentheses
here in order to preserve compatibility with previous GHC releases. It
was found that dropped parentheses would break some testsuites which
compared against output from Show. This has been documented in the users
guide.

This reverts commit 5692643c9d17e746327588cd6157a923642b7975.

Test Plan: Validate

Reviewers: hvr, austin

Subscribers: thomie

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

GHC Trac Issues: #2350

comment:40 Changed 4 years ago by bgamari

Status: patchmerge

comment:41 Changed 4 years ago by bgamari

Resolution: fixed
Status: mergeclosed
Note: See TracTickets for help on using tickets.