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:

Description

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

    (==)
      (ImportDecl a1_acXi
                  a2_acXj
                  a3_acXk
                  a4_acXl
                  a5_acXm
                  a6_acXn
                  a7_acXo
                  a8_acXp)
      (ImportDecl b1_acXq
                  b2_acXr
                  b3_acXs
                  b4_acXt
                  b5_acXu
                  b6_acXv
                  b7_acXw
                  b8_acXx)
      = (((((((((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

Summary:
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: https://phabricator.haskell.org/D5104

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.