File type to track the content and direction of files #5105

newhoggy · 2023-04-15T16:08:09Z

This is an alternate design to #5017

newhoggy · 2023-04-15T16:40:58Z

cardano-api/src/Cardano/Api/IO.hs

+-- contain and whether it is to be used for reading or writing.
+newtype File content (direction :: FileDirection) = File
+  { unFile :: FilePath
+  } deriving newtype (Eq, Ord, Read, Show, IsString, FromJSON, ToJSON)


The File type has an additional content type parameter.

newhoggy · 2023-04-16T01:53:30Z

bench/tx-generator/src/Cardano/TxGenerator/Setup/NixService.hs

@@ -70,7 +71,7 @@ instance AdjustFilePaths NixServiceOptions where
  adjustFilePaths f opts
    = opts {
      _nix_nodeConfigFile = f <$> _nix_nodeConfigFile opts
-    , _nix_sigKey = SigningKeyFile . f . unSigningKeyFile $ _nix_sigKey opts
+    , _nix_sigKey = mapFile f $ _nix_sigKey opts


mapFile works on any File regardless of file type.

newhoggy · 2023-04-16T02:07:18Z

cardano-cli/src/Cardano/CLI/Types.hs

-  deriving newtype (IsString, Show)
+data OfSigningKey
+
+type SigningKeyFile = File OfSigningKey


Instead of being a newtype with its own set of instances we will use a type alias to File with the file type content already supplied. This means it's possible to use file operation functions like writeByteStringFile directly because they accept any content.

To avoid a proliferation of type level tags we can reuse the types in cardano-api and cardano-cli with something like the following:

data Any type SigningKeyFile = File (SigningKey Any) type VerificationKeyFile = File (VerificationKey Any) type TxBodyFile = File (TxBody Any) type TxFile = File (Tx Any)

Let me know what you think

newhoggy · 2023-04-16T02:09:44Z

cardano-cli/test/Test/Golden/Byron/SigningKeys.hs

@@ -90,7 +89,7 @@ prop_migrate_legacy_to_nonlegacy_signingkeys =
     ]

    eSignKey <- liftIO . runExceptT . readByronSigningKey NonLegacyByronKeyFormat
-                  $ SigningKeyFile nonLegacyKeyFp
+                  $ File nonLegacyKeyFp


We can use File and Haskell will infer content = OfSigningKey.

Alternatively we can use type applications to be more specific: File @OfSigningKey.

newhoggy · 2023-04-16T02:16:33Z

cardano-node/src/Cardano/Node/Protocol/Shelley.hs

@@ -181,9 +181,9 @@ readLeaderCredentialsSingleton ProtocolFilepaths {shelleyKESFile = Nothing} =
     left KESKeyNotSpecified

 opCertKesKeyCheck
-  :: FilePath
+  :: File () In


We can use () for when we previously used FilePath.

This means we're not tracking the content type of the file yet. We can add this later.

newhoggy · 2023-04-16T02:26:54Z

cardano-api/src/Cardano/Api/IO.hs

 writeByteStringFile fp bs = runExceptT $
-  handleIOExceptT (FileIOError fp) $ BS.writeFile fp bs
+  handleIOExceptT (FileIOError (unFile fp)) $ BS.writeFile (unFile fp) bs


unFile is an escape hatch to get the FilePath within. We use it to implement file operations like writeByteStringFile.

newhoggy · 2023-04-16T02:27:31Z

cardano-api/src/Cardano/Api/IO.hs

+  => MonadIO m
+  => File content Out
+  -> ByteString
+  -> m (Either (FileError ()) ())


Functions can work on all files by leaving content unspecified.

newhoggy · 2023-04-16T02:30:17Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Genesis.hs

-   coldSK = SigningKeyFile $ dir </> "delegate" ++ strIndex ++ ".skey"
-   opCertCtr = OpCertCounterFile $ dir </> "delegate" ++ strIndex ++ ".counter"
+   kesVK = File @OfVerificationKey $ dir </> "delegate" ++ strIndex ++ ".kes.vkey"
+   coldSK = File @OfSigningKey $ dir </> "delegate" ++ strIndex ++ ".skey"


coldSK is a signing key file that is both written to and read from in the same command. It is therefore annotated as InOut. This type is inferred, but can be made explicit with type application of @InOut.

