Merge IOHK fork

[peterbecich]

https://github.com/eskimor/purescript-bridge/issues/63

• CodeGenSwitches no longer used
• Proxy no longer used
• Printer uses Leijen.Text instead of Text
• genericShow PureScript instance
  • I think the same as https://github.com/eskimor/purescript-bridge/pull/85
    • PR 85 has been replaced with IOHK's implementation
• some unit tests replaced with IOHK's tests
• all printer imports are combined into one function instanceToImportLines instead of being divided between two modules
• excellent RoundTrip tests implemented by IOHK
  • greatly improves test coverage for both Argonaut and json-helpers and exposes issues

TODO

• ~check that derived Generic instances are correct~
  • https://github.com/eskimor/purescript-bridge/issues/74#issuecomment-1498529423
• ~fix RoundTrip test~
• ~some other broken tests~

---

The PR supports https://github.com/input-output-hk/purescript-bridge-json-helpers and the existing library https://github.com/coot/purescript-argonaut-aeson-generic

The issues with purescript-argonaut-aeson-generic are documented in the readme. Importantly, these issues exist on master right now without the PR, so I think it should not block merging. Discussion https://github.com/purescript-contrib/purescript-argonaut-codecs/issues/115#issuecomment-1863200986

The other library https://github.com/paf31/purescript-foreign-generic continues to be supported, but the test coverage is much less than the other two.

[peterbecich]

Current error in RoundTrip test

cabal test --test-options="--match \"/writePSTypesWith/should produce aeson-compatible argonaut instances/\""

  3) writePSTypesWith should produce aeson-compatible argonaut instances
       Falsifiable (after 1 test):
         Either (Right (Just False))
       {"contents":{"Right":false},"tag":"Either"}
       expected: ""
        but got: "got {\"contents\":{\"Right\":false},\"tag\":\"Either\"}"

[peterbecich]

Encoding with argonaut-aeson-generic has issues. Can be demonstrated by running the example. The example generates a random payload. It is served to the client. If the client decodes the payload successfully, the client modifies some values and sends the payload back to the server.

• enter example directory
• spago bundle-app --to static/index.js
• cabal run example
• open http://localhost:8080/index.html in browser
• see browser console
• refresh the page. Sometimes the client will decode the payload successfully, sometimes not

Example success, from server:

Serving:
{
    "_fooBaz": {
        "_bazMessage": "hello"
    },
    "_fooList": [
        10,
        11,
        12,
        13,
        14,
        15,
        16,
        17,
        18,
        19,
        20
    ],
    "_fooMap": {
        "bar": 3,
        "baz": 3,
        "foo": 2
    },
    "_fooMessage": "Hello",
    "_fooNumber": 123,
    "_fooTestData": {
        "contents": {
            "contents": false,
            "tag": "Bool"
        },
        "tag": "Maybe"
    },
    "_fooTestSum": {
        "contents": -5.6,
        "tag": "Number"
    }
}
Received from client:
Foo message: Hola        Foo number: 124         Foo list length: 22     Foo Map length: 4

Example failure, from server:

Serving:
{
    "_fooBaz": {
        "_bazMessage": "hello"
    },
    "_fooList": [
        10,
        11,
        12,
        13,
        14,
        15,
        16,
        17,
        18,
        19,
        20
    ],
    "_fooMap": {
        "bar": 3,
        "baz": 3,
        "foo": 2
    },
    "_fooMessage": "Hello",
    "_fooNumber": 123,
    "_fooTestData": {
        "contents": {
            "Right": true
        },
        "tag": "Either"
    },
    "_fooTestSum": {
        "tag": "Nullary"
    }
}

Decoding error message on client:

Error decoding Foo: An error occurred while decoding a JSON value:
  Under 'When decoding a Foo':
  At object key '_fooTestData':
  Under 'When decoding a Either':
  Under 'Either':
  At object key 'tag':
  No value was found.

I don't know why this Either payload is missing its value.

[flip111]

@peterbecich can you clarify this?

Encoding with argonaut-aeson-generic has issues

Decoding error message on client

Is it both encoding and decoding that are problematic ?

The example generates a random payload.

---

EDIT: Most of what i wrote down below is nonsense because i was on the wrong branch. I will retest it

---

I don't find the code that generates a random payload. To me it seems the data is hardcoded. I get this output when i run the example

