Open
Conversation
ca00695 to
c46f1b9
Compare
vkpgwt
commented
Aug 8, 2022
vkpgwt
commented
Aug 8, 2022
| , "module " ++ modName | ||
| , " ( happyError" | ||
| , " ( Err" | ||
| , " , Failure(..)" |
Author
There was a problem hiding this comment.
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
vkpgwt
commented
Aug 8, 2022
vkpgwt
commented
Aug 8, 2022
Author
|
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? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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=stringis a default.Par.ynow exportsFailuretype and some more types to represent parser errors.Par.ynow creates aFailurevalue and converts it to the old-style error string, if string errors are used. Otherwise it returnsEither Failure ainstead ofEither String a. As I checked manually, the string error format hasn't been changed (maybe we need a test for it?).ErrM.hsis 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.Posntype inLex.x, sincePosnis now used to report error position in exported failure types, and users might deal with it. I find it useful to usePosnas 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.hsis 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--glrgenerates broken code on 2.9.4. This PR should make things even worse, since I added some directives toPar.ywhich 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-errorsand--structured-errorsfor 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=....textargument for--errorsoption 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 thatErrwould 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 errwould get the same result with almost the same performance, as compared withTexterrors constructed internally inBNFC. But if you think it's worth doing, I can addTexterrors support.