[web-devel] xml-types IsString instance for Name causes crashes

[Michael Snoyman]

On Wed, Jun 1, 2011 at 7:04 PM, John Millikin <jmillikin at gmail.com> wrote:
> Well, first, there's no additional risk of crashes. The code won't
> ever *crash*, it will merely throw an exception, which can be caught
> by the standard exception handling computations. If your application
> requires high availability, you should be catching exceptions anyway,
> since they can arise from circumstances outside of the author's
> control.
>
> Second, GHC's IsString feature is not enabled by default. It requires
> the OverloadedStrings language extension to be enabled, which requires
> either an explicit pragma or a command-line flag. There is no risk in
> merely defining the instance, because users must opt in to use it.
>
> Third, the IsString instance is largely for convenience. It allows
> something like this code:
>
> ---------------------------------------------------------------
> name = case parseClark "{urn:foo}bar" of
>    Just (ns, ln) -> Name ln ns Nothing
>    Nothing -> error "string literal is incorrect"
> ---------------------------------------------------------------
>
> to be shortened to simply:
>
> ---------------------------------------------------------------
> name = "{urn:foo}bar"
> ---------------------------------------------------------------
>
> String literals are inherently untyped. There is no way to verify at
> compile-time that they are correct. IMO, it's better for the code to
> fail obviously (by throwing an exception) than quietly (by accepting
> the bad Name and doing something with it). Thus, I disagree that
> IsString instances should be total -- IMO, they should always throw an
> exception on invalid input, else it becomes difficult to notice typos
> without extensive testing.
>
> If you are truly concerned about your developers introducting errors
> via incorrect string literals, I have three suggestions:
>
> 1) Use the Name constructor explicitly. That's what it's for. You can
> guarantee that the contents of the various fields are exactly as you
> want them.
>
> 2) If you don't care about the prefix, you can write a simple
> constructor function:
>
> ---------------------------------------------------------------
> name :: String -> String -> Name
> name ns ln = Name (Data.Text.pack ln) (Just (Data.Text.pack ns)) Nothing
> ---------------------------------------------------------------
>
> 3) Since the exception is *always* thrown when the literal is invalid,
> even simple tests of the new functionality will discover the problem.
> You could even require the names themselves be tested, though that
> might be overkill.

Option 4* if it's *really* necessary: introduce a quasiquoter that
checks for correctness at compile time. It would also improve
performance (marginally) since the parsing would be taken care at
compile time.

Michael

* Just because I said it doesn't mean I endorse it. I'm quite happy
with IsString.