[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

[David Goldman via Phabricator via cfe-commits]

dgoldman added a comment.
Herald added a subscriber: usaxena95.

In D64799#1651908 <https://reviews.llvm.org/D64799#1651908>, @vsapsai wrote:

> For the record, there was another change regarding the delayed typos in `clang::Sema::~Sema()`: D62648 <https://reviews.llvm.org/D62648> [Sema][Typo] Fix assertion failure for expressions with multiple typos.


Note that D62648 <https://reviews.llvm.org/D62648> only makes fixes to the typo correction application (e.g. when `CorrectDelayedTyposInExpr` is called), in particular caused by the recursive nature of typo correction (correcting typos might introduce more typos!). The assertion failure can still be caused by failing to correct Typos (e.g not calling `CorrectDelayedTyposInExpr` for a given Expression).

This patch seems like a good idea as currently it only impacts clangd/libclang in practice. But here a few thoughts about potentially fixing the root cause here:

- It looks as if when initially implemented <https://github.com/llvm/llvm-project/commit/6c759519bb374cbe9866d602665f4048b597a7e6> typos were meant to be propagated up through the `ExprEvalContext` (see the commit description) as opposed to automatically clearing for every context, perhaps due to the issues described above. It's unclear what the intended behavior is though - when should `CorrectDelayedTyposInExpr` be called? When are we in a `stable state` to do typo correction?
- From my limited understanding there is one main issue here: failure to call `CorrectDelayedTyposInExpr` for an expression. When fixing the typo correction handling mentioned above, this could happen during `TreeTransform` due to an Expression being created (along with a typo) and then being thrown away (due to an error). The problem here is that once it is discarded, it becomes unreachable. I'm assuming the proper handling here is to `CorrectDelayedTyposInExpr` more frequently, but I'm not quite sure.
  - Could `Sema` check if a `TypoExpr` becomes unreachable? If so, we can check for unreachable `TypoExprs`, diagnose them, and potentially if in `DEBUG` emit a warning (although we'd need to track where a `TypoExpr` was created to really understand why it was thrown away).



In D64799#1605514 <https://reviews.llvm.org/D64799#1605514>, @ilya-biryukov wrote:

> Tried the suggested approach and ran into the issue described above.
>  Either we make the diagnostics less useful ('did you mean foo::bar?' turns into 'unresolved identifier bar'; without mentioning the correction)  or we even stop providing some corrections in addition to that.
>
> On the other hand, I agree that over time we will start emitting too many diagnostics at the end of TU if keep the patch as is. That is not a good way.
>  There should be a better option for emitting the uncorrected diagnostics closer to where they are produced, but I don't have a good idea on what it should be off the top of my head. Ideas warmly welcome.


I might be wrong here, but I thought that diagnostics were delayed until typo correction has been completed on an expression. For example, you'll only get something like `unresolved identifier bar` instead of `did you mean foo::bar?` only when you call the `DiagHandler` with either a proper `TypoCorrection` or an empty one. If so, I'm not sure how you'd get into this case if you're calling `CorrectDelayedTyposInExpr` and tracking which typos have been resolved already.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64799/new/

https://reviews.llvm.org/D64799