-
Notifications
You must be signed in to change notification settings - Fork 165
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
Haskell: structured errors #424
base: master
Are you sure you want to change the base?
Conversation
ca00695
to
c46f1b9
Compare
@@ -66,7 +67,14 @@ header modName absName lexName tokenText eps = unlines $ concat | |||
, "{-# LANGUAGE PatternSynonyms #-}" | |||
, "" | |||
, "module " ++ modName | |||
, " ( happyError" | |||
, " ( Err" | |||
, " , Failure(..)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe avoid exporting Failure
& co if --errors=string
? It might break the backward compatibility if the user imports Par.hs
unqualified without import list.
c46f1b9
to
99f98a8
Compare
Hello @andreasabel! Would you mind to look at the PR? |
New options are introduced: "--structured-errors" and "--string-errors". They can specify the parser failure type.
afee537
to
874c50b
Compare
@andreasabel Is this blocked on something? Any design decisions are yet to be made? |
Hello! This is a proposed implementation of structured errors in Haskell (discussed in #423). Currently it's mostly done, but I guess I need to add some tests, and some issues should be discussed.
--errors=string|structured
. It's implemented for Haskell backend only, but its name is abstract enough to be used in other backends in future.--errors=string
is a default.Par.y
now exportsFailure
type and some more types to represent parser errors.Par.y
now creates aFailure
value and converts it to the old-style error string, if string errors are used. Otherwise it returnsEither Failure a
instead ofEither String a
. As I checked manually, the string error format hasn't been changed (maybe we need a test for it?).ErrM.hs
is not created with--errors=structured
, since I couldn't find a way to modify its content appropriately. And it seems to be for keeping backward compatibility only. Now all error types are exported fromPar.y
.Posn
type inLex.x
, sincePosn
is now used to report error position in exported failure types, and users might deal with it. I find it useful to usePosn
as it is (absolute position too, not just the line and column numbers). In my project I need to get some context around the error position, which is more convenient to do when we have the absolute position too.Some issues that might be fixed if needed:
ErrM.hs
is omitted). I guess we need some success tests with--errors=structured
, and the generated parser tests should be run too - what I have to do manually. Could you please make a hint what tests and where I should add?--glr
, since--glr
generates broken code on 2.9.4. This PR should make things even worse, since I added some directives toPar.y
which change the lexer signature from the parser's perspective. I found some comments in the source code which mention that GLR is obsolete. Should I fix it?--errors=string|structured
, but in the end I decided to add--string-errors
and--structured-errors
for the following reasons:Options.hs
. In fact, I did it and wasn't happy with the result.--glr
,--text-tokens
,--string-tokens
, but not--tokens=...
.text
argument for--errors
option which is not supported in the PR, since I'm not sure I understand what it should mean. Should it just change the error type toText
, so thatErr
would beEither Text
? Frankly speaking, I don't think it will be helpful or efficient, since error messages are short strings, and a user doingText.pack err
would get the same result with almost the same performance, as compared withText
errors constructed internally inBNFC
. But if you think it's worth doing, I can addText
errors support.