Nothing Special   »   [go: up one dir, main page]

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

Haskell: structured errors #424

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

vkpgwt
Copy link
@vkpgwt vkpgwt commented Aug 8, 2022

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.

  • a new option --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 exports Failure type and some more types to represent parser errors.
  • Par.y now creates a Failure value and converts it to the old-style error string, if string errors are used. Otherwise it returns Either Failure a instead of Either 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 from Par.y.
  • I added record syntax to Posn type in Lex.x, since Posn is now used to report error position in exported failure types, and users might deal with it. I find it useful to use Posn 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:

  • no tests added (but a little test to see if 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?
  • no documentation added
  • I could not check the new option together with --glr, since --glr generates broken code on 2.9.4. This PR should make things even worse, since I added some directives to Par.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?
  • you proposed adding option --errors=string|structured, but in the end I decided to add --string-errors and --structured-errors for the following reasons:
    • supporting an option with a checked argument would require to do some unwieldy refactoring in Options.hs. In fact, I did it and wasn't happy with the result.
    • it matches the style of other options: --glr, --text-tokens, --string-tokens, but not --tokens=....
  • In Haskell: errors as record types #423 you proposed also 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 to Text, so that Err would be Either Text? Frankly speaking, I don't think it will be helpful or efficient, since error messages are short strings, and a user doing Text.pack err would get the same result with almost the same performance, as compared with Text errors constructed internally in BNFC. But if you think it's worth doing, I can add Text errors support.
  • rebase and squash fixup commits

source/src/BNFC/Options.hs Outdated Show resolved Hide resolved
@@ -66,7 +67,14 @@ header modName absName lexName tokenText eps = unlines $ concat
, "{-# LANGUAGE PatternSynonyms #-}"
, ""
, "module " ++ modName
, " ( happyError"
, " ( Err"
, " , Failure(..)"
Copy link
Author

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.

@vkpgwt vkpgwt changed the title Structured errors in Haskell Haskell: structured errors Aug 8, 2022
@vkpgwt vkpgwt marked this pull request as ready for review August 8, 2022 09:50
source/src/BNFC/Options.hs Outdated Show resolved Hide resolved
@vkpgwt
Copy link
Author
vkpgwt commented Aug 24, 2022

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.
@dpwiz
Copy link
dpwiz commented Oct 28, 2022

@andreasabel Is this blocked on something? Any design decisions are yet to be made?

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.

2 participants