Serving Foo:
{
    "_fooBaz": {
        "_bazMessage": "hello"
    },
    "_fooList": [
        10,
        11,
        12,
        13,
        14,
        15,
        16,
        17,
        18,
        19,
        20
    ],
    "_fooMap": {
        "bar": 3,
        "baz": 3,
        "foo": 2
    },
    "_fooMessage": "Hello",
    "_fooNumber": 123
}
Foo message: Hola	 Foo number: 124	 Foo list length: 22	 Foo Map length: 4
Foo message: Hola	 Foo number: 124	 Foo list length: 22	 Foo Map length: 4
Foo message: Hola	 Foo number: 124	 Foo list length: 22	 Foo Map length: 4

---

In Example failure, from server: you have

    "_fooTestData": {
        "contents": {
            "Right": true
        },
        "tag": "Either"
    },

The error reads

At object key 'tag':
  No value was found.

You say I don't know why this Either payload is missing its value.

But under key tag there is actually a value. Makes me believe there was a mismatch between the posted JSON and the error you got.

[flip111]

When i edit your code

Types.hs

data Foo = Foo
  { _fooMessage :: Text
  , _fooNumber  :: Int
  , _fooList    :: [Int]
  , _fooMap     :: Map.Map Text Int
  , _fooBaz     :: Baz
  , _fooTestData :: Either String Bool
  }
  deriving (FromJSON, Generic, ToJSON)

MyLib.hs

foo :: Foo
foo = Foo
  (pack "Hello")
  123
  [10..20]
  (Map.fromList [(pack "foo", 2), (pack "bar", 3), (pack "baz", 3)])
  (Baz $ pack "hello")
  (Right True)

then i do get a missing Either tag, but it seems to be the server who doesn't send it rather than the client making a mistake of not decoding it properly.

Hello, Haskell!
Serving Foo:
{
    "_fooBaz": {
        "_bazMessage": "hello"
    },
    "_fooList": [
        10,
        11,
        12,
        13,
        14,
        15,
        16,
        17,
        18,
        19,
        20
    ],
    "_fooMap": {
        "bar": 3,
        "baz": 3,
        "foo": 2
    },
    "_fooMessage": "Hello",
    "_fooNumber": 123,
    "_fooTestData": {
        "Right": true
    }
}

[flip111]

running the example

cd example
spago bundle-app --to static/index.js
stack run # (implies latest LTS)

Visit http://localhost:8080/index.html

General remarks

• Would like to update stack/GHC to the latest LTS

[peterbecich]

@flip111 , thanks for taking a look at this.

Is it both encoding and decoding that are problematic ?

I think the only issue is server-side encoding

then i do get a missing Either tag, but it seems to be the server who doesn't send it rather than the client making a mistake of not decoding it properly.

agreed

I have updated the Stack project.

Two alternative ways to run the example:

• in project root, nix run .#example. Then http://localhost:8080/index.html should be up
• cd example , cabal run example, http://localhost:8080/index.html

Then refresh the page repeatedly. Success is when server log shows "Received from client:..." Failure is when there is no response from the client. Then I look at the JSON object the server sent out to try to understand the problem.

---

Another example JSON object served to the client which produces a client-side error. I assume the server-side encoding is incorrect somehow:

Serving:
{
    ...,
    "_fooMessage": "Hello",
    "_fooNumber": 123,
    "_fooTestData": {
        "contents": {
            "Right": false
        },
        "tag": "Either"
    },
    "_fooTestSum": {
        "contents": -11,
        "tag": "Int"
    }
}

[peterbecich]

The issue is on the Haskell side. Aeson encoding of Either is incompatible with Purescript library argonaut-aeson-generic. Example:

    "_fooE": {
        "Left": "foooo"
    },

The Purescript library expects to find a tag.

[peterbecich]

The Either has a tag now. Next issue is missing field value. The other datatypes decode fine with contents; why does Either need to have value?

Serving JSON:
{
    "_fooBaz": {
        "_bazMessage": "hello"
    },
    "_fooE": {
        "contents": 8,
        "tag": "Right"
    },
    "_fooList": [
        10,
        11,
        12,
        13
    ],
    "_fooMap": {
        "bar": 3,
        "baz": 3,
        "foo": 2
    },
    "_fooMessage": "Hello",
    "_fooNumber": 123,
    "_fooTestData": {
        "contents": "foooo",
        "tag": "TEither"
    },
    "_fooTestSum": {
        "contents": true,
        "tag": "Bool"
    }
}

