Dhall-haskell: Make type of `extract` richer.

[sellout]

Currently, if we have an error anywhere in our many Interpret instances, we get the same message. I propose that

extract :: Expr Src X -> Maybe a

be changed to something like

extract :: Expr Src X -> Validation (NonEmpty InterpretError) a

in order to get more precision in the messages. I’m not yet sure exactly what InterpretError looks like, but I imagine it would have cases like UnexpectedExpression and UnsupportedExpression for handling things like

nonEmptyTy :: Type a -> Type (NonEmpty a)
nonEmptyTy Type{..} =
  Type
  (\case
      e@(D.ListLit _ elems) ->
        maybe (Failure . pure $ UnsupportedExpression e D.List "list must not be empty")
              Success
              (nonEmpty . toList =<< traverse extract elems)
      e -> Failure . pure $ UnexpectedExpression e D.List)
  D.List

If this seems acceptable, I could take a swipe at it.

[ocharles]

I've also been thinking that we should some how maintain the 'path' we're validating - a bit like how aeson can see exactly where in a JSON document the parse failed

[sellout]

@ocharles Agreed. I’d be happy with either my suggestion or yours (or both 😄).

[Gabriel439]

Yeah, if it's a simple change. Keep in mind that if a Type is implemented correctly then extract should never fail, so I wouldn't go overboard in improving the error message here

[sellout]

@Gabriel439 Yeah, the problem is finding programming bugs.

[joneshf]

An example of hitting this issue: I had a type that used to be a record:

{ executables :
    List { executableName : Text, executableDirectory : Text }
, manifest :
    < Cabal : {} | Hpack : {} >
, name :
    Text
, sourceDirectory :
    Text
, tests :
    List { suite : Text, testDirectory : Text }
, version :
    Text
}

Using auto and the following Haskell module:

module Shake.Package where

data Executable
  = Executable
    { executableName      :: String
    , executableDirectory :: FilePath
    }
  deriving (Generic)

instance Interpret Executable

data Manifest
  = Cabal
  | Hpack
  deriving (Generic)

instance Interpret Manifest

data Package
  = Package
    { executables     :: [Executable]
    , manifest        :: Manifest
    , name            :: String
    , sourceDirectory :: FilePath
    , tests           :: [Test]
    , version         :: String
    }
  deriving (Generic)

instance Interpret Package

data Test
  = Test
    { suite         :: String
    , testDirectory :: FilePath
    }
  deriving (Generic)

instance Interpret Test

I changed it to be a union:

< Haskell :
    { executables :
        List { executableName : Text, executableDirectory : Text }
    , manifest :
        < Cabal : {} | Hpack : {} >
    , name :
        Text
    , sourceDirectory :
        Text
    , tests :
        List { suite : Text, testDirectory : Text }
    , version :
        Text
    }
| JavaScript :
    { bin :
        List { file : Text, name : Text }
    , name :
        Text
    , sourceDirectory :
        Text
    }
>

Using the following Haskell modules:

module Shake.Package where

import qualified "this" Shake.Package.Haskell
import qualified "this" Shake.Package.JavaScript

data Package
  = Haskell Shake.Package.Haskell.Package
  | JavaScript Shake.Package.JavaScript.Package

packageType :: Type Package
packageType = union (haskell <> javaScript)
  where
  haskell :: UnionType Package
  haskell = constructor (pack "Haskell") $ fmap Haskell auto
  javaScript :: UnionType Package
  javaScript =
    constructor
      (pack "JavaScript")
      (fmap JavaScript Shake.Package.JavaScript.packageType)

module Shake.Package.Haskell where

data Executable
  = Executable
    { executableName      :: String
    , executableDirectory :: FilePath
    }
  deriving (Generic)

instance Interpret Executable

data Manifest
  = Cabal
  | Hpack
  deriving (Generic)

instance Interpret Manifest

data Package
  = Package
    { executables     :: [Executable]
    , manifest        :: Manifest
    , name            :: String
    , sourceDirectory :: FilePath
    , tests           :: [Test]
    , version         :: String
    }
  deriving (Generic)

instance Interpret Package

data Test
  = Test
    { suite         :: String
    , testDirectory :: FilePath
    }
  deriving (Generic)

instance Interpret Test

module Shake.Package.JavaScript where

data Bin
  = Bin
    { file :: FilePath
    , name :: String
    }

data Package
  = Package
    { bin             :: [Bin]
    , name            :: String
    , sourceDirectory :: FilePath
    }

binType :: Type Bin
binType = record $ do
  file <- field (pack "file") string
  name <- field (pack "name") string
  pure Bin { file, name }

packageType :: Type Package
packageType = record $ do
  bin <- field (pack "bin") (list binType)
  name <- field (pack "name") string
  sourceDirectory <- field (pack "sourceDirectory") string
  pure Package { bin, name, sourceDirectory }

This is the error I get:

Shakefile.hs: Error: Invalid Dhall.Type                                                  

Every Type must provide an extract function that succeeds if an expression      
matches the expected type.  You provided a Type that disobeys this contract     

The Type provided has the expected dhall type:                                  

↳ < Haskell :
      { executables :
          List { executableName : Text, executableDirectory : Text }
  ==================================================================
          Text
      }
  >

and it couldn't extract a value from the well-typed expression:                 

↳ < JavaScript =
      { bin =
          [ { file = "src/index.js", name = "build-browserify" } ]
  ================================================================
          Text
      }
  >

i know I'm doing something wrong, but I can't figure out what it is.

[joneshf]

For the sake of tying everything together, my issues should be fixed when #857 is released. Which is to say, no need to try and diagnose this specific problem. The example is still relevant for the amount of information provided in the error message.

[divarvel]

I'm working on it (with @ocharles' help) at Zurihac

[ocharles]

Fixed by #1011