Better evaluator errors #407
Conversation
| import Wasp.Analyzer.Type (Type) | ||
| import Wasp.Util (concatPrefixAndText, indent) | ||
|
|
||
| newtype EvaluationError = EvaluationError (WithCtx EvaluationError') |
There was a problem hiding this comment.
Here for the first time I used WithCtx to put Ctx in the error, and not just AST. In the later PR I also do this for TypeError.
The only "ugly" thing is that I had a bit of hard time naming data EvaluationError'. I could name them EvaluationErrorWithCtx and EvaluationError, but the thing is that in the code we mostly use the first one and don't care about the second one much, so having such a long and overly detailed name sounded impractical. Therefore I decided to just call it EvaluationError and EvaluationError'. One other way to do it could be EvaluationError and EvaluationErrorReason/EvalautionErrorCause, but again I didn't want to over complicate.
| @@ -24,4 +24,4 @@ getErrorMessageAndCtx :: AnalyzeError -> (String, Ctx) | |||
| getErrorMessageAndCtx = \case | |||
| ParseError e -> first (("Parse error:\n" ++) . indent 2) $ PE.getErrorMessageAndCtx e | |||
| TypeError e -> first (("Type error:\n" ++) . indent 2) $ TE.getErrorMessageAndCtx e | |||
| EvaluationError _e -> error "TODO" | |||
| EvaluationError e -> first (("Evaluation error:\n" ++) . indent 2) $ EE.getErrorMessageAndCtx e | |||
There was a problem hiding this comment.
How informative is it to the user to know that something is an "evaluation" error? Parse and Type error sound more intuitive (especially Type), but the evaluation is probably the least informative, right? I guess there is no other word that would make things clearer?
There was a problem hiding this comment.
Yeah, hard to say, but as you said, we don't have a better name right now. We could make it more general, just "Compiler error", but I am not sure if that brings any value also.
| ) | ||
| UndefinedVariable varName -> ("Undefined variable " ++ varName, ctx) | ||
| InvalidEnumVariant enumType validEnumVariants actualEnumVariant -> | ||
| ( "Expected value of enum type '" ++ enumType |
There was a problem hiding this comment.
Yes, but when I finished it I realized this is one of those errors that will probably never happen due to typechecker doing most of the job and evaluations being generated automatically by our TH logic. But ok still cool :).
There was a problem hiding this comment.
And I considered doing something similar in earlier stages of compiler, but the thing is, if you type wrong identifier, e.g. AuthhMethod, it will fail during typechecking so early that TypeChecker doesn't yet know which type it expects there. We could probably make it work if we really wanted to, but I don't see an easy way right now to generate such nice error message for this case so I would rather leave that for later.
There was a problem hiding this comment.
Yeah, just took a look, there is certainly a way to make this work, but it would require refactoring type inference to make it more intertwined with the type "weakening", and that would take some amount of time/focus to get right, so probably best not to do it now.
There was a problem hiding this comment.
ok, a lot of cool language words here :D But I agree that we don't have to go now into it if it is complicated. We could open a gh issue for it though, so we don't forget it?
There was a problem hiding this comment.
All right, created #421!
Martinsos
force-pushed
the
better-type-errors
branch
2 times, most recently
from
8044623
to
cf3ec4b
Compare
Martinsos
force-pushed
the
better-evaluator-errors
branch
2 times, most recently
from
7e54f66
to
b4f91ad
Compare
|
LGTM 👍🏻 nice job! |
Martinsos
force-pushed
the
better-evaluator-errors
branch
from
b4f91ad
to
53d835d
Compare
Same thing as for ParseError and for TypeError, but now for EvaluatorError -> I enriched errors with Ctx (source position) information and taught them how to display themselves and report ctx/position.