[cfe-dev] Suppress secondary diagnostics for typo correction

Fri Oct 30 13:31:32 PDT 2020

On Fri, 30 Oct 2020 at 11:37, David Blaikie <dblaikie at gmail.com> wrote:

> 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?)
>

I don't think we have the edit distance computation right yet. force ->
free looks to the edit distance algorithm like two errors (each with a cost
of 1): a spurious 'o' and a 'c' where an 'e' was meant (falling below the
threshold of (length+2)/3 == 2), but to a human they look like completely
different words, whereas getDist -> getDistance looks like 4 errors, which
exceeds the threshold of (7+2)/3 == 3, but to a human would look like
very-likely-correct suggestion.

We'd probably do a lot better if we treated a run of length N of
consecutive additions / removals as having a lower cost than N independent
additions / removals. Similarly, adjacent transposed characters should have
a lower cost than a removal plus an addition / two replacements (which is
how it's currently weighted) -- and should probably have a lower cost than
a single addition or removal. And perhaps a doubled letter should have a
lower cost than a general spurious letter (but maybe I only think that
because the keyboard on my laptop is misbehaving!).

There are other obvious-to-a-human mistakes we make as part of the edit
distance algorithm (for example, suggesting that `get` might be a typo for
`set` or vice versa). There might be broadly-applicable rules we could use
to detect those; for example, we could divide identifiers into "words" by
splitting on underscores and lowercase -> uppercase transitions, and never
treat a word as containing a typo if we've seen that word elsewhere in the
compilation -- so we wouldn't suggest that "getFoo" is a typo for "setFoo"
if we've seen the word "get" appearing in some identifier already, but we
would treat "aetFoo" as a typo for "setFoo". We could in principle get
really smart and notice that (for example) "get" and "set" mean different
things (because we've seen "getFoo" and "setFoo" overloaded) but that "get"
and "is" aren't known to mean different things (because we've never seen
"getFoo" and "isFoo" overloaded), and so decide that "getFoo" is a typo for
"isFoo" not "setFoo". But that's probably excessively complicated.

There are also heuristics we could apply based on keyboard layouts --
spurious letters are more likely if they're on keycaps that are adjacent to
the next or previous letter, and replacements are similarly more likely if
they're on adjacent keycaps -- but in general we don't know what keyboard
layout the user is using.

But even with the best edit distance algorithm, I think you're right that
we can't set a hard cutoff and say "anything better than this that's valid
when substituted into the local context is definitely right; anything worse
than this is definitely wrong", and having a middle ground of "we're not
sure this correction is good enough so we're not going to use it for error
recovery / fix-its" might make sense.

- 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/5062d831/attachment.html>