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
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
Seems like if this interpretation of this part of the protocol causes confusion between servers and clients, it might be worth clarifying usage, etc, to help get some uniformity here.
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.
I was thinking of it more as a more general fix - if part of the issue is multiple fixits on a single line, addressing that general problem (which could come up in non-spelling correction contexts too) would be useful?
- 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.
In the sense that the compiler invoked in its default configuration doesn’t apply any fixits - you have to pass a flag to get it to apply fixits automatically (-fixit or something like that). It’s true that once you pass this flag, it applies all fixits.
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.
OK, then I’m a bit confused about the particular nature of the problem. Is it that the UI doesn’t make it clear which fixits should be applied in what order? There are many non-typo-correction cases where clang would emit ordered fixits (where it doesn’t make sense to apply one without applying the previous one) so I’m a bit confused about which issue is being solved.
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
- 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).
- T1 < match < T2 (less-confident mode): a typo fix-it on a diagnostic note, and no full recovery in the AST (with RecoveryExpr).
- 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.
Yes, I think it does - I think there were a couple of issues. “we can turn the recover off by setting T1 = T2” seems to be incorrect. Setting T1 = T2 would /remove/ (2), the entire thing we’re discussing creating. So I assume you meant “we can turnt he recover off by setting T1 = 0” so that (1) becomes empty (nothing matches in that category) and everything becomes (2). I don’t think it’d be a great user experience to turn off (1) entirely and as much as possible I think it’d be good not to diverge too much between the command line compiler and the IDE integration - so I’d be in favor of doing more to improve the typo correction to help make (1) high quality and (2) lower quality fairly consistently, and then support that both on the command line and via clangd with the same values for T1 and T2.
But if improving the typo correction quality is too much of a rabbit hole, yes, maybe adding these configurations would be good - but even then, perhaps we should make clang use the values that clangd uses here, and offer less recovery typo corrections until they can be made more reliable.