Opened 4 years ago

Closed 12 months ago

#10859 closed bug (fixed)

Generated Eq instance associates && wrongly

Reported by: nomeata Owned by: simonpj
Priority: lowest Milestone: 8.6.1
Component: Compiler Version: 7.10.2
Keywords: deriving, newcomer Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: #10858 Differential Rev(s): Phab:D5104
Wiki Page:


Currently, we derive this code for (==):

      (ImportDecl a1_acXi
      (ImportDecl b1_acXq
      = (((((((((a1_acXi == b1_acXq)) && ((a2_acXj == b2_acXr)))
              && ((a3_acXk == b3_acXs)))
             && ((a4_acXl == b4_acXt)))
            && ((a5_acXm == b5_acXu)))
           && ((a6_acXn == b6_acXv)))
          && ((a7_acXo == b7_acXw)))
         && ((a8_acXp == b8_acXx)))

To me this looks wrongly associated: Since && is strict in the left but lazy in the right argument, shouldn’t this be associated the other way around? The compiler might clean it up later, but why not produce it correctly right away?

The culprit is this line:

          = foldl1 and_Expr (zipWith3Equal "nested_eq" nested_eq tys as bs)

and I was just about to change that ot foldr1, but then I did some git-archeology and found that Simon changed that deliberately from foldr1 to foldl1 in changeset:8de16184643ea3c2f9f30b5eaed18db6ef247760.

Simon, do you still recall what you were thinking when you applied this commit ... 18½years ago? :-)

Change History (7)

comment:1 Changed 4 years ago by nomeata

Priority: lowlowest

(I just verified that the resulting code after optimization is indeed the same. So this is purely cosmetic, and probably not worth the bother.)

comment:2 Changed 4 years ago by simonpj

I'm afraid I have no idea.

Remember that we are also (#10854) trying to generate HsExprs that have explicit HsParens when needed. So I'm all for this clean-up.

comment:3 Changed 16 months ago by RyanGlScott

Keywords: deriving newcomer added

comment:4 Changed 13 months ago by ckoparkar

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

comment:5 Changed 13 months ago by Krzysztof Gogolewski <krz.gogolewski@…>

In 2d953a6/ghc:

Fix #10859 by using foldr1 while deriving Eq instances

Previously, we were using foldl1 instead, which led to the derived
code to be wrongly associated.

Test Plan: ./validate

Reviewers: RyanGlScott, nomeata, simonpj, bgamari

Reviewed By: RyanGlScott, nomeata

Subscribers: rwbarton, carter

GHC Trac Issues: #10859

Differential Revision:

comment:6 Changed 13 months ago by monoidal

Milestone: 8.6.1
Status: patchmerge

If it's not too late, this can be merged to 8.6.

comment:7 Changed 12 months ago by bgamari

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