Make ioToFsError OS specific

[kderme]

Since windows and unix sometimes throw different kind of Exceptions, on IntersectMBO/ouroboros-network#285 we made the equality check too lenient (always true). This can be improved. @edsko commented IntersectMBO/ouroboros-network#530 (comment):

It would be nice to be a little bit more precise here (as precise as we can be without making MockFS OS 
dependent I guess). But this can be done in a separate PR.

Concretely: let's replace FsIllegalOperation, FsResourceInappropriateType and FsInvalidArgument by a 
more general FsInvalidOperation, and then introduce a Posix and Windows specific ioToFsError function. 
Ideally we'd not bundle FsInsufficientPermissions in there though, so if you do try this and find some 
problems with that, let's think about the specific problems there.

[edsko]

Closed in IntersectMBO/ouroboros-network#557.

[edsko]

Uh, no, sorry, this isn't closed yet. We relaxed the tests so that we allow different errors in Windows and Posix; ideally though we'd translate both the same set of FsErrors so that we can write code against a single unified API.

[mrBliss]

Apparently, on darwin (macOS), we also get differing FsErrors.
The test failure below was reported by @kantp:

Up to date
ouroboros-storage
  Storage
    HasFS
      HasFS
        q-s-m:
Model {knownHandles = RefEnv []
      ,knownPaths = RefEnv []
      ,mockFS = MockFS {mockFiles =
                       Folder (Map.fromList [])
                       ,mockHandles = Map.fromList []
                       ,mockNextHandle = Handle 0}}

   == At (CreateDirIfMissing False (PExpPath ["z"])) ==> At (Resp {getResp = Right (Path (Reference (Concrete ["z"])) ())}) [ 0 ]

Model {knownPaths = RefEnv [+_×_
                               (Reference ["z"]) ["z"]]
      ,mockFS = MockFS {mockFiles =
                       Folder (Map.fromList [+_×_ "z"
                                                (Folder (Map.fromList []))])
                       ,mockHandles = ...}
      ,knownHandles = ...}

   == At (RemoveFile (PExpPath ["z"])) ==> At (Resp {getResp = Left (FsError {fsErrorType = FsInsufficientPermissions, fsErrorPath = ["z"], fsErrorString = "permission denied", fsErrorStack = [("ioToFsError",SrcLoc {srcLocPackage = "ouroboros-consensus-0.1.0.0-inplace", srcLocModule = "Ouroboros.Storage.FS.IO", srcLocFile = "src/Ouroboros/Storage/FS/IO.hs", srcLocStartLine = 86, srcLocStartCol = 12, srcLocEndLine = 86, srcLocEndCol = 32}),("handleError",SrcLoc {srcLocPackage = "ouroboros-consensus-0.1.0.0-inplace", srcLocModule = "Ouroboros.Storage.FS.IO", srcLocFile = "src/Ouroboros/Storage/FS/IO.hs", srcLocStartLine = 81, srcLocStartCol = 19, srcLocEndLine = 81, srcLocEndCol = 34}),("rethrowFsError",SrcLoc {srcLocPackage = "ouroboros-consensus-0.1.0.0-inplace", srcLocModule = "Ouroboros.Storage.FS.IO", srcLocFile = "src/Ouroboros/Storage/FS/IO.hs", srcLocStartLine = 65, srcLocStartCol = 27, srcLocEndLine = 65, srcLocEndCol = 44}),("removeFile",SrcLoc {srcLocPackage = "main", srcLocModule = "Test.Ouroboros.Storage.FS.StateMachine", srcLocFile = "test-storage/Test/Ouroboros/Storage/FS/StateMachine.hs", srcLocStartLine = 173, srcLocStartCol = 64, srcLocEndLine = 173, srcLocEndCol = 74})], fsLimitation = False})}) [ 0 ]

Model {knownHandles = RefEnv []
      ,knownPaths = RefEnv [_×_
                              (Reference ["z"]) ["z"]]
      ,mockFS = MockFS {mockFiles =
                       Folder (Map.fromList [_×_ "z"
                                               (Folder (Map.fromList []))])
                       ,mockHandles = Map.fromList []
                       ,mockNextHandle = Handle 0}}

