Error reporting

[ratmice]

Sorry for sending another large PR. I was hoping I could just send some smaller ones, but it seems fairly difficult to break up. In theory I could break it up at patches 04ba0c1, c7bc81f and last 050d207.

But 6d41bbb modifies a trait. I wanted the addition of the trait method to be close to its usage, because the specific method was a key source of bugs during the development (when failing to call it) one way to avoid it would be to call finish from the ErrorMap trait. But we'd have to document that finish can be called multiple times, so using drain would be necessary. In a sense that would just swapping API awkwardness elsewhere.

Overall this branch has aspects of all three of the patches (callbacks, trait, and channel series minus the channels). I'm not soo terribly certain how well actually trying to do incremental or prompt receiving of errors at the time they occurr. In particular the combination of warnings and errors means that one of warnings or errors could block the other. unless you spawn a thread for each, or use a channel which is sync on the grmtools side, and async on the outside (like my lsp does).

I'm not very smitten on some of the names, but have lacked any good alternatives like ErrorReport is kind of like ErrorEscrow or ErrorCache and using the NewlineCache style names for methods like feed_error(), feed_warning(). But I tried not to get bogged down on naming and just get it working from end to end.

There isn't anything in the branch which calls it from build.rs, follows is an example, but FWIW, the parts hooking it up to CTParserBuilder could probably use some work, but at least as a proof of concept show it is possible here to get the source + the path to an error formatter without plumbing it all the way through the parser, or repeatedly building NewlineCache.

struct Formatter {}

impl Formatter {
    fn formatter(_src: &str, _path: &std::path::Path) -> Box<dyn ErrorFormatter> {
        Box::new(Self {}) as Box<dyn ErrorFormatter>
    }
}

impl ErrorFormatter for Formatter {
    fn format_error(&self, _: &YaccGrammarError) -> Box<dyn Error> {
        "fancy error".into()
    }

    fn format_warning(&self, _: &YaccGrammarWarning) -> String {
        "fancy warning".to_string()
    }
}

... .lrpar_config(|ctp| { ctp.error_formatter(Box::new(Formatter::formatter)).grammar_in_src_dir(...).unwrap() })

[ratmice]

I just cleaned up some by removing some lifetimes, these were holdovers from experiments regarding where path/sources should be fed into formatters from (Specifically it was in the ErrorReport at some time, but it turned out to simplify things immensely to pass it directly to the ErrorFormatter instead). Anyhow with that stuff no longer the case it seems we can elide some lifetimes that I wasn't able to before.

[ratmice]

I wanted to break this out a bit, (sorry for all the noise).

[The idea of a Stage below is interesting. I'm mildly nervous about it, though, because it does somewhat bake in some aspects of the implementation that aren't fundamental.

I went from finish() (revealing the least amount of information) to revealing the most amount of information (the actual stage). In theory there is a middle ground that just notifies when a scope in which we do the checking for specific errors is entered/exited. I.e. more than finished() but less than stage().

I at least understand that this bakes in a guarantee that is a little bit weird, and not impossible to break if we start producing the same errors across different sections. But I think the consequences of failure to uphold it are pretty minimal in this usage at least (potential unmerged duplicate errors being reported seperately when they could be merged). But not really sure about other cases.

I also wonder if a user who cares about these distinctions can already infer it from the error kinds they see?]

In particular our errors are pretty opaque now with kind field being private, and no accessor function. So they would either have to parse text output, or map span information to the spans of sections (concrete syntax we don't keep anywhere in the AST).

The important thing that we don't get from mapping errors to section information though, is an indication of progress absent errors. Let me just give a list of thoughts on use cases I imagined for stages (and other random thoughts):

• Reporting progress in steps (possible with just start/finish events not revealing the actual stage if you use an indeterminate number).
• Providing stage information along with errors: "[Error] In the rules section: ..."
• Knowing when an error has not occurred, so you can flush buffers or stop listening for them.
• If we did associate span information to Stage, that is a way we could map errors to sections, without having to add section spans to the AST, then figuring out the section of an error would be as easy as section spans and error spans to a interval tree.

[ltratt]

@ratmice Sorry I'm so slow on this one. It's big enough that every time I sit down to think it through I run out of time / mental capacity! That's not your fault at all, but I'm making things worse by not responding promptly! I wonder if we can start breaking this down into chunks that an idiot like me can understand? Here are some concrete suggestions --- please feel free to tell me these are a bit off!

https://github.com/softdevteam/grmtools/pull/436/commits/04ba0c15bb5aad1e1d76b446dbfb8252cff3e92d (though obviously missing the SimpleReport struct ATM?) looks like a simple change we can happily PR against master?

https://github.com/softdevteam/grmtools/pull/436/commits/c7bc81fac4a97b1c46cbce5fc439cb32f22b6e3c is a little trickier, and probably couldn't be PRed as is, but it does suggest a path forward. Most obviously this is a part of grmtools which probably has very few direct users, so API breakage is acceptable, assuming it leads us to a better system.

The observation (I think) underlying c7bc81f is that we've got two completely stages, each of which can have different kinds of errors: parsing the input grammar into an AST; converting the AST to a more thoroughly checked format. [In retrospect "AST" was probably the wrong term here. It's really a sort of "semi-processed parse tree" or "unchecked AST". Perhaps the latter is the least-worst term?] Dumb question: should we be separating those two stages out? In other words, are there use cases where someone really needs the UncheckedAST but not the YaccGrammar? My gut feeling is that users don't really care about the UncheckedAST phase per se: they want to know about syntax errors in their input; and they want to know about the semantic errors; but I don't think they really want to separate those two things out so strictly that we have separate APIs for each. So, perhaps, we could merge these two things into a single GrammarBuilder? Obviously we'd then have to think about the error API, but that's step 2 :)

