Fix #10859 by using foldr1 while deriving Eq instances
ClosedPublic

Authored by ckoparkar on Aug 24 2018, 12:16 PM.

Details

Summary

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

Test Plan

./validate

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.
ckoparkar created this revision.Aug 24 2018, 12:16 PM
RyanGlScott accepted this revision.Aug 24 2018, 12:21 PM

Gotta love one-character bugfixes.

Assuming this validates, LGTM!

This revision is now accepted and ready to land.Aug 24 2018, 12:21 PM
nomeata added inline comments.Aug 24 2018, 12:22 PM
compiler/typecheck/TcGenDeriv.hs
219

Maybe phrase the comment in a way that it talks positively about the current code? (“Using foldr causes correctly associated cdoe”) Future readers of the comment don’t know much about the history.

ckoparkar marked an inline comment as done.Aug 24 2018, 12:34 PM
ckoparkar added inline comments.
compiler/typecheck/TcGenDeriv.hs
219

You're right, this does read better.

ckoparkar marked an inline comment as done.Aug 24 2018, 12:35 PM
nomeata requested changes to this revision.Aug 24 2018, 12:35 PM

Arg, Phab ate half my comments :-(

I am not sure about the test case. Without -dsuppress-uniques, the output will change a lot. And even with I am not sure of the merits of just dumping the whole output: It will still change as people make unrelated changes, and then the developer will have to approve the new output, likely without knowing what to look for.

A testcase that actually declares the properties we are looking for would be far more difficiult to write than this issue warrants.

So I am inclined to just not have a test case for this issue.

This revision now requires changes to proceed.Aug 24 2018, 12:35 PM

Oh right, I should've used -dsuppress-uniques. Would a comment in the test file help ? Otherwise, I'll update this patch to not include it.

A testcase that actually declares the properties we are looking for would be far more difficiult to write than this issue warrants.

Out of curiosity, how can I write such a test ? Anything particular that I should grep to find other tests that do something similar ?

Oh right, I should've used -dsuppress-uniques. Would a comment in the test file help ? Otherwise, I'll update this patch to not include it.

Yeah, comments in the test file are always a good idea (although it still requires the developer to look there – initially they only see the differences in the output)

A testcase that actually declares the properties we are looking for would be far more difficiult to write than this issue warrants.

Out of curiosity, how can I write such a test ? Anything particular that I should grep to find other tests that do something similar ?

You can grep for grep, some tests declare what to look for the in the dump. But in this example, I think it is hard to write a reliable grep command. One could use the GHC API to inspect the desugared Core, but for this issue at hand that’d be great overkill.

ckoparkar updated this revision to Diff 17791.Aug 24 2018, 3:42 PM

Get rid of the test

nomeata accepted this revision.Aug 24 2018, 3:46 PM

I hope I did not discourage you from writing test cases in the future :-)

This revision is now accepted and ready to land.Aug 24 2018, 3:46 PM

You can grep for grep, some tests declare what to look for the in the dump. But in this example, I think it is hard to write a reliable grep command.

Interesting. I had not come across those before. Good to know for future patches that might need it.

One could use the GHC API to inspect the desugared Core, but for this issue at hand that’d be great overkill.

Agreed.

I hope I did not discourage you from writing test cases in the future :-)

Not at all! Thank you for reviewing the patch :)

This revision was automatically updated to reflect the committed changes.