[cfe-dev] Suppress secondary diagnostics for typo correction

Haojian Wu via cfe-dev cfe-dev at lists.llvm.org
Tue Nov 3 01:16:55 PST 2020 

On Mon, Nov 2, 2020 at 7:09 PM David Blaikie <dblaikie at gmail.com> wrote:

> On Mon, Nov 2, 2020 at 2:52 AM Haojian Wu <hokein at google.com> wrote:
>
>> 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.
>>
>
> Yep
>
>
>> 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
>>
>
> Any chance of improving the consumers here?
>

I hope so -- usually, we report bugs to the consumers. As a language
server, we don't own them and from our experience, different LSP clients
have different opinions, and we can't really fix the world :(

 Alternatively/also, perhaps a general workaround/solution to clangd that
> strips fixits if they're at the same position?
>

This is a possible solution, but it seems to workaround the issue rather
than fixing it.


>   - applying fix-its in interactive environments requires user
>> confirmations, offering somewhat-speculative diagnostics nearby is a
>> relatively worse tradeoff
>>
>
> Not quite sure I follow this one - applying fix-its in a command line
> compiler situation requires user interaction too.
>

oh, I didn't know this, can you say more about this?  My understanding of
applying fix-its via command-line tools (e.g. clang -cc1 -fixit
/path/fix.cc) is that it just applies all fix-its simultaneously, and there
is no user interaction.


> You mean some editors automatically prompt for application of fixits as
> soon as they're generated? Rather than the user, say, right-clicking and
> asking to apply them?
>

I mean the latter one. In general, editors will highlight the range of
diagnostics in some way, and the user can put a cursor in that range, and
ask to apply fix-it.


> Yeah, that would create a very different bar for fixits, I think. I'd
> potentially argue that that might not be a great editor default (like
> clang's -fixit isn't the default, but can be opted into).
>
>
>>   - 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.
>>
>
> I'd still be pretty hesitant to encourage/support the direction of turning
> off typo correction recovery (though I guess you mean "by setting T1 = 0",
> right?) but not fundamentally problematic to demote all the typo correction
> to note fixits - why not all fixits, then? Is there something specifically
> about spelling fixits that make them different from other fixits? Perhaps
> today, given the not-so-great edit distance calculation, the false positive
> rate for typo corrections may be too high and so even at low values of T1
> there are still too many suspect typo corrections compared to other more
> manually implemented non-typo fixits.
>

If I read your words correctly, we might have a misunderstanding about the
"recover" term here.
I think what I meant is
1) 0 < match < T1 (high-confident mode): a typo fix-it on a diagnostic
error, and full recovery in the AST (the AST is exactly the same as the
typo fix-it is applied).
2) T1 < match < T2 (less-confident mode): a typo fix-it on a diagnostic
note, and no full recovery in the AST (with RecoveryExpr).
3) match > T2: no typo-correction at all.

Currently we have 1) and 3) in clang. 2) is the missing part, and I think
it matches what you suggested before.


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

More information about the cfe-dev mailing list