Skip to content

Promote R "parseError" errors to ParseResult::SyntaxError #840

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DavisVaughan
Copy link
Contributor

Closes #598
Closes #722

@lionel- I do want to chat about this one with you

Some caveats and things to think about:

  • Sending empty backticks like `` is not a "parseError". It's a regular R error that gets thrown from the parser when R tries to call install() on that symbol, and install() throws its own R error. So we still don't capture it with this, which stinks.
  • We do capture 42 + _ and 1 |> {} and "\s" now, which are all "parseError"s, yay!
  • The "parseError" class was only added in R 4.3+, so I expect some test failures for the moment in our 4.2 check.
  • The "parseError" class is a structured error class that does have lineno as an attribute, so if we had a way to provide a custom handler to try_catch(), then we could pluck out information we care about from there, and that way we would not have to make line optional. Right now it just gives us the message and the class (among other things) and we have no way to customize it.

Some questions:

  • Should we just expect that if R_ParseVector() throws any kind of error, then we should convert it to a ParseResult::SyntaxError? The intent would be to capture the `` case, and any other case we don't yet know about. One thing to be careful of is that in check_console_input() we expect that if we saw a ParseResult::SyntaxError, then we can still just write the input to R's buffer, because we expect that R is going to error on that input "normally". I would not want to make a mistake there. But if we could do this then we'd support R 4.2 better too (I'm not too worried about it if we can't, just saying).
  • Alternatively we can have some known list of messages we look for like attempt to use zero-length variable name that we promote to ParseResult::SyntaxError, but that doesn't feel great either.

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.

Weird error with "\s" Sending a pair of empty backticks to the kernel results in an internal error showing up in the console
1 participant