[cfe-dev] [clangd-dev] Delayed typo correction is fragile

[Ilya Biryukov via cfe-dev]

Thanks for pointing this out, the codegen does run on every top level
declaration.
However, it does nothing if any errors were reported.

That means we could prevent codegen by:
1. emitting the diagnostics for uncorrected typos on each top-level
declaration, before the codegen kick in,
2. checking if there are any "pending" typos in addition to checking for
errors before doing the codegen.

Either should be doable. (1) has the advantage of reporting the errors
earlier, making them easier to fix/diagnose.

However, (1) might not be a little involved. At least I got the impression
from talking to various people that some typos are only fixed at template
instantiation time.
The code to figure out at which point the uncorrected typos should be
emitted for template instantiations might be a little involved because of
this.

I would be surprised if the proposed assertion(!HasErrors || Typos.empty())
ever fired in practice. It's rare to see only a single compiler error
coming from clang, so I would expect almost any typo to induce at least
another error right away. That's actually why I'd expect the "broken
codegen" to be hardly possible in practice.

Out of the options we have, I'll probably add checks for (2) to codegen and
emit the delayed typos at the end of TU. That seems to be the simplest
option, at least.
Happy to go with (1) or the alternative assertion if people think the
proposed approach would lead to too many diagnostics.

On Tue, Jul 16, 2019 at 7:29 PM Reid Kleckner <rnk at google.com> wrote:

> On Tue, Jul 16, 2019 at 10:18 AM David Blaikie via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> End of the TU sounds too late to me - IR generation is done incrementally
>> (at the end of functions, for instance - though I'm not sure that's the
>> only point), so leaving typos in until the end of the TU could lead to the
>> "IR generation getting weird because of pending typo corrections" issue, no?
>>
>
> Well, we currently assert in ~Sema, which is after the end of the TU, so
> if we hit today's assert, we've already done incremental codegen without
> crashing. Diagnosing at end of TU doesn't make the situation worse. The way
> I understand clang's incremental codegen strategy, we generate code after
> every top level decl. We sometimes skip incremental codegen if errors have
> been reported or if a Decl is invalid. I think it's a bug if an error
> hasn't been reported but a delayed typo expr gets sent to codegen, and we
> should add asserts to defend against that.
>
>
>> On Mon, Jul 15, 2019 at 8:19 PM Ilya Biryukov via clangd-dev <
>>>>> clangd-dev at lists.llvm.org> wrote:
>>>>>
>>>>>> We would like to avoid assertion failures for those, which leads me
>>>>>> to the following questions:
>>>>>> - Is there a way to quickly track down the place that miss the
>>>>>> CorrectDelayedTypos* call?
>>>>>>
>>>>> A common pattern is that an error causes an Expr subtree to be
>>>>> discarded, and the code that does so "forgets" to call CorrectDelayedTypos.
>>>>> e.g. https://reviews.llvm.org/rL366200
>>>>> There's usually a diagnostic emitted before the Expr is discarded, so
>>>>> in these cases poking around the diag emit location often sheds light. But
>>>>> my fear is there are tens or hundreds of these bugs, and it's hard to
>>>>> enumerate them.
>>>>>
>>>>> At some level, this seems silly - if the Expr doesn't survive, its
>>>>> typos don't need to be corrected to protect CodeGen from them. The
>>>>> diagnostics are probably important though.
>>>>> If we could ensure the diagnostics are emitted as Reid says, and
>>>>> reduce the requirement to be that Exprs that survive parsing get
>>>>> typo-corrected, then this might be tractable.
>>>>>
>>>>
> Another idea is to weaken the assert to validate that either errors have
> been reported, or there are no delayed typos. That would mean it's OK to
> forget to diagnose typos if parts of the AST are invalid, since we expect
> the user to fix their code and recompile, potentially discovering the typo
> on the next compile.
>


-- 
Regards,
Ilya Biryukov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20190716/26d3033d/attachment.html>