Page MenuHomePhabricator

[RecoveryAST] Add design doc to clang internal manual.
ClosedPublic

Authored by hokein on Feb 18 2021, 12:50 AM.

Details

Summary

Hopefully it would be useful for new developers.

Diff Detail

Event Timeline

hokein requested review of this revision.Feb 18 2021, 12:50 AM
hokein created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2021, 12:50 AM

thanks! i think this mostly looks great, leaving a couple of suggestions and questions in comments.

clang/docs/InternalsManual.rst
1882

rather than some form maybe say while indicating the error ?

1886

so far we've always been talking about the current state right? comparison to past seems surprising now. can we have a couple of examples for the cases that we still drop the nodes today?

1889

s/nice/better

1890

maybe drop to AST consumers or move it to the begining of the statement i.e.
provides AST consumers with a rich AST reflecting the written source code as ....

1910

s/was/would be

1933

this talks about why it would be hard to make use of CallExprs but doesn't say what we would gain by doing so. I suppose it would come with the benefit of preserving more details about the source code, like locations for parantheses and knowing the type of the expr? (or are these still accessible e.g. do we have an enum in RecoveryExpr telling us about its type?)

1959

that's great to know! i would expect it to be there already, but i think we should have this as a comment within the code too, so that future maintainers can feel confident when setting the type of a recovery expr.

1971

IIRC, there were other problems with clang trying to emit secondary diags on such cases. It might be worth mentioning those too, again to warn future travellers about the side effects of making this non-value-dependent.

1978

might be worth mentioning this only exists for expressions.

1979

not sure what second this is for

1980

this sounds like it is propagated all the way to the TUDecl, but i suppose that's not the case. can you elaborate on when the propagation stops?

sammccall accepted this revision.Feb 18 2021, 5:26 AM
sammccall added reviewers: rsmith, kadircet.

I've reviewed this previously and am happy with how it ended up.
But Kadir's comments are good, and we should also wait a few days in case others want changes.

clang/docs/InternalsManual.rst
1886

nit: "graceful recovery. Prior to..."

1933

Ooh, you're right: this says "the trade off" but only mentions one side :-)

Maybe "This would capture more call details and allow it to be treated uniformly with valid CallExprs. However, jamming..."

1971

Do you have an example? I chatted with Haojian offline about this and we couldn't find a good one apart from constant contexts. (And the wording of the standard suggests that value-dependence is a concept that's only interesting in constant contexts)

This revision is now accepted and ready to land.Feb 18 2021, 5:26 AM
hokein updated this revision to Diff 324616.Feb 18 2021, 6:30 AM
hokein marked 4 inline comments as done.

address review comments

hokein added inline comments.Feb 18 2021, 6:32 AM
clang/docs/InternalsManual.rst
1882

I'm not sure this will be better. indicating error seems to be a bit strong.

e.g. if () {}, this ill-formed statement, the AST node is like below, which doesn't have an obvious error signal (we could argue the OpaqueValueExpr is )

`-IfStmt
   |-OpaqueValueExpr 'bool'
   `-CompoundStmt
1886

. can we have a couple of examples for the cases that we still drop the nodes today?

The concern of giving a dropped example is that it might be stale once it gets preserved and recovered in the future. So personally, I'd prefer to give a true example, it was the mismatched-argument function call.

I guess this is not too hard for readers to come up with an example, a pretty broken statement would be the case.

1933

yeah, this is good point. added.

1959

yeah, we already have this in the comment of RecoveryExpr.

1971

I think the existing doesn't emit more errors texts already indicate we suppress the secondary diags etc.

1978

in reality, they are complicated, these bits (template, contains-errors) are not only for expressions, but also for Type, NestedNameSpecifier, TemplateArgument.

1979

oh, removed it.

1980

you're right, but the whole propagation process is complex, I think it needs more words to explain how and when it stops (and it is probably out-of-scope). I adjusted the word a bit to make less confusing.

kadircet accepted this revision.Feb 19 2021, 4:53 AM
kadircet added inline comments.
clang/docs/InternalsManual.rst
1882

oh okay didn't know that. i thought we would always have a node which can indicate the error somehow (not having that is still surprising though).

1971

Do you have an example?

unfortunately not :( but something tells me the particular bug was somehow about typo correction happening inside recoveryexprs ?

I think the existing doesn't emit more errors texts already indicate we suppress the secondary diags etc.

yeah i guess you are right.