[cfe-dev] Suppress secondary diagnostics for typo correction

Fri Oct 30 11:37:15 PDT 2020

On Fri, Oct 30, 2020 at 11:21 AM Richard Smith <richard at metafoo.co.uk>
wrote:

> On Fri, 30 Oct 2020 at 10:15, David Blaikie <dblaikie at gmail.com> wrote:
>
>> 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.
>>
>
> Right. It's an explicit goal to recover as if the typo-correction is
> applied, in the case where we're confident that it's right. Currently we
> get that confidence by checking the enclosing context in which the typo
> appears is valid once the correction is applied. But that's imperfect in
> various ways -- one of them is that the context we check is a little too
> narrow sometimes; another (the issue in this case) is that making the
> enclosing context be valid is not really sufficient to know that the typo
> correction actually makes sense.
>
> Perhaps we could add some further heuristics to determine whether the
> result of typo correction seems reasonable before deciding we're confident
> it's correct; I could imagine, for example, annotating warnings with a
> "causes typo correction to be considered 'bad'" flag, in much the same way
> as we have a "causes SFINAE failure" flag, and using that to validate
> corrections -- that is, reject typo corrections not only if they would make
> the code invalid, but also if they would produce a warning that suggests
> the code is unlikely to be what the user intended. (In this case I think
> the warning is actually produced after we've finished correcting the typo,
> though that's probably not all that hard to fix.)
>

Sounds plausible to me - what do you think about the typo correction itself
being a bit more reserved about what constitutes a recoverable typo
correction? If the edit distance is too far maybe we shouldn't be
suggesting it, or should be suggesting it at lower priority (like force V
free below - I mean, I appreciate the suggestion if that's the nearest
thing, but even if it did make the code compile without any further
warnings, I'm not sure it's a sufficiently good guess to recover with it?)

- Dave

>
> 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/1de8fe65/attachment-0001.html>