Fix incomplete AST in standard json on analysis fail

[cameel]

Depends on #14327. Merged.

In Standard JSON we produce some outputs even in presence of compilation errors. For example if the analysis passes but you get an error in the codegen, you can still get metadata, ABI or the AST, because they depend only on analysis. The AST is even available in an initial form (without annotations) before analysis, which means you can get it in cases where only parsing finishes successfully and analysis fails.

The problem is that if analysis fails, the AST is in some not very well defined intermediate state, and we should not be returning it. StandardCompiler actually has a check against that but this check is not fully effective because it depends on CompilerStack's hasError() flag and this flag is not reliably set on some analysis failures (due to early returns from analysis).

The flag is a problem of its own and will be addressed in a subsequent PR. Here I'm addressing just the part of its unreliability that actually has user-visible consequences and can be considered a bug.

This PR changes the behavior in case where parsing and analysis are both performed (i.e. stopAfter: parsing is not used) and parsing succeeds but analysis does not. There are 3 kinds of such failures:

1. non-fatal error (sets a flag and continues analysis)
2. fatal error that interrupts analysis after current step (returns early from analysis without setting the flag)
3. fatal error that interrupts analysis immediately (raises FatalError, which gets caught and the flag gets set).

Case 2 had this bug while the other two were behaving correctly.

[ekpyron]

I actually don't think we can or should do it like this :-).

The AST export should always check for each annotation, if it's present or not, and only output it, if present, shouldn't it?

Just refusing to output even the merely parsed AST on errors is quite breaking... and outputting all annotations that are there is also fine. The problem is that our annotation mechanism is not completely reliable in the sense that it doesn't make it easy to tell if any annotation actually has a valid value...

[cameel]

Well, I'm not introducing a new mechanism here though. StandardCompiler already blocks outputs that way, so it's not like users can expect the AST to reliably be there on errors. It's just buggy so it does not manage to prevent AST from being included in those cases where analysis returns early. I think it's fair to consider it a bugfix.

But I can also change it the other way so that the AST is never blocked. Or just change nothing and only add tests to bless the current behavior and add coverage. Which one should I do?

[ekpyron]

I haven't really read through the PR yet, but you do prevent any AST output on analysis failures here, don't you?
It's not like the AST output in those cases is "by accident" - it's very much intentional to output all information that's available in that case (in particular including the parsing information). And just dropping that output is very much breaking. But maybe I misread what's happening there, I really only had a very quick glance so far :-).

[cameel]

you do prevent any AST output on analysis failures here, don't you?

Yes, but my point is that even on develop we already prevent it most of the time, even if we have it. I.e. we never output it when CompilerStack.hasError() is true. The cases where we do are exactly the cases where this flag was supposed to be set but isn't.

[ekpyron]

Ah, right - and in the actually more reasonable cases of still outputting an AST, you'd need the error recovery flag, right?
Ok, yeah, then it's fine.

[cameel]

Yeah, but only until #14329. It removes AST output with --error-recovery so you won't be able to get the AST when analysis fails, even with that flag. AST will be produced only when analysis succeeds.

If you say it's useful to get this partial AST, then like I said, I can flip the change and instead output it whenever I have it. Apparently that's how it worked at the time of #6856. Looks like it was changed not to output AST when the error flag is set in the PR that added stopAfter: #9364.

[ekpyron]

Yeah, no, it's fine as is - I just didn't catch the fact that this is only those weird cases. If outside of a few weird cases we already don't get an AST, it's fine to never get an AST, so we can go ahead with the PR as is.