[cfe-dev] Suppress secondary diagnostics for typo correction

[David Blaikie via cfe-dev]

A major goal of Clang's diagnostic experience is that if a fixit is
suggested (such as a typo correction) then subsequent errors are exactly
as-if the fixit had been applied. It sounds like your suggestion would go
counter to that?

I think there's probably a good argument to be made that not all typo
corrections are high-confidence enough to merit a fixit on the error itself
- if the fixit is on a note instead, the above requirement of recovery
isn't applicable (so that's where we can put, say "did you mean if (a ==
b)" as well as "did you mean if ((a = b))" fixits on alternative notes on
the general -Wparentheses warning) - so perhaps having some level of typo
correction confidence would be useful to determine which kind of recovery
we should do - full recovery as if the user wrote the code (with a fixit
hint attached to the error itself) or "well, we're not sure but here's out
best guess" where an invalid expr is created and the fixit hint is attached
to a note with some wording that's a bit more vague/conveys the increased
uncertainty compared to the former case.

On Fri, Oct 30, 2020 at 7:34 AM Haojian Wu via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> Hello folks,
>
> Given the following case:
>
> void free();
> void test() {
>    if (!force) {} // diagnostic 1:  use of undeclared identifier 'force';
> did you mean 'free'?
>                      // diagnostic 2:  warning: address of function 'free'
> will always evaluate to 'true'
> }
>
> The secondary diagnostic seems to be bogus, and it doesn't reflect the
> written source code, which can easily cause confusions. My idea is to use a
> dependent RecoveryExpr (which wraps the typo-correct AST node) to suppress
> all secondary diagnostics.
>
> I have a prototype at https://reviews.llvm.org/D90459. I see some
> improvements, but there are some regressions as well:
>
> Improvements
> - the resulting AST look better because the error is visible in the AST
> (with RecoveryExpr node)
> - we emit more typo corrections for more cases, see [1]
> <https://reviews.llvm.org/differential/changeset/?ref=2240247>, [2]
> <https://reviews.llvm.org/differential/changeset/?ref=2240248>
>
> Regressions
> - recursive/nested typo corrections, e.g. `TypoX.TypoY;`, we emit just 1
> typo-correction while the old behavior emits two, see [1]
> <https://reviews.llvm.org/differential/changeset/?ref=2240254>
> - ambiguous typos, when there are ambiguous typo candidates (they have the
> same edit distance), the old one seems to perform better in some cases, see
> [1] <https://reviews.llvm.org/differential/changeset/?ref=2240246>
> - other misc regressions, I think we could fix them
>
> The secondary diagnostics are not wrong from the AST perspective, but they
> seem to be unnecessary. In clangd, we'd like to suppress all secondary
> diagnostics, but I'm not sure this is a right choice for clang.
>

That would seem unfortunate to me - clang works pretty hard on diagnostic
recovery so users can see/act on multiple diagnostics in one pass. Though I
realize that model is a bit different if you're dealing with an editor
that's recompiling after every textual change - is that always the case for
clangd? I think it might still be useful to see more than one error in an
IDE/editor's error list, and certainly if I were dealing with some code
that's slow to compile or an editor that chooses to do less fine-grained
recompiles.


>
> What do people think?
>
>
> Thanks,
> Haojian
>
> _______________________________________________
> 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/cfe-dev/attachments/20201030/12379ac4/attachment-0001.html>