Error decoding Foo: An error occurred while decoding a JSON value:
  Under 'When decoding a Foo':
  At object key '_fooE':
  Under 'Either':
  At object key 'value':
  No value was found.

[peterbecich]

@eskimor, @kindaro, @bentongxyz , please review

I have updated the summary: https://github.com/eskimor/purescript-bridge/pull/89#issue-1910635911

IOHK's RoundTrip tests are the best check on this PR in my opinion. This are run in CI. cabal test will run them.

There are issues with Argonaut discussed here: https://github.com/purescript-contrib/purescript-argonaut-codecs/pull/116

There are likely to be simple issues in this PR like dead code, lint, etc.

Additionally, there are comments about things remaining to fix. If the PR is merged these issues will be exposed to more people who may fix them. In my opinion these are not blockers because the issues exist already in the repository, and were just exposed by improved test coverage.

I don't think a new GitHub tag should be created, or new Hackage release, yet.

Thanks!

[flip111]

What are your thoughts of using an approach like codec-argonaut instead of relying on type classes? As it was discussed here https://github.com/purescript-contrib/purescript-argonaut-codecs/issues/113#issuecomment-1656845390

One fork as you mentioned https://github.com/purescript-contrib/purescript-argonaut-codecs/issues/115#issuecomment-1874884848 can solve the issues of incompatibility with aeson. But that still leaves additional types on the table like Ratio from the first comment.

[peterbecich]

@flip111 it sounds like a good idea to me. I think it is an addition to be done later in a different PR, an additional option to the three existing libraries.

[kindaro]

As I see it, there are two big risks right now:

1. The biggest risk is that this patch does not get merged and all the work is lost to bit rot — to counteract this, it is better to merge sooner, even if something is not working.
2. The other, smaller risk is that merging future patches from the IOHK fork and other improvements will be hard — to counteract this risk, I think the patch should match some basic non-functional requirements like automatic formatting.

review

design, architecture, overall approach

This patch is of significant size and I cannot say anything meaningful as to its design and architecture. It would have been ideal if a series of smaller patches or atomic commits equipped with clear commit messages were offered, with each clearly marked as a feature or a breaking change. But I understand that making one big patch is faster. What matters now is that it gets merged.

non-functional requirements

good

• The code builds.
• There are no warnings even with -Wall -Wextra.
• I did not spot any dead code.

bad

• There are 12 hints when running hlint -XNoPatternSynonyms src, 6 hints when running hlint example and 5 hints when running hlint test. In my ideal world, some of these hints are fixed and others are ignored by creating a file hlint.yaml.
• Formatting is not perfect. Running either ormolu or stylish-haskell does modify some files. In my ideal world, running ormolu --mode inplace **/*.hs && stylish-haskell --inplace **/*.hs has no effect because everything is already formatted this way. Also, it should be noted in documentation that ormolu should be used.
• I am unhappy about huge chunks of code commented out of Printer.hs. Do they serve any purpose? Can they simply be deleted?

functional requirements

good

• The working example shows that there is at least one way to use this library. So, we know this patch did not break the code altogether.

bad

• There are some faults exposed by the new test suite. This is fine — so far as the foundation is strong, we can repair the local faults eventually.

advice

My advice would be:

1. For @peterbecich to make hlint happy, remove code that is commented out (if it needs to be put back eventually, make an issue about it) and automatically format the whole code base. This should take no more than a few hours.
2. For @flip111 to merge and consider adding some GitHub action that checks formatting and linting. In my ideal world, there is an interactive rebase step, but it would easily take a day of work so I understand if this does not happen.
3. In my ideal world, Peter and @flip111 manage to write a change log that explains what new features are added and what breaking changes are made in this patch.

summary

Overall, this is great work. I shall be putting it into production as soon as my code base is upgraded to PureScript 0.15.

[flip111]

@kindaro thanks for the review. I care little for hlint and formatting, for me it's one of the lowest priorities. I understand the code review is difficult because of it's size, i thought the same. What would be good is to try a functional test. Take one or more of your own projects and make a new git branch if you will, then add peter's fork as a dependency. You can see here how to do that with stack. Then observe the differences with the current library. Did you have to refactor your code to make it work again with the new version? Is it missing functionality? Are you hitting edge cases? Can you do a full round trip of serializing and deserializing. If you have Arbitrary together with Aeson on your data structures it would be useful to generate cases, serialize them, de-serialize them and then check them for equality with quickcheck or hedgehog.

[kindaro]

I care little for hlint and formatting, for me it's one of the lowest priorities.

