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

[John Millikin]

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.

On Wed, Jun 1, 2011 at 00:31, Yitzchak Gale <gale at sefer.org> wrote:
> I wrote:
>>> I noticed when looking at the IsString instance
>>> for Name: it can introduce crashes into a program if someone
>>> accidentally puts a '{' at the beginning of a Name string.
>
> Or accidentally omits the '}' in Clark notation.
>
> The way xml-types is now, it cannot be used in an
> environment where code is not allowed to introduce
> any additional risk of crashes. That is quite common in
> commercial development.
>
> John Millikin wrote:
>> If you are concerned that a developer may cause exceptions
>> by writing incorrect code, it would be better to use the Name
>> constructor directly.
>
> The only way to ensure that the IsString will not be used
> would be to remove the instance entirely from xml-types.
> In GHC there is no way to prevent an instance from being
> imported when you import any part of a module.
>
> But that would be a shame - I have been using
> xml-types extensively, and I don't think I have used
> the Name constructor directly even once. It's just so
> much neater to use the IsString instance.
>
> If you don't like my suggestion of what fromString
> should do when Clark notation fails to parse, that's OK,
> do something else. As long as it doesn't crash.
>
> Let me put it another way. The IsString class was
> introduced mainly for ByteString and Text; it's a way
> for other string implementations to hook into Haskell's
> built-in special syntax for string literals. I think your
> Name type does qualify though - it really is just a kind
> of string, with special support for XML namespaces.
> But people would be really, really surprised if their program
> crashed at runtime (possibly for a customer) just because
> of certain characters they included in a string literal.
> It is very important for any implementation of the fromString
> method to be total.
>
> Thanks,
> Yitz
>