Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored the error handling #474

Merged
merged 2 commits into from
Jun 16, 2016
Merged

Refactored the error handling #474

merged 2 commits into from
Jun 16, 2016

Conversation

EliasC
Copy link
Contributor

@EliasC EliasC commented Jun 3, 2016

This commit reimplements the error handling of encorec. Before, an error
carried the message that should be displayed, meaning that the more
detailed a message you wanted, the more bloated the error handling code
got. The new model introduces a datatype Error which has one
constructor for each kind of error, and showing this value prints the
error message. For example, if there is a type mismatch error, instead
of writing

tcError $ "Expected type '" ++ show ty1 ++ "' but found '" ++ show ty2 ++ "'"

you now write

tcError $ TypeMismatchError ty1 ty2

The hope is that this will encourage writing better error messages, and
also that it will reduce code duplication when the same kind of error
message needs to be thrown by two different places in the code. As an
added bonus, we can now catch specific errors:

matchTypes (getResultType expected) (getResultType ty)
  `catchError` (\case
                 TCError (TypeMismatchError _ _) _ ->
                     tcError $ TypeMismatchError ty expected
                 TCError err _ -> tcError err
               )

If the first line above throws a TypeMismatchError, we will catch it
and throw a new error. If it's another error we will pass this error
along.

This commit reimplements the error handling of encorec. Before, an error
carried the message that should be displayed, meaning that the more
detailed a message you wanted, the more bloated the error handling code
got. The new model introduces a datatype `Error` which has one
constructor for each kind of error, and showing this value prints the
error message. For example, if there is a type mismatch error, instead
of writing
```
tcError $ "Expected type '" ++ show ty1 ++ "' but found '" ++ show ty2 ++ "'"
```
you now write
```
tcError $ TypeMismatchError ty1 ty2
```

The hope is that this will encourage writing better error messages, and
also that it will reduce code duplication when the same kind of error
message needs to be thrown by two different places in the code. As an
added bonus, we can now catch specific errors:
```
matchTypes (getResultType expected) (getResultType ty)
  `catchError` (\case
                 TCError (TypeMismatchError _ _) _ ->
                     tcError $ TypeMismatchError ty expected
                 TCError err _ -> tcError err
               )
```

If the first line above throws a `TypeMismatchError`, we will catch it
and throw a new error. If it's another error we will pass this error
along.
@@ -110,24 +112,264 @@ instance Pushable Expr where
instance Pushable Typedef where
push t@(Typedef {typedefdef}) = pushMeta t (BTTypedef typedefdef)

classOrTraitName :: Type -> String
classOrTraitName ty
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to be explicit about it, you are missing the classOrTraitOrCapability.
I am not sure if it's better to abstract away from these details and just name the function typeErrorName (you may need to switch the order of the nouns... in Spanish this sounds ok) since the intended use is: given a type (class, capability or trait), return the associated error name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's really just a convenience function for writing "class Foo", "trait Foo" or "capability Foo + Bar" rather than "type Foo". Maybe refTypeName would be better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe :) I thought that it is good if it refers to an abstraction, rather than being super specific

@EliasC
Copy link
Contributor Author

EliasC commented Jun 15, 2016

Adressed the comments about ObjectCreationError thusly:

    show (ObjectCreationError ty)
        | isMainType ty = "Cannot create additional Main objects"
        | isCapabilityType ty =
            printf "Cannot create instance of %s (type must be a class)"
                   (refTypeName ty)
        | otherwise = printf "Cannot create object of type '%s'" (show ty)

@kikofernandez
Copy link
Contributor

I will look at the changes (you need to push :) ) tomorrow and merge around 10 if there are no more issues

This commit can be squashed after review!
@EliasC
Copy link
Contributor Author

EliasC commented Jun 15, 2016

Pushed now! Please squash when you are done :)

@kikofernandez kikofernandez merged commit fb3c1aa into parapluu:development Jun 16, 2016
@kikofernandez kikofernandez deleted the refactoring/error-handling branch June 16, 2016 07:27
@albertnetymk
Copy link
Contributor

I can't build on dev after this merging. Anyone else?

@kikofernandez
Copy link
Contributor

you are completely right... weird... I'll look into it right away

@kaeluka
Copy link
Contributor

kaeluka commented Jun 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants