WIP support sets (well, kinda of) #79

adinapoli · 2023-03-09T13:55:24Z

To be discussed. This introduces a {x} syntax for sets of type x, and serialises to CBOR using the IANA cbor spec.

adamgundry

Thanks @adinapoli, I think sets would be very useful to support more natively in api-tools. I have a few concerns about the CBOR-related details and I think we can do something simpler, but the general idea seems good to me.

adamgundry · 2023-03-09T16:22:08Z

src/Data/API/JSON.hs

+instance (Ord a, FromJSONWithErrs a) => FromJSONWithErrs (Set.Set a) where
+  parseJSONWithErrs v@(JS.Object kvs) = case jsonParseCborSet kvs of
+    Nothing -> failWith $ expectedSet v
+    Just xs -> fmap Set.fromList <$> traverse help $ zip (V.toList xs) [0..]
+    where
+      help (x, i) = stepInside (InElem i) $ parseJSONWithErrs x
+  parseJSONWithErrs JS.Null      = pure mempty
+  parseJSONWithErrs v            = failWith $ expectedArray v


I think the JSON representation of a set should simply be the list of its elements (insensitive to order, when decoding, and ordered when encoding).

adamgundry · 2023-03-09T16:24:20Z

src/Data/API/Parse.y

@@ -188,6 +190,7 @@ Type :: { APIType }
 Type
    : '?' Type                          { TyMaybe            $2             }
    | '[' Type ']'                      { TyList             $2             }
+    | '{' Type '}'                      { TySet              $2             }


What about using Set T as the syntax, rather than {T}? Similarly I think in retrospect Maybe T would have been better than ? T; we could even add that as an alternative now?

adamgundry · 2023-03-09T16:25:32Z

src/Data/API/Types.hs

@@ -296,6 +299,7 @@ instance NFData BasicType where
 -- | A default value for a field
 data DefaultValue
    = DefValList
+    | DefValSet


I'm not sure we need DefValSet with its own syntax, can we just use DefValList for both?

adamgundry · 2023-03-09T16:27:28Z

src/Data/API/Value.hs

+    TySet ty   -> case v of
+                    JS.Array arr -> Set . Set.fromDistinctAscList <$> traverse (parseJSON api ty) (V.toList arr)
+                    _            -> failWith (expectedArray v)


Why fromDistinctAscList? Is it clear we can assume the list is sorted here?

adamgundry · 2023-03-09T16:29:37Z

src/Data/Binary/Serialise/CBOR/Extra.hs

+-- | Cribbed from 'cardano-base' and 'cardano-sl', functions that I (adinapoli)
+-- wrote eons ago anyway.


We can't necessarily use these without satisfying the license conditions on the source packages (which probably means incorporating their licenses).

adamgundry · 2023-03-09T16:31:33Z

src/Data/Binary/Serialise/CBOR/Extra.hs

+--
+-- Encoding and decoding sets
+--
+
+encodeSetLikeWith :: (a -> Encoding) -> [a] -> Encoding
+encodeSetLikeWith f xs = encodeSetSkel f length foldr xs
+


I'd prefer not to complicate the CBOR encoding/decoding code unless absolutely necessary, and I don't think we need special support for CBOR sets. Let's just lists.

adamgundry · 2023-03-09T16:32:06Z

src/Data/API/Changes.hs

@@ -679,6 +681,10 @@ updateTypeAt' :: Map TypeName UpdateDeclPos
 updateTypeAt' upds alter (UpdateList upd)    v p = do
    xs <- expectList v p
    List <$!> mapM (\ (i, v') -> updateTypeAt' upds alter upd v' (InElem i : p)) (zip [0..] xs)
+updateTypeAt' upds alter (UpdateSet upd)    v p = do
+    xs <- expectSet v p
+    Set . Set.fromDistinctAscList <$!>


Assuming the elements are distinct and sorted looks a bit dangerous to me?

adamgundry · 2023-03-09T16:33:06Z

src/Data/API/Value.hs

@@ -372,6 +390,7 @@ arbitraryOfType :: NormAPI -> APIType -> QC.Gen Value
 arbitraryOfType api ty0 = case ty0 of
    TyName  tn -> arbitraryOfDecl api (lookupTyName api tn)
    TyList  ty -> List  <$> QC.listOf (arbitraryOfType api ty)
+    TySet   ty -> Set . Set.fromDistinctAscList <$> QC.listOf (arbitraryOfType api ty)


This is definitely not safe.

adamgundry · 2023-03-09T16:34:52Z

src/Data/API/Value.hs

@@ -246,6 +256,7 @@ decode :: NormAPI -> APIType -> CBOR.Decoder s Value
 decode api ty0 = case ty0 of
    TyName tn  -> decodeDecl api (lookupTyName api tn)
    TyList ty  -> List  <$!> decodeListWith (decode api ty)
+    TySet ty   -> Set . Set.fromDistinctAscList <$!> decodeSetLikeWith (decode api ty)


I guess this use of fromDistinctAscList is probably safe, but I'm wary...

adamgundry · 2023-03-09T16:38:37Z

src/Data/API/Value.hs

@@ -93,6 +95,7 @@ data Value = String  !T.Text
           | Bool    !Bool
           | Int     !Int
           | List    ![Value]
+           | Set     !(Set.Set Value)


We need to make sure that the CBOR encoding/decoding of the "generic representation" Value is consistent with the specific CBOR encoding/decoding instances generated by TH for particular types. Here's a worry: what if the Ord instance for the specific type used an ordering that isn't consistent with the ordering of the corresponding Values?

Maybe this is okay, provided the decoder doesn't assume that the values are sorted, because it can sort them when constructing the set in memory.

WIP support sets

8b97056

adamgundry reviewed Mar 9, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP support sets (well, kinda of) #79

WIP support sets (well, kinda of) #79

adinapoli commented Mar 9, 2023

adamgundry left a comment

adamgundry Mar 9, 2023

adamgundry Mar 9, 2023

adamgundry Mar 9, 2023

adamgundry Mar 9, 2023

adamgundry Mar 9, 2023

adamgundry Mar 9, 2023

adamgundry Mar 9, 2023

adamgundry Mar 9, 2023

adamgundry Mar 9, 2023

adamgundry Mar 9, 2023

		-- \| Cribbed from 'cardano-base' and 'cardano-sl', functions that I (adinapoli)
		-- wrote eons ago anyway.

WIP support sets (well, kinda of) #79

Are you sure you want to change the base?

WIP support sets (well, kinda of) #79

Conversation

adinapoli commented Mar 9, 2023

adamgundry 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment