[cfe-dev] Suppress secondary diagnostics for typo correction

Haojian Wu via cfe-dev cfe-dev at lists.llvm.org
Mon Nov 2 02:52:23 PST 2020 

Thanks for all the replies. They are all useful.

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?


Thanks. I didn't notice that this is a clang goal for recovery. At the same
time, this would mean we can't tell the difference from ASTs for
non-typo-correct & typo-correct code because they are exactly the same.

Speaking of clangd, I should have given more information on why we want to
suppress secondary diagnostics:
  - the provided case is from a bug report, and we have user complaints
about it, see https://github.com/clangd/clangd/issues/547
  - the idea of the "first" and "secondary" diagnostics is UI-specific,
most LSP clients just group all diagnostics together, which makes the
"first" diagnostic less obvious to users. The tricky case here is that we
have multiple diagnostics with fix-its at the *same* position -- different
LSP clients have different behavior, and they are suboptimal :(
     - vscode -- the quick-fix prompt-up widget just shows fix-it text, and
there is no way to see which diagnostic does the fix-it fix
     - neovim builtin lsp -- this is worse, the typo-correct "did you mean
free" diag is emitted on the "void free()" line rathan than "if (!force)"
line
  - applying fix-its in interactive environments requires user
confirmations, offering somewhat-speculative diagnostics nearby is a
relatively worse tradeoff
  - recompiling code in clangd is usually fast -- we have preamble
optimization, and fix-its usually don't touch preamble region

Agree that we should improve the typo-correction heuristics, "force/free"
is a bad typo-correction. I will try to see what I can do to make it better
with some reasonable amount of work.

Regarding confidence levels of typo-correction, how about having two
thresholds T1, T2:
 - 0 < match < T1: suggest and recover (the current one is
EditDistance/(length + 2) <= 1/3)
 - T1 < match < T2: suggest but don't recover
 - match > T2: don't suggest

And we can pick different values for clang vs clangd, we can turn the
recover off by setting T1 = T2.

On Fri, Oct 30, 2020 at 9:36 PM David Blaikie <dblaikie at gmail.com> wrote:

>
>
> 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/20201102/281136ae/attachment-0001.html>

More information about the cfe-dev mailing list