This is of course up to you. I can only say that for me tidy code is easier to justify contributing to and depending on. What matters most right now is that the patch gets merged, and if linting and formatting are not blockers for you — the better!

Take one or more of your own projects and make a new git branch if you will, then add peter's fork as a dependency.

This is exactly what I want to do, but unfortunately it is a bit complicated because the code I have right now does not work with the latest purescript-bridge and it will take significant work to get it to work. However, if something will not be working I am confident that I could fix it and send you a patch.

If you have Arbitrary together with Aeson on your data structures it would be useful to generate cases, serialize them, de-serialize them and then check them for equality with quickcheck or hedgehog.

This is also something I should like to do, but at a later point in time. You should not wait for me on this.

[peterbecich]

Thanks @kindaro , about formatting, eskimor/purescript-bridge is all Stylish Haskell; IOHK uses Ormolu. The repo originally had no consistent format chosen so I decided on Stylish Haskell and @eskimor agreed: https://github.com/eskimor/purescript-bridge/pull/76

There is a .stylish-haskell yaml. This should not produce any diffs:

find . -name '*.hs' | xargs stylish-haskell -i

I will respond to the other points later.

[kindaro]

eskimor/purescript-bridge is all Stylish Haskell; IOHK uses Ormolu.

Alright! I thought you eventually decided to format with ormolu to match the IOHK fork but I see now this is not so. By the way, formatting with fourmolu with the following settings and then with stylish-haskell makes a diff of 814 lines, compared with 4186 lines if ormolu is used, so we could make these settings standard if we wanted to have total formatting. Maybe it would help merge future changes from the IOHK fork?

indentation: 4
function-arrows: leading
comma-style: leading
import-export-style: leading
record-brace-space: true

This should not produce any diffs:

find . -name '*.hs' | xargs stylish-haskell -i

Alas:

% git status
On branch merge-iohk-3
Your branch is up to date with 'origin/merge-iohk-3'.