newhoggy · 2023-04-16T02:30:49Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Genesis.hs

-        (VerificationKeyFilePath kesVK)
-        coldSK
+        (VerificationKeyFilePath (onlyIn kesVK))
+        (onlyIn coldSK)


To use an InOut file for an In operation, we can cast it. This works on all file types regardless of content type.

newhoggy · 2023-04-16T02:30:57Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Genesis.hs

-        coldSK
-        opCertCtr
+        (File @OfVerificationKey $ dir </> "delegate" ++ strIndex ++ ".vkey")
+        (onlyOut coldSK)


To use an InOut file for an Out operation, we can cast it. This works on all file types regardless of content type.

newhoggy · 2023-04-16T02:32:56Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs

@@ -1279,7 +1279,7 @@ runQueryLeadershipSchedule (AnyConsensusModeParams cModeParams) network

      case mJsonOutputFile of
        Nothing -> liftIO $ printLeadershipScheduleAsText schedule eInfo (SystemStart $ sgSystemStart shelleyGenesis)
-        Just (OutputFile jsonOutputFile) ->
+        Just (File jsonOutputFile) ->


There are parts of the code where we still unwrap the FilePath and use that directly. All these cases can be converted to use the type-safe functions like writeLazyByteString instead. It isn't done here to keep this PR as small as possible.

newhoggy · 2023-04-16T02:35:27Z

cardano-cli/src/Cardano/CLI/Shelley/Parsers.hs

    pByronKeyFile =
          (ASigningKeyFile      <$> pByronSigningKeyFile)
      <|> (AVerificationKeyFile <$> pByronVerificationKeyFile)

-    pByronSigningKeyFile :: Parser SigningKeyFile
+    pByronSigningKeyFile :: Parser (SigningKeyFile In)


We can now be explicit in the types for parsers to say this is meant for input only. If we use it for Out or for InOut or for the wrong content type, we get a type error.

carbolymer · 2023-04-17T09:09:28Z

cardano-api/src/Cardano/Api/IO.hs

+
+-- | A file path with additional type information to indicate what the file is meant to
+-- contain and whether it is to be used for reading or writing.
+newtype File content (direction :: FileDirection) = File


direction could also be represented as a type-level list [FileDirection] . This way functions requiring file for writing for example could have a following signature:

writeByteStringOutput :: () => MonadIO m => Out `CouldBeAnyOf` directions => Maybe (File content directions) -> ByteString -> m (Either (FileError ()) ())

this way conversion of InOut to In or Out would be not necessary and onlyOut functions wouldn't be needed.

I like this idea, but it can be a separate PR.

carbolymer · 2023-04-17T09:20:40Z

I think I'm missing the value of having content as a type parameter for File . It's not used as a type check for decoding data from files. What are the benefits of having it?

Jimbo4350

LGTM but just one consideration: #5105 (comment)

Jimbo4350 · 2023-04-17T14:05:46Z

cardano-cli/src/Cardano/CLI/Types.hs

-  deriving newtype (IsString, Show)
+data OfSigningKey
+
+type SigningKeyFile = File OfSigningKey


To avoid a proliferation of type level tags we can reuse the types in cardano-api and cardano-cli with something like the following:

data Any type SigningKeyFile = File (SigningKey Any) type VerificationKeyFile = File (VerificationKey Any) type TxBodyFile = File (TxBody Any) type TxFile = File (Tx Any)

Let me know what you think

newhoggy · 2023-04-18T03:32:56Z

I've used this instead:

type SigningKeyFile = File (SigningKey ())
type VerificationKeyFile = File (VerificationKey ())
type TxBodyFile = File (TxBody ())
type TxFile = File (Tx ())

newhoggy · 2023-04-18T03:37:37Z

I think I'm missing the value of having content as a type parameter for File . It's not used as a type check for decoding data from files. What are the benefits of having it?

The same is true of the newtype system it replaces.

The benefits are the same also. If a function takes two different file types you can't accidentally put them in the wrong order.

Having a content type argument means we don't need to have an extra newtype for each file type we want to support and not having them means we don't have to worry about adding new type classes and instances.

There's lots of examples of File () Out still and we can replace more of the () as we go.

deepfire

Thank you @newhoggy !

newhoggy marked this pull request as ready for review April 15, 2023 16:08

