[CDEC-385] For types with `deriveSimpleBi` instances, remove partial field accessors from sum types and convert to `deriveIndexedBi` #3153

mhuesch · 2018-06-28T05:11:25Z

Description

For types with deriveSimpleBi instances, remove partial field accessors from sum types and convert to deriveIndexedBi.

I also modify deriveSimpleBi to error out on sum-types where constructors have more than 0 fields. This is to discourage partial field accessors.

The module Test.Pos.Binary.Helpers.GoldenRoundTrip must be added to cardano-sl-binary's test-suite, because we cannot import cardano-sl-binary-test from the testsuite. So some dependencies are added.

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CDEC-385

Type of change

[~] 🐞 Bug fix (non-breaking change which fixes an issue)
[~] 🛠 New feature (non-breaking change which adds functionality)
[~] ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
[~] 🔨 New or improved tests for existing code
[~] ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

I have read the style guide document, and my code follows the code style of this project.
If my code deals with exceptions, it follows the guidelines.
I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.

Named fields in sum types result in accessors that are partial functions. I remove those from 3 types, and then switch their derived `Bi` instances to use `deriveIndexedBi`, which doesn't require named fields.

Class.TH We now error out when `deriveSimpleBi` encounteres named fields in a sumtype. Modifying `deriveSimpleBi` in this way causes some modules to break; this commit also modifies or eliminates those breakages. BiSerialize Because `deriveSimpleBi` no longer supports sum types, the comparison translation done previously gives us weaker guarantees (it only compares structs). In light of that, the most effective tests seem to be golden tests for checking that the old behavior of `deriveSimpleBi` is kept aligned with the future behavior of `deriveIndexedBi`.

mhuesch · 2018-06-28T05:12:37Z

binary/src/Pos/Binary/Class/TH.hs

@@ -368,6 +368,12 @@ deriveSimpleBiInternal predsMB headTy constrs = do
                when (length realFields /= length dcFields) $ templateHaskellError $
                    sformat ("Some field of "%shown
                    %" constructor doesn't have an explicit name") cName
+                when (length constrs > 1 && not (null realFields)) $


If it's a sum type, and any of the constructors take values, it could have partial field accessors.

Cool! Doing this was a good idea 👍

mhuesch · 2018-06-28T05:13:02Z

binary/test/Test/Pos/Binary/BiSerialize.hs

+        Field [| 0 :: TestIndexed |],
+        Field [| 1 :: TestIndexed |]
+        ]
+    ]


This is the same as before, it was just moved, and git can't tell that.

mhuesch · 2018-06-28T05:14:22Z

binary/test/Test/Pos/Binary/BiSerialize.hs

-tests :: IO Bool
-tests =
-  H.checkParallel $$discover
+-}


We could delete this block comment. I think it gives context for anyone wondering about the transition from deriveSimpleBi to deriveIndexedBi` in the future. But it could also be left in git history.

Given what we're testing here and how the tests were generated, I like the idea of leaving this in (it'd be especially helpful if I wanted to write another golden test for some reason).

intricate

LGTM

intricate · 2018-06-29T00:30:31Z

binary/test/Test/Pos/Binary/BiSerialize.hs

-tests :: IO Bool
-tests =
-  H.checkParallel $$discover
+-}


Given what we're testing here and how the tests were generated, I like the idea of leaving this in (it'd be especially helpful if I wanted to write another golden test for some reason).

intricate · 2018-06-29T00:37:18Z

binary/src/Pos/Binary/Class/TH.hs

@@ -368,6 +368,12 @@ deriveSimpleBiInternal predsMB headTy constrs = do
                when (length realFields /= length dcFields) $ templateHaskellError $
                    sformat ("Some field of "%shown
                    %" constructor doesn't have an explicit name") cName
+                when (length constrs > 1 && not (null realFields)) $


Cool! Doing this was a good idea 👍

erikd

Just need a a mention of deriveIndexedBi in that error message and a comment on its implementation.

When thats done, please feel free to dismiss this review and merge it.

Thanks!

erikd · 2018-06-29T01:27:50Z

binary/src/Pos/Binary/Class/TH.hs

+                        sformat ("`deriveSimpleBi` no longer supports sum types \
+                                 \with named fields.\n Constructor "%shown%" has \
+                                 \named fields: "%shown%".")
+                        cName (map fst realFields)


Maybe a mention of deriveIndexedBi in that error message so people don't have to go looking?

As simple as "Use derivedIndexedBiinstead. Probably a good idea to add a comment explainingderiveIndexedBi` as well.