FAIL (0.05s)
          *** Failed! Falsified (after 37 tests and 7 shrinks):
          Commands
            { unCommands =
                [ Command
                    (At (CreateDirIfMissing False (PExpPath [ "z" ])))
                    (At
                       Resp { getResp = Right (Path (Reference (Symbolic (Var 0))) ()) })
                    [ Var 0 ]
                , Command
                    (At (RemoveFile (PExpPath [ "z" ])))
                    (At
                       Resp
                         { getResp =
                             Left
                               FsError
                                 { fsErrorType = FsResourceInappropriateType
                                 , fsErrorPath = [ "z" ]
                                 , fsErrorString = "expected file"
                                 , fsErrorStack =
                                     [ ( "checkFsTree'"
                                       , SrcLoc
                                           { srcLocPackage = "ouroboros-consensus-0.1.0.0-inplace"
                                           , srcLocModule = "Ouroboros.Storage.FS.Sim.MockFS"
                                           , srcLocFile = "src/Ouroboros/Storage/FS/Sim/MockFS.hs"
                                           , srcLocStartLine = 402
                                           , srcLocStartCol = 12
                                           , srcLocEndLine = 402
                                           , srcLocEndCol = 31
                                           }
                                       )
                                     , ( "checkFsTree"
                                       , SrcLoc
                                           { srcLocPackage = "ouroboros-consensus-0.1.0.0-inplace"
                                           , srcLocModule = "Ouroboros.Storage.FS.Sim.MockFS"
                                           , srcLocFile = "src/Ouroboros/Storage/FS/Sim/MockFS.hs"
                                           , srcLocStartLine = 732
                                           , srcLocStartCol = 17
                                           , srcLocEndLine = 732
                                           , srcLocEndCol = 32
                                           }
                                       )
                                     , ( "removeFile"
                                       , SrcLoc
                                           { srcLocPackage = "ouroboros-consensus-0.1.0.0-inplace"
                                           , srcLocModule = "Ouroboros.Storage.FS.Sim.Pure"
                                           , srcLocFile = "src/Ouroboros/Storage/FS/Sim/Pure.hs"
                                           , srcLocStartLine = 51
                                           , srcLocStartCol = 34
                                           , srcLocEndLine = 51
                                           , srcLocEndCol = 68
                                           }
                                       )
                                     , ( "removeFile"
                                       , SrcLoc
                                           { srcLocPackage = "main"
                                           , srcLocModule = "Test.Ouroboros.Storage.FS.StateMachine"
                                           , srcLocFile =
                                               "test-storage/Test/Ouroboros/Storage/FS/StateMachine.hs"
                                           , srcLocStartLine = 173
                                           , srcLocStartCol = 64
                                           , srcLocEndLine = 173
                                           , srcLocEndCol = 74
                                           }
                                       )
                                     ]
                                 , fsLimitation = False
                                 }
                         })
                    []
                ]
            }
          Mount point: /var/folders/9g/9yvzdmgn26s0j54d7gpdzctw0000gn/T/cardano-s-m-547f92b3086be5fd/HasFS-9f0a022e6a4aee74
          PostconditionFailed "PredicateC (Resp {getResp = Left (FsError {fsErrorType = FsInsufficientPermissions, fsErrorPath = [\"z\"], fsErrorString = \"permission denied\", fsErrorStack = [(\"ioToFsError\",SrcLoc {srcLocPackage = \"ouroboros-consensus-0.1.0.0-inplace\", srcLocModule = \"Ouroboros.Storage.FS.IO\", srcLocFile = \"src/Ouroboros/Storage/FS/IO.hs\", srcLocStartLine = 86, srcLocStartCol = 12, srcLocEndLine = 86, srcLocEndCol = 32}),(\"handleError\",SrcLoc {srcLocPackage = \"ouroboros-consensus-0.1.0.0-inplace\", srcLocModule = \"Ouroboros.Storage.FS.IO\", srcLocFile = \"src/Ouroboros/Storage/FS/IO.hs\", srcLocStartLine = 81, srcLocStartCol = 19, srcLocEndLine = 81, srcLocEndCol = 34}),(\"rethrowFsError\",SrcLoc {srcLocPackage = \"ouroboros-consensus-0.1.0.0-inplace\", srcLocModule = \"Ouroboros.Storage.FS.IO\", srcLocFile = \"src/Ouroboros/Storage/FS/IO.hs\", srcLocStartLine = 65, srcLocStartCol = 27, srcLocEndLine = 65, srcLocEndCol = 44}),(\"removeFile\",SrcLoc {srcLocPackage = \"main\", srcLocModule = \"Test.Ouroboros.Storage.FS.StateMachine\", srcLocFile = \"test-storage/Test/Ouroboros/Storage/FS/StateMachine.hs\", srcLocStartLine = 173, srcLocStartCol = 64, srcLocEndLine = 173, srcLocEndCol = 74})], fsLimitation = False})} :/= Resp {getResp = Left (FsError {fsErrorType = FsResourceInappropriateType, fsErrorPath = [\"z\"], fsErrorString = \"expected file\", fsErrorStack = [(\"checkFsTree'\",SrcLoc {srcLocPackage = \"ouroboros-consensus-0.1.0.0-inplace\", srcLocModule = \"Ouroboros.Storage.FS.Sim.MockFS\", srcLocFile = \"src/Ouroboros/Storage/FS/Sim/MockFS.hs\", srcLocStartLine = 402, srcLocStartCol = 12, srcLocEndLine = 402, srcLocEndCol = 31}),(\"checkFsTree\",SrcLoc {srcLocPackage = \"ouroboros-consensus-0.1.0.0-inplace\", srcLocModule = \"Ouroboros.Storage.FS.Sim.MockFS\", srcLocFile = \"src/Ouroboros/Storage/FS/Sim/MockFS.hs\", srcLocStartLine = 732, srcLocStartCol = 17, srcLocEndLine = 732, srcLocEndCol = 32}),(\"removeFile\",SrcLoc {srcLocPackage = \"ouroboros-consensus-0.1.0.0-inplace\", srcLocModule = \"Ouroboros.Storage.FS.Sim.Pure\", srcLocFile = \"src/Ouroboros/Storage/FS/Sim/Pure.hs\", srcLocStartLine = 51, srcLocStartCol = 34, srcLocEndLine = 51, srcLocEndCol = 68}),(\"removeFile\",SrcLoc {srcLocPackage = \"main\", srcLocModule = \"Test.Ouroboros.Storage.FS.StateMachine\", srcLocFile = \"test-storage/Test/Ouroboros/Storage/FS/StateMachine.hs\", srcLocStartLine = 173, srcLocStartCol = 64, srcLocEndLine = 173, srcLocEndCol = 74})], fsLimitation = False})})" /= Ok
          Use --quickcheck-replay=29262 to reproduce.

