Code from "Write Yourself a Scheme in 48 Hours" tutorial

I recently went through this Haskell tutorial.
I'd be interested in any thoughts or comments at all, in terms of improving the structure, order, haskell conventions, or that long, kind of ugly eval function.

``
{-# LANGUAGE ExistentialQuantification #-}

module SchemeParser (LispVal (..), LispError (..), readExpr, eval, runIOThrows, primitiveBindings, liftThrows) where

import Control.Monad
import Control.Monad.Error
import Data.Array
import Data.Char
import Data.Complex
import Data.IORef
import Data.Maybe
import Data.Ratio
import Numeric
import System.Environment
import System.IO
import Text.ParserCombinators.Parsec hiding (spaces1)

data LispVal = Atom String
| List [LispVal]
| DottedList [LispVal] LispVal
| Vector (Array Int LispVal)
| Number Integer
| Float Double
| Complex (Complex Double)
| Rational Rational
| String String
| Bool Bool
| Char Char
| PrimitiveFunc ([LispVal] -> ThrowsError LispVal)
| Func { params :: [String], vararg :: Maybe String,
body :: [LispVal], closure :: Env }
| IOFunc ([LispVal] -> IOThrowsError LispVal)
| Port Handle

instance Show LispVal where show = showVal
instance Eq LispVal where a == b = a eq` b
eq :: LispVal -> LispVal -> Bool
eq (Atom a) (Atom b) = a == b
eq (List a) (List b) = a == b
eq (DottedList a b)(DottedList c d) = a == c && b == d
eq (Vector a) (Vector b) = a==b
eq (Number a) (Number b) = a==b
eq (Float a) (Float b) = a==b
eq (Complex a) (Complex b) = a==b
eq (Rational a) (Rational b) = a==b
eq (String a) (String b) = a==b
eq (Bool a) (Bool b) = a==b
eq (Char a) (Char b) = a==b
eq _ _ = False

data LispError = NumArgs Integer [LispVal]
| TypeMismatch String LispVal
| Parser ParseError
| BadSpecialForm String LispVal

Bike shedding

to get the bike shedding out of the way: i prefer tibbe's style guide. in that spirit, have blank lines between instance declarations, use a newline after an instance where. and please stick to 80 char lines.

you might like to order your functions somehow. the order is imo pretty chaotic.

to the more substantial review.

General advice

Compile with -Wall (in cabal: ghc-options: -Wall), so that ghc tells you many things that are potentially bad.

There is also hlint, but that did not yield anything substantial here.

Avoid partial functions

Listen to advices regarding non-exhaustive pattern matches!

In particular don't use partial record accessors:

data LispVal = ...
             | Func { params  :: [String]
                    , vararg  :: Maybe String
                    , body    :: [LispVal]
                    , closure :: Env 
                    }

Here params :: LispVal -> [String] will fail for ever LispVal that is not a function. Better use a separate type and let the sum type simple. e.g.:

data LispVal = ...
             | Func LispFunc

data LispFunc = LispFunc { params :: ... }

Use meaningful names

Parser is a constructor for LispError. Better use ParseError. Especially because there is a type Parser (from parsec) as well.

Show and Read instances should be reasonable

They should be inverses to each other and should read like what you write in ghci. For your use case, better not use a Show instance at all but rename showVal to prettyLispVal or similar.

Get rid of meaningless type synonyms (ThrowError, IOThrowError)

They obscure what's going on. see e.g. readOrThrow

readOrThrow parser input = case parse parser "lisp" input of          
    Left err  -> throwError $ Parser err
    Right val -> return val

is the same as

readOrThrow parser input = left Parser $ parse parser "lisp"

(with left :: (a -> b) -> Either a c -> Either b c from Control.Arrow, but easily defined by itself).

Use Applicative style when parsing

e.g. parseVector reads so much better with

parseVector = listArr  (string "#(" *> sepBy parseExpr spaces1 <* char ')')
  where
    listArr l = Vector $ listArray (0, length l - 1) l

Avoid shadow bindings

e.g. in makeFunc

Supply type signatures for all toplevel functions

Use pure values and functions whenever possible

Is there a reason you use an IORef [(String, IORef LispVal)] instead of simply [(String, LispVal)] (or better a HashMap from unordered-containers)?

Don't unneccessarily use monadic functions. e.g. f = return $ ....

Try to reject invalid scheme programs before interpreting them

you can have a second pass over your AST after you parsed it to weed out invalid terms or enrich the AST to include conditionals, etc. that way, your eval function won't have to deal with too many things at once.

you might also like to look into advanced concepts like HOAS.

StackExchange Code Review Q#52500, answer score: 7