make Delayed type abstract

[jvoigtlaender]

@owestphal, wouldn't it be better to hide the data constructor of the newtype thus?

I've also been thinking about further replacing

newtype Delayed a = Delayed {unDelayed :: String} deriving (Eq, Typeable, Generic)

by

newtype Delayed a = Delayed String deriving (Eq, Typeable, Generic)

instance Show (Delayed a) where
  show (Delayed str) = str

and

    Left err -> reject $ case parse (fully tokenSequence) "" (unDelayed ans) of

by

    Left err -> reject $ case parse (fully tokenSequence) "" (show ans) of

Is there a need somewhere to really have the default derived Show instance for Delayed, i.e., with "Delayed " being output?

[jvoigtlaender]

For symmetry with parse, maybe the order of arguments of parseDelayed should be flipped?

[jvoigtlaender]

Is there even a need for a Show instance for Delayed now?

[owestphal]

At the moment it is used for the Hashable (Delayed a) instance in Autotool. At the "expense" of a hashable dependency the instance could be defined here instead (making it also non-orphan).

[jvoigtlaender]

But why is Hashable (Delayed a) even needed? I guess I am still struggling to get to grips with the role of Delayed. There shouldn't be a task type that has Delayed as its input type, nor inside a configuration or instance type, right? That is the whole point of having things like

type CnfString = String
instance Partial TruthTableMaxterm MaxInst CnfString where

in Autotool, not

instance Partial TruthTableMaxterm MaxInst (Delayed Cnf) where

right?

[owestphal]

You are right. It is not needed. I transformed the actual string input too early in a few cases where completeGrade is used inside of toReporterWithGlobalPics which requires Hashable inputs.

[jvoigtlaender]

Does something similar apply to the Eq, Typeable, Generic instances?

Due to the peculiar role of Delayed, the pull request's intention really was to "isolate" it somewhat from the rest of the code via making it as abstract as possible. Any provided type class instance of course makes it a bit less abstract, since one can then learn something about internals of a delayed value.

[owestphal]

Should the signature of parseDelayedRaw :: Parser b -> Delayed a -> Either ParseError b also be restricted to achieve better isolation? With the current signature, the internal string is fully recoverable.

The obvious Parser () -> Delayed a -> Either ParseError () might be too restrictive though. E.g., when we want to count the number of parentheses (see #126).

[jvoigtlaender]

I'd say that parseDelayedRaw as a name is good enough in order to convey that one is dealing with internals if one uses it.

After all, the point of the abstraction is not really to guard against "malicious actors" here, but against "accidental misuse". If someone calls parse...Raw, like unsafe..., they have it coming. 😄