Add initial error reporting framework #266
Conversation
Tests aren't compiling yet.
| * the whole pretty function string, so we copy it into an array which contains only | ||
| * the type name. | ||
| * | ||
| * Just do type_name<T>() and there's your string_view. */ |
There was a problem hiding this comment.
Final piece: We could do this all in type_name, since we're under a function parameterized by T. But to prevent the entire __PRETTY_FUNCTION__ string from get compiled into the binary, we reassemble it to a smaller string via an array.
There was a problem hiding this comment.
I said that already, in the second to last paragraph.
| case kind::analysis_invalid_recur_from_try: | ||
| return "recur may not be used within a 'try'"; | ||
| case kind::analysis_invalid_recur_args: | ||
| return "The argument arity passed to 'recur' don't match the function's arity"; |
| * is collapsed by an ellipsis. Thus, we need to expand the ellipsis either partially | ||
| * or fully. | ||
| * | ||
| * We do this by finding the releveant ellipsis (there may be multiple) and inserting |
There was a problem hiding this comment.
ellipsis => ellipses?
Ditto remove unneeded ellips[e]s, maybe a few other places.
I'm not familiar with the word ellipse in the variable names.
| /* Remove ellipsis if needed. */ | ||
| if(lines[i].kind == line::kind::ellipsis) | ||
| { | ||
| /* We can be confident there's no note right after an ellipsis. */ |
There was a problem hiding this comment.
I'm just skimming but I didn't catch which invariants guarantee this?
There was a problem hiding this comment.
The top margin guarantees this.
| return err( | ||
| error{ start_token.pos, native_persistent_string{ "value after #( must be present" } }); | ||
| return error::parse_invalid_shorthand_function({ start_token.start, latest_token.end }, | ||
| "Value after #( must be present"); |
There was a problem hiding this comment.
Something's not sitting right with me talking about "values" for all these cases. Is this reporting EOF while reading these tokens?
What does this #( case in particular correspond to?
;; main branch
clojure.core=> #(
Read error (1 - 1): Unterminated list
clojure.core=> #'
Read error (1 - 1): value after #' must be presentThere was a problem hiding this comment.
I haven't combed through every parse error yet, to verify they're all reachable. I think this one is not reachable and so should be either removed or changed to an internal parse error instead.
| @@ -735,7 +727,8 @@ namespace jank::read::parse | |||
| } | |||
| else | |||
| { | |||
| return err(error{ start_token.pos, native_persistent_string{ "Unknown symbolic value" } }); | |||
| return error::parse_invalid_reader_symbolic_value("Unsupported symbolic value", | |||
There was a problem hiding this comment.
I'd vote "Invalid symbolic value".
| return err(error{ start_token.pos, | ||
| native_persistent_string{ "#?@ splice must not be top-level" } }); | ||
| return error::parse_invalid_reader_splice({ start_token.start, latest_token.end }, | ||
| "#?@ splice must not be top-level"); |
There was a problem hiding this comment.
"Top-level #?@ not allowed"?
Or simplify #?@ splice to #?@? Ditto below. Sort of reads like "reader conditional splice splice".
| return err(error{ token.pos, fmt::format("unknown namespace: {}", ns_portion) }); | ||
| return error::parse_unresolved_namespace( | ||
| fmt::format("Unresolved namespace '{}'", ns_portion), | ||
| { start_token.start, latest_token.end }); |
There was a problem hiding this comment.
FYI this case might not be an error due to #195, but I'm working on that.
I have this as
ns = ns_portion;
in my WIP branch, but I don't have an example handy:
There was a problem hiding this comment.
Gotcha. Leaving as-is for now. Can be fixed in your PR.
| * to later review that for error reporting. We automatically clean it up | ||
| * and we reuse the same file over and over. */ | ||
| auto const tmp{ boost::filesystem::temp_directory_path() }; | ||
| auto const path{ tmp / boost::filesystem::unique_path("jank-repl-%%%%.jank") }; |
There was a problem hiding this comment.
Was this bothering you somehow? 😁
There was a problem hiding this comment.
I found this file littered around my directories at some point.
There was a problem hiding this comment.
Hmmm. We weren't writing to a file before. Must've been from something else.
There was a problem hiding this comment.
Oh, probably .jank-repl-history.
There was a problem hiding this comment.
Ah, yes. You'll see that. We don't clean them up. Leiningen does the same with .lein-repl-history.
There was a problem hiding this comment.
Never been bothered by Leiningen's behavior here. Looks like it uses the project-root if project.clj is present, otherwise uses a config directory
There was a problem hiding this comment.
Yeah, I'd prefer if we did something like that, too. Not sure how we'd want to do it, though. We could just use jank's cache dir, which is in ~/.cache/jank by default. That would be a global history for all projects.
There was a problem hiding this comment.
Yes maybe jank should take a path to save history, that can be set by e.g., lein-jank.
Thanks for the review. No requirement to try it locally. You're welcome to, though. |
|
@jeaye just tried it, it's great. I can look at the tests another time, for now I'm happy. |
This branch is getting very far from main, which used to work fine when it was just me, but now we have a lot of people changing main each week. I've decided to merge this in batches.
Included in this PR
./configureRemaining work