[cfe-dev] Suppress secondary diagnostics for typo correction

Fri Oct 30 13:36:44 PDT 2020

On Fri, Oct 30, 2020 at 1:31 PM Richard Smith <richard at metafoo.co.uk> wrote:

> 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.
>

Ah, fair points all - yeah, maybe the higher prioritiy/bigger bang-for-buck
today will be improvements to the edit distance algorithm itself, before we
get to the point of benefiting from the sort of nuanced
separation/classification of relative certainty.

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