Here's what the error message will look like

/Users/mhueschen/Projects/IOHK-work/cardano-sl/binary/test/Test/Pos/Binary/Cbor/CborSpec.hs:70:1: error: `deriveSimpleBi` no longer supports sum types with named fields. Constructor Test.Pos.Binary.Cbor.CborSpec.Login has named fields: [Test.Pos.Binary.Cbor.CborSpec.login,Test.Pos.Binary.Cbor.CborSpec.age]. Please use `deriveIndexedBi` instead.

erikd · 2018-06-29T01:28:19Z

binary/test/Test/Pos/Binary/BiSerialize.hs

+-- Since `deriveSimpleBi` no longer works on sum types, we cannot do a simple
+-- comparison property between `deriveSimpleBi` and `deriveIndexedBi`. Instead,
+-- this module contains golden tests. The tests were generated by the following
+-- process (done before sumtypes were disallowed from `deriveSimpleBi`):


- Spruce up example derivations for `deriveSimpleBi` and `deriveIndexedBi`. - Document `deriveIndexedBi` & the motivation behind it.

I've fixed the requested change.

Michael Hueschen added 2 commits June 27, 2018 21:57

[CDEC-385] Remove named fields from core/ sum-types

6012aca

Named fields in sum types result in accessors that are partial functions. I remove those from 3 types, and then switch their derived `Bi` instances to use `deriveIndexedBi`, which doesn't require named fields.

mhuesch requested review from adinapoli-iohk, erikd and parsonsmatt as code owners June 28, 2018 05:11

mhuesch commented Jun 28, 2018

View reviewed changes

mhuesch self-assigned this Jun 28, 2018

mhuesch mentioned this pull request Jun 28, 2018

Remove field accessors from SscProof #3152

Closed

11 tasks

mhuesch requested review from Jimbo4350 and intricate June 28, 2018 18:34

intricate approved these changes Jun 29, 2018

View reviewed changes

erikd previously requested changes Jun 29, 2018

View reviewed changes

[CDEC-385] deriveSimpleBi: update error message/docs

5ee09bc

- Spruce up example derivations for `deriveSimpleBi` and `deriveIndexedBi`. - Document `deriveIndexedBi` & the motivation behind it.

Jimbo4350 mentioned this pull request Jun 29, 2018

Remove field accessors for SscPayload data type #2902

Closed

11 tasks

mhuesch merged commit e7b1f1d into develop Jun 29, 2018

mhuesch deleted the mhuesch/CDEC-385 branch June 29, 2018 16:23

Jimbo4350 mentioned this pull request Jul 13, 2018

Remove field accessors from SscPayload #3139

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CDEC-385] For types with `deriveSimpleBi` instances, remove partial field accessors from sum types and convert to `deriveIndexedBi` #3153

[CDEC-385] For types with `deriveSimpleBi` instances, remove partial field accessors from sum types and convert to `deriveIndexedBi` #3153

mhuesch commented Jun 28, 2018

mhuesch Jun 28, 2018

intricate Jun 29, 2018

mhuesch Jun 28, 2018

mhuesch Jun 28, 2018

intricate Jun 29, 2018

intricate left a comment

intricate Jun 29, 2018

intricate Jun 29, 2018

erikd left a comment

erikd Jun 29, 2018

mhuesch Jun 29, 2018

erikd Jun 29, 2018

[CDEC-385] For types with deriveSimpleBi instances, remove partial field accessors from sum types and convert to deriveIndexedBi #3153

[CDEC-385] For types with deriveSimpleBi instances, remove partial field accessors from sum types and convert to deriveIndexedBi #3153

Conversation

mhuesch commented Jun 28, 2018

Description

Linked issue

Type of change

Developer checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intricate left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[CDEC-385] For types with `deriveSimpleBi` instances, remove partial field accessors from sum types and convert to `deriveIndexedBi` #3153

[CDEC-385] For types with `deriveSimpleBi` instances, remove partial field accessors from sum types and convert to `deriveIndexedBi` #3153