[ratmice]

Do not worry about taking time, I want to apologize too because while I've tried, I haven't really managed to split it up nicely enough due to all the dependencies across the series.

04ba0c1 I intended to say more to the effect of "Convert ASTWithValidityInfo to use SimpleReport", SimpleReport is in abc5f0a which itself depends on 2244b6a The whole of 2244b6a .. 04ba0c1 probably has to be squashed into one patch (or two if we want to separate the existing code from the additions).

Let me try and quickly explain the original purpose of returning a "the valid partial fragment of an invalid AST", basically it allows us to fail to parse the grammar, continue parsing the lexer, and do checks like "token present in parser but missing from lexer", check on the lexer still. It was kind of a precursor to my Analysis branch which never managed to make it in. I.e. you could run still an analysis because the AST only ever contains the part which was valid up-to the point of the error which caused it to be invalid. So part of the thinking was using it as a way to move some of the checks which happen currently inside the main parse function out, or doing additional restrictions like "all tokens should be uppercase" or something.

So, it was part of an attempt to produce more errors within a single run even when the parser fails to build, but for cross-tool ones which depend upon the AST from which we can't produce a full lrtable.

[ltratt]

Why don't we do a PR for the (probably-squashed) https://github.com/softdevteam/grmtools/commit/2244b6a6545172699b14f33779eb01601a44f07d .. https://github.com/softdevteam/grmtools/commit/04ba0c15bb5aad1e1d76b446dbfb8252cff3e92d as that seems largely uncontentious to me?

Let me try and quickly explain the original purpose of returning a "the valid partial fragment of an invalid AST", basically it allows us to fail to parse the grammar, continue parsing the lexer, and do checks like "token present in parser but missing from lexer", check on the lexer still. It was kind of a precursor to my Analysis branch which never managed to make it in. I.e. you could run still an analysis because the AST only ever contains the part which was valid up-to the point of the error which caused it to be invalid. So part of the thinking was using it as a way to move some of the checks which happen currently inside the main parse function out, or doing additional restrictions like "all tokens should be uppercase" or something.

Right, but I think we could still give users the same kinds of errors (including those where we've attempted a sort-of error recovery) and only expose 1 stage as pub externally? Internally we could have as many stages as we want, of course! But I might be very wrong and have failed to understand the big picture.

[ratmice]

Why don't we do a PR for the (probably-squashed) 2244b6a .. 04ba0c1 as that seems largely uncontentious to me?

Sure, I would be happy to do that, it would be good to have a look at driver though, it pulls some of these structure out and some stuff in cfgrammar like Span/Spanned, it's own crates and adds some things like SourceId Error: SourceArtifact where SourceArtifact (Artefact?) being a way to get a SourceId from an Error.

In other words it contains much of the stuff in this patch but pulls it out into it's own crate/interface that both lrlex/lrpar could depend upon.

It would be probably good to see if this has any legs, and we'd rather pursue either it or something similar instead of continuing this with this one.

Edit: It is probably worth noting that there are a few improvements I would like to make, but haven't figure out how to yet

[ratmice]

Why don't we do a PR for the (probably-squashed) 2244b6a .. 04ba0c1 as that seems largely uncontentious to me?

Let me try and quickly explain the original purpose of returning a "the valid partial fragment of an invalid AST", basically it allows us to fail to parse the grammar, continue parsing the lexer, and do checks like "token present in parser but missing from lexer", check on the lexer still. It was kind of a precursor to my Analysis branch which never managed to make it in. I.e. you could run still an analysis because the AST only ever contains the part which was valid up-to the point of the error which caused it to be invalid. So part of the thinking was using it as a way to move some of the checks which happen currently inside the main parse function out, or doing additional restrictions like "all tokens should be uppercase" or something.

Right, but I think we could still give users the same kinds of errors (including those where we've attempted a sort-of error recovery) and only expose 1 stage as pub externally? Internally we could have as many stages as we want, of course! But I might be very wrong and have failed to understand the big picture.

The main difficulty here with making a 1 stage pub API here, with all this internal to it has been the cross-crate aspects of it (the checks occur in lrlex, with the ast in question being in lrpar, so by the time we have a both lex symbols, and the list of parse symbols from the return value, the AST is gone, so this two-stage validation is a way to return it externally from the crate. But then only produce YaccGrammars for those things which passed all the hidden internal checks without any tampering in between.

[ratmice]

I'm going close this, and continue experimenting, because I think the driver experiment shows that even some of the things within the 2244b6a .. 04ba0c1 changes might be better suited to being in their own library, where they can be used by both lrpar/lrlex.

But it also seems clear to me that the experiment is over complex, and I don't know what it should ultimately look like. And I'd like to see whether making user visible breaking changes (around lrpar_config closure) can reduce the reliance on mutable state or if it's really inherent to the problem.

Since I guess feel now for the time being that I am not actually confident in any of this or the other stuff ending up as something I would actually propose for merging. It seems best for now to do that experimentation in a way that has the least impact for everybody.