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

[Reid Kleckner via clangd-dev]

Richard and I happened to discuss this problem two years ago, I think. What
I half remember from the conversation is that the problem of delayed typo
checking is somewhat equivalent to memory ownership management. Some
methods produce Stmt nodes that may contain delayed typos, and others do
not, and the caller just has to "know" if they are responsible for checking
delayed typos or not. I guess it's different in that you can check the same
Stmt tree twice for typos, but you can't free memory twice. The
consequences of forgetting to check delayed typos are that the compiler may
accept invalid code containing typos, only to crash later in CodeGen or
analysis.

Richard explained that Clang used to do manual memory management for Stmt
nodes. You can see evidence of this in clang/Sema/Ownership.h header.
Experience showed that managing ownership was onerous, so a decision was
made to make all nodes implicitly owned by the ASTContext. Given that clang
removed one form of ownership management, it seems like it would be awkward
to try to use the type system to track delayed typo correction.

Based on that background, I think we need a design that has safe failure
modes. For example, if we could emit errors at end of TU instead of
asserting, that would probably be good enough. Then placing the calls to
CorrectDelayedTyposInExpr well is a matter of diagnostic quality, not
correctness.

Anyway, we got this far in the discussion, but then had to run off and go
do other things as usual. :) If someone has time to improve this, I agree,
it would be great, this assertion pops up in the clang fuzzer reports again
and again.

On Mon, Jul 15, 2019 at 11:19 AM Ilya Biryukov via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> Hi cfe-dev,
>
> In clangd, we often hit the following assertion in the Sema's destructor:
>     assert(DelayedTypos.empty() && "Uncorrected typos!");
>
> It's purpose seems to be to indicate that there are places that should
> call CorrectDelayedTypos* and forget to do so.
>
> However, when the assertion fires it's hard to track down the exact place
> where the CorrectDelayedTypos* call should be inserted. Moreover, it never
> fires in normal clang executions because the driver passes -disable-free by
> default, which stops Sema's destructor from running.
>
> That leaves clangd and libclang in an unfortunate situation of being the
> only clients that experience crashes (apart from lit that run clang -cc1
> mode and don't pass -disable-free).
>
> 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?
> - If no, would it be ok to turn this assertion into some kind of
> debug-only warning and
>   document that some typos are never actually corrected due to limitations?
> - Even broader, are there any ideas for an alternative design that would
> be more resilient to
>   changes in the codebase?
>   E.g. no delayed typo corrections or easy-to-audit places that should run
> CorrectDelayedTypos*, etc.
>
> --
> Regards,
> Ilya Biryukov
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/clangd-dev/attachments/20190715/14498c32/attachment.html>