nothing to commit, working tree clean
% stylish-haskell --inplace **/*.hs
% git status
On branch merge-iohk-3
Your branch is up to date with 'origin/merge-iohk-3'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   src/Language/PureScript/Bridge/SumType.hs
	modified:   test/RoundTripArgonautAesonGeneric/Spec.hs
	modified:   test/RoundTripArgonautAesonGeneric/Types.hs
	modified:   test/RoundTripJsonHelpers/Spec.hs

no changes added to commit (use "git add" and/or "git commit -a")

[peterbecich]

 By the way, formatting with fourmolu with the following settings and then with stylish-haskell makes a diff of 814 lines, compared with 4186 lines if ormolu is used, so we could make these settings standard if we wanted to have total formatting. Maybe it would help merge future changes from the IOHK fork?

I did something like this here: https://github.com/eskimor/purescript-bridge/pull/89/commits/b5fbac0dbdfac82e230d46082a895c49bbb9aed7 The best results at reversing IOHK's Ormolu formatting were to re-format first with Fourmolu, then stylish-haskell. Directly formatting IOHK with stylish-haskell did not work as well.

I agree there could have been something missed in the IOHK fork which could be merged upstream later. The easiest way to proceed there i.m.o. is to format IOHK with Fourmolu, then stylish-haskell.

[kindaro]

Yes, thank you for putting my mind to the right spot. I was remembering that you formatted everything some time ago, but I was confused between ormolu and fourmolu. So, the code base has perfect formatting at commit b5fbac0dbdfac82e230d46082a895c49bbb9aed7. What if we were to rebase and format every subsequent commit in the same way? If this sounds like a lot of work, I have some scripts that can do it in 1 minute.

I come with better news though. I came around to try this patch on an industrial grade code base, and at once I found a fault.

review

functional requirements

bad

• This patch breaks the feature I added in #85. I see you dropped it in favour of code from the IOHK fork, but their code generates broken instances for types with phantom parameters. Here is an example:

  newtype ID a = ID {getID :: Int}
  

  The code form #85 generates this:

  newtype ID a =
      ID {
        getID :: Int
      }
  
  instance showID ∷ Show (ID a) where
  show value = genericShow value
  

  However, the code from this patch — presumably coming from IOHK — generates this:

  newtype ID a = ID { getID :: Int }
  
  instance (Show a) => Show (ID a) where
    show a = genericShow a
  

  Notice the spurious constraint Show a. This will break compilation of other types that depend on ID and want to have a Show of their own — they should not need an instance of Show for whichever phantom variable they instantiate ID with.

summary

Generating healthy instances when phantom types are at play is important for my use cases. This is certainly not something I could not fix, but maybe we can try not breaking it in the first place?

[kindaro]

By the way, the current master of purescript-bridge can handle data declarations with phantom type parameters, but not with non-phantom type parameters. The issue seems to be that the fake types like A that purescript-bridge instantiates type variables with do not have a Show instance of their own. I do not know exactly how this is supposed to work. It seems, however, that Eq and Ord for data declarations with non-phantom type parameters also do not work. Is this something this patch improves upon?

[peterbecich]

Excellent points, thanks @kindaro , I had not noticed these issues. Thinking about how to fix the instances.

About the formatting, I'd rather not go back and reformat these commits, despite the script making it easier. Should I squash my commits?

[flip111]

Should I squash my commits?

Let them be as tribute to your work :wink:

[kindaro]

@peterbecich   I pray for your good health.

[peterbecich]

I agree this should be fixed: https://github.com/eskimor/purescript-bridge/pull/89#issuecomment-1890994859 Here is the easy part of the change: https://github.com/peterbecich/purescript-bridge/commit/e663a3d31c18e9474e0da1818234e01894132dad However, this breaks other things which I haven't had time to look into.

The example now covers json-helpers; if you run example on merge-iohk-3, it works. If you run example on branch fix-show-on-iohk-fork, it will show the issues with this fix remaining to be solved. Would someone else complete this for me and make a PR to this PR?

[flip111]

Questions

• Why were switches removed?

Stuff that i noticed that i liked

• Added word8Bridge and word16Bridge
• Added Eq A

Problems

Should not derive Enum and Bounded when Ord is not specified and missing Show instance

haskell in

data WeekInMonth = WeekFirst | WeekSecond | WeekThird | WeekFourth | WeekLast
  deriving (Generic, Eq, Show)
  deriving TextShow via FromGeneric WeekInMonth
instance Aeson.ToJSON WeekInMonth where
  toEncoding = Aeson.genericToEncoding Aeson.defaultOptions
instance Aeson.FromJSON WeekInMonth

purescript out

data WeekInMonth
  = WeekFirst
  | WeekSecond
  | WeekThird
  | WeekFourth
  | WeekLast

derive instance Eq WeekInMonth

derive instance Generic WeekInMonth _

instance Enum WeekInMonth where
  succ = genericSucc
  pred = genericPred

instance Bounded WeekInMonth where
  bottom = genericBottom
  top = genericTop

Problem description

• Enum requires Ord but it is not available. Solution: should not generate Enum when Ord is not there. Possibly could give an info message that Enum was skipped
• Save as above but then for Bounded
• specified Show instance is missing
• specified Aeson instances missing

Previously code looked like this

data WeekInMonth =
    WeekFirst
  | WeekSecond
  | WeekThird
  | WeekFourth
  | WeekLast

derive instance eqWeekInMonth :: Eq WeekInMonth
instance showWeekInMonth :: Show WeekInMonth where
  show = genericShow
instance encodeJsonWeekInMonth :: EncodeJson WeekInMonth where
  encodeJson = genericEncodeAeson Argonaut.defaultOptions
instance decodeJsonWeekInMonth :: DecodeJson WeekInMonth where
  decodeJson = genericDecodeAeson Argonaut.defaultOptions
derive instance genericWeekInMonth :: Generic WeekInMonth _

[flip111]

@peterbecich do you have a list of open points that still need work?

[peterbecich]

@flip111 thank you for the detailed comment, I will read carefully later

One thing I need help with is fixing the generic Show instance. It was correctly implemented by @kindaro, then it was broken in my PR: https://github.com/eskimor/purescript-bridge/pull/89#issuecomment-1890994859 Here is how far I got with that: https://github.com/eskimor/purescript-bridge/pull/89#issuecomment-1913755047 You can use the example to check it.

[peterbecich]

Thank you @flip111 : https://github.com/peterbecich/purescript-bridge/pull/1

[peterbecich]

TODO Enum and Ord https://github.com/peterbecich/purescript-bridge/pull/1#issue-2137164432

[flip111]

@peterbecich did you look at the problem of Enum and Ord? Can you confirm or otherwise comment whether the findings here are correct https://github.com/eskimor/purescript-bridge/pull/89#issuecomment-1934932243 ?

[peterbecich]

@flip111 , in progress, thanks