newhoggy requested review from deepfire, jutaro, mgmeier, fmaste, cleverca22, dcoutts, Jimbo4350, erikd, JaredCorduan and CarlosLopezDeLara as code owners April 15, 2023 16:08

newhoggy force-pushed the newhoggy/file-type-2 branch 4 times, most recently from 89986e4 to 730c07e Compare April 15, 2023 16:37

newhoggy commented Apr 15, 2023

View reviewed changes

newhoggy force-pushed the newhoggy/file-type-2 branch from 46ee037 to 6b12259 Compare April 16, 2023 01:02

newhoggy commented Apr 16, 2023

View reviewed changes

newhoggy force-pushed the newhoggy/file-type-2 branch 2 times, most recently from 70f40bc to 1937111 Compare April 16, 2023 04:13

newhoggy force-pushed the newhoggy/file-type-2 branch from 1937111 to 75ec1bc Compare April 16, 2023 13:10

carbolymer reviewed Apr 17, 2023

View reviewed changes

Jimbo4350 approved these changes Apr 17, 2023

View reviewed changes

newhoggy force-pushed the newhoggy/file-type-2 branch 4 times, most recently from e1e75c9 to 6a46d9a Compare April 18, 2023 03:32

newhoggy enabled auto-merge April 18, 2023 07:06

carbolymer approved these changes Apr 18, 2023

View reviewed changes

deepfire approved these changes Apr 18, 2023

View reviewed changes

newhoggy added 5 commits April 19, 2023 07:34

Rename FileDirection to ParserFileDirection

804697c

New File type that tracks content and direction

1b9b9c9

Use new File type to track content and direction

48793ba

Tidy up the parsers that were touched

f5b4251

Use existing types instead of "Of" types.

20a31ec

newhoggy force-pushed the newhoggy/file-type-2 branch from 6a46d9a to 20a31ec Compare April 18, 2023 22:07

newhoggy added this pull request to the merge queue Apr 18, 2023

Merged via the queue into master with commit 26685ca Apr 18, 2023

iohk-bors bot deleted the newhoggy/file-type-2 branch April 18, 2023 23:24

newhoggy mentioned this pull request Apr 19, 2023

New File type that tracks whether the file is an input or output file or both #5017

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File type to track the content and direction of files #5105

File type to track the content and direction of files #5105

newhoggy commented Apr 15, 2023 •

edited

Loading

newhoggy Apr 15, 2023

newhoggy Apr 16, 2023

newhoggy Apr 16, 2023

Jimbo4350 Apr 17, 2023 •

edited

Loading

newhoggy Apr 16, 2023

newhoggy Apr 16, 2023

newhoggy Apr 16, 2023

newhoggy Apr 16, 2023 •

edited

Loading

newhoggy Apr 16, 2023

newhoggy Apr 16, 2023

newhoggy Apr 16, 2023

newhoggy Apr 16, 2023

newhoggy Apr 16, 2023

carbolymer Apr 17, 2023 •

edited

Loading

newhoggy Apr 18, 2023

carbolymer commented Apr 17, 2023

Jimbo4350 left a comment •

edited

Loading

Jimbo4350 Apr 17, 2023 •

edited

Loading

newhoggy commented Apr 18, 2023

newhoggy commented Apr 18, 2023 •

edited

Loading

deepfire left a comment

File type to track the content and direction of files #5105

File type to track the content and direction of files #5105

Conversation

newhoggy commented Apr 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 Apr 17, 2023 • edited Loading

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

newhoggy Apr 16, 2023 • edited Loading

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

carbolymer Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbolymer commented Apr 17, 2023

Jimbo4350 left a comment • edited Loading

Choose a reason for hiding this comment

Jimbo4350 Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

newhoggy commented Apr 18, 2023

newhoggy commented Apr 18, 2023 • edited Loading

deepfire left a comment

Choose a reason for hiding this comment

newhoggy commented Apr 15, 2023 •

edited

Loading

Jimbo4350 Apr 17, 2023 •

edited

Loading

newhoggy Apr 16, 2023 •

edited

Loading

carbolymer Apr 17, 2023 •

edited

Loading

Jimbo4350 left a comment •

edited

Loading

Jimbo4350 Apr 17, 2023 •

edited

Loading

newhoggy commented Apr 18, 2023 •

edited

Loading