[kderme]

In IntersectMBO/ouroboros-network#557 I had introduced an FsError equality check which is os specific. For example in windows we have

sameError :: FsError -> FsError -> Bool
sameError e1 e2 = fsErrorPath e1 == fsErrorPath e2
               && sameFsErrorType (fsErrorType e1) (fsErrorType e2)
  where
    sameFsErrorType ty1 ty2 = case (ty1, ty2) of
      (FsReachedEOF,           FsReachedEOF)           -> True
      (FsReachedEOF,           _)                      -> False
      (_,                      FsReachedEOF)           -> False
      (FsDeviceFull,           FsDeviceFull)           -> True
      (FsDeviceFull,           _)                      -> False
      (_,                      FsDeviceFull)           -> False
      (FsResourceAlreadyInUse, FsResourceAlreadyInUse) -> True
      (FsResourceAlreadyInUse, _)                      -> False
      (_,                      FsResourceAlreadyInUse) -> False
      (_,                      _)                      -> True

while in unix there is a stricter equality check.

Also the sameError function is used only in tests, while changing ioToFsError will change the error that the user sees and may confuse him.

So, maybe this can close given the alternative solution?

[mrBliss]

In IntersectMBO/ouroboros-network#557 I had introduced an FsError equality check which is os specific. For example in windows we have

[..]

while in unix there is a stricter equality check.

Also the sameError function is used only in tests, while changing ioToFsError will change the error that the user sees and may confuse him.

So, maybe this can close given the alternative solution?

No, because the custom sameError is just a workaround for now, not a real solution, as Edsko said in this same thread:

Uh, no, sorry, this isn't closed yet. We relaxed the tests so that we allow different errors in Windows and Posix; ideally though we'd translate both the same set of FsErrors so that we can write code against a single unified API.

[kderme]

Uh, no, sorry, this isn't closed yet. We relaxed the tests so that we allow different errors in Windows and Posix; ideally though we'd translate both the same set of FsErrors so that we can write code against a single unified API.

I missed this comment above, but the point remains. I think modifying the error that the os throws can be confusing to the user. The errors in MockFs are based on the errors that unix throws, so relaxing the equality checker for windows seems like a good solution to me. Are there (or will be) any places where we pattern match against the error thrown (other than the tests) so that we need a unified api?

[mrBliss]

Uh, no, sorry, this isn't closed yet. We relaxed the tests so that we allow different errors in Windows and Posix; ideally though we'd translate both the same set of FsErrors so that we can write code against a single unified API.

I missed this comment above, but the point remains. I think modifying the error that the os throws can be confusing to the user. The errors in MockFs are based on the errors that unix throws, so relaxing the equality checker for windows seems like a good solution to me. Are there (or will be) any places where we pattern matching against the error thrown (other than the tests) so that we need a unified api?

We don't pattern match against the thrown errors, no. That's why this isn't a high-priority issue.

We're not really modifying the thrown error, we're assigning the correct FsErrorType to the error, see the current implementation of ioToFsErrorType. The reason for it doing the complex pattern match is that we were previously getting a FsDeviceFull error type when running out of file descriptors 😕.

By doing a custom pattern match for Windows, the goal is to at least get an FsErrorTypes that matches the actual error. Ideally, we assign the same FsErrorType as on Unix, while we're at it. IMO, it is not a priority that we get exactly the same error on all platforms. That's why the custom sameError workaround is ok for now.

[jorisdral]

Closing this as this issue is now tracked in input-output-hk/fs-sim#45