Json parser, incorrectly parsing string as a number

Question

I'm still pretty new to Haskell and functional programming in general, so I'm writing a small program with Parsec to parse JSON and pretty print it as a means of learning basic concepts. This is what I have so far:

import Text.Parsec
import Text.Parsec.String

data JValue = JString String
            | JNumber Double
            | JBool Bool
            | JNull
            | JObject [(String, JValue)]
            | JArray [JValue]
              deriving (Eq, Ord, Show)

parseJString, parseJNumber, parseJBool, parseJNull :: Parser JValue
parseJString = do
    str <- between (char '"') (char '"') (many (noneOf "\""))
    return . JString $ str

parseJNumber = do
    num <- many digit
    return . JNumber . read $ num

parseJBool = do
    val <- string "true" <|> string "false"
    case val of
        "true"  -> return (JBool True)
        "false" -> return (JBool False)

parseJNull = string "null" >> return JNull

parseJValue :: Parser JValue
parseJValue =   parseJString 
            <|> parseJNumber 
            <|> parseJBool 
            <|> parseJNull

For now, I'm assuming that the numbers are integers. Individually, parseJString, parseJNumber, parseJBool, and parseJNull work as expected in ghci. Additionally, parseJValue correctly parses strings and numbers.

ghci> parse parseJString "test" "\"test input\""
Right (JString "test input")
ghci> parse parseJNumber "test" "345"
Right (JNumber 345.0)
ghci> parse parseJBool "test" "true"
Right (JBool True)
ghci> parse parseJNull "test" "null"
Right JNull
ghci> parse parseJValue "test" "\"jvalue test\""
Right (JString "jvalue test")
ghci> parse parseJValue "test" "789"
Right (JNumber 789.0)

parseJValue fails, however, when I try to parse true, false, or null, and I get an interesting error.

ghci> parse parseJValue "test" "true"
Right (JNumber *** Exception: Prelude.read: no parse

I get a successful parse, but the parse returns a JNumber followed by an error stating that Prelude.read failed. I feel like I'm missing some core concept in building my parsers, but I can't see where I've gone wrong. Also, am I making any beginner mistakes with my code, i.e. would any of this be considered "bad" haskell?

Answer 1

The problem is the usage of many in parseJNumber. It is also a valid parse, when no character of the following string is consumed ("many p applies the parser p zero or more times. [...]"). What you need is many1:

parseJNumber = do
  num <- many1 (oneOf "0123456789")
  return $ JNumber (read num :: Double)

Edit:

Somehow, I think your combination of (.) and ($) looks kind of weird. I use (.) when I can get rid of a function parameter (like in the usage of (>>=)) and ($) when I'm to lazy to write parentheses. In your function parseJString you do not need (.) in order to get the right binding precedences. (I did the same transformation in the code above.)

parseJString = do
  str <- between (char '"') (char '"') (many (noneOf "\""))
  return $ JString str

Additionally, you could eliminate code-repetition by refactoring parseJBool:

parseJBool = do
  val <- string "true" <|> string "false"
  return (case val of
    "true"  -> JBool True
    "false" -> JBool False)

I would even rewrite the case-construct into a (total) local function:

parseJBool = (string "true" <|> string "false") >>= return . toJBool
 where
  -- there are only two possible strings to pattern match
  toJBool "true" = JBool True
  toJBool _      = JBool False

Last but not least, you can easily transform your other functions to use (>>=) instead of do-blocks.

-- additionally, you do not need an extra type signature for `read`
-- the constructor `JNumber` already infers the correct type
parseJNumber =
  many1 (oneOf "0123456789") >>= return . JNumber . read

parseJString =
  between (char '"') (char '"') (many (noneOf "\"")) >>= return . JString