[PATCH] D124726: Suggest typoed directives in preprocessor conditionals

Wed May 4 19:00:58 PDT 2022

ken-matsui added a comment.

Thank you so much for your clear review!

================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1023-1024

+// Typoed directive warnings
+def TypoedDirective : DiagGroup<"typoed-directive">;
+
----------------
aaron.ballman wrote:
> We don't typically use "typo" in the user-facing part of diagnostics and this group doesn't seem likely to be reused, so I would remove it (another comment on this elsewhere).
Ah, I see. Thank you!

================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:362-365

+def warn_pp_typo_directive : Warning<
+  "'#%0' directive not found, did you mean '#%1'?">,
+  InGroup<TypoedDirective>;
----------------
aaron.ballman wrote:
> Rather than adding a warning over the top of an existing error, I think we should modify `err_pp_invalid_directive` to have an optional "did you mean?" when we find a plausible typo to correct.
> 
> However, we do not issue that diagnostic when it's inside of a skipped conditional block, and that's what the crux of https://github.com/llvm/llvm-project/issues/51598 is about. As @rsmith pointed out in that issue (and I agree), compilers are free to support custom directives and those will validly appear inside of a conditional block that is skipped. We need to be careful not to diagnose those kinds of situations as an error. However, we still want to diagnose when the unknown directive is "sufficiently close" to another one which can control the conditional chain. e.g.,
> ```
> #fi FOO // error unknown directive, did you mean #if?
> #endfi // error unknown directive, did you mean #endif?
> 
> #if FOO
> #esle // diag: unknown directive, did you mean #else?
> #elfi // diag: unknown directive, did you mean #elif?
> #elfidef // diag: unknown directive, did you mean #elifdef
> #elinfdef // diag: unknown directive, did you mean #elifndef
> 
> #frobble // No diagnostic, not close enough to a conditional directive to warrant diagnosing
> #eerp // No diagnostic, not close enough to a conditional directive to warrant diagnosing
> 
> #endif
> ```
> Today, if `FOO` is defined to a nonzero value, you'll get diagnostics for all of those, but if `FOO` is not defined or is defined to 0, then there's no diagnostics. I think we want to consider directives that are *very likely* to be a typo (edit distance of 1, maybe 2?) for a conditional directive as a special case.
> 
> Currently, we only diagnose unknown directives as an error. I think for these special cased conditional directive diagnostics, we'll want to use a warning rather than an error in this circumstance (just in case it turns out to be a valid directive in a skipped conditional block). If we do go that route and make it a warning, I think the warning group should be `-Wunknown-directives` to mirror `-Wunknown-pragmas`, `-Wunknown-attributes`, etc and it should be defined to have the same text as the error case. e.g., 
> ```
> def err_pp_invalid_directive : Error<
>   "invalid preprocessing directive%select{|; did you mean '#%1'?}0"
> >;
> def warn_pp_invalid_directive : Warning<
>   err_pp_invalid_directive.Text>, InGroup<DiagGroup<"unknown-directives">>;
> ```
> WDYT?
> 
> (These were my thoughts before seeing the rest of the patch. After reading the patch, it looks like we have pretty similar ideas here, which is great, but leaving the comment anyway in case you have further opinions.)
> Currently, we only diagnose unknown directives as an error. I think for these special cased conditional directive diagnostics, we'll want to use a warning rather than an error in this circumstance (just in case it turns out to be a valid directive in a skipped conditional block). If we do go that route and make it a warning, I think the warning group should be `-Wunknown-directives` to mirror `-Wunknown-pragmas`, `-Wunknown-attributes`, etc and it should be defined to have the same text as the error case. e.g., 
> ```
> def err_pp_invalid_directive : Error<
>   "invalid preprocessing directive%select{|; did you mean '#%1'?}0"
> >;
> def warn_pp_invalid_directive : Warning<
>   err_pp_invalid_directive.Text>, InGroup<DiagGroup<"unknown-directives">>;
> ```
> WDYT?
> 
> (These were my thoughts before seeing the rest of the patch. After reading the patch, it looks like we have pretty similar ideas here, which is great, but leaving the comment anyway in case you have further opinions.)

For now, I totally agree with deriving a new warning from `err_pp_invalid_directive`.

However, for future scalability, I think it would be great if we could split those diagnostics into Error & Warning and Help, for example. Rustc does split the diagnostics like the following, and I think this is quite clear. So, a bit apart from this patch, I speculate creating a diagnostic system that can split them would bring Clang diagnostics much more readable.

https://github.com/rust-lang/rust/blob/598d89bf142823b5d84e2eb0f0f9e418ee966a4b/src/test/ui/suggestions/suggest-trait-items.stderr

================
Comment at: clang/include/clang/Tooling/LevDistance.h:1
+//===--- LevDistance.h ------------------------------------------*- C++ -*-===//
+//
----------------
aaron.ballman wrote:
> You shouldn't need this file or the implementation file -- we have this functionality already in: https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/edit_distance.h and https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/StringRef.h#L240.
I totally missed the implementations! Sorry.

But where should I put both `findSimilarStr` & `findSimilarStr`?
It seems that their same implementations aren't there.

================
Comment at: clang/lib/Lex/PPDirectives.cpp:441-449
+                                          const SourceLocation &endLoc) {
+  const std::array<std::string, 8> candidates{
+      "if", "ifdef", "ifndef", "elif", "elifdef", "elifndef", "else", "endif"};
+
+  if (const auto sugg =
+          tooling::levdistance::findSimilarStr(candidates, Directive)) {
+    CharSourceRange DirectiveRange =
----------------
aaron.ballman wrote:
> Mostly just cleaning up for coding conventions, but also, no need to use a `std::array` and we typically don't use local top-level `const` qualification.
Thank you!

Just wondering, but is there any reason not to use the `const` qualifier?

================
Comment at: clang/test/OpenMP/predefined_macro.c:7
 // RUN: %clang_cc1 -fopenmp-simd -fopenmp-version=50 -verify -o - %s
-// expected-no-diagnostics
+// expected-warning at +5 {{'#elsif' directive not found, did you mean '#elif'?}}
 #ifdef FOPENMP
----------------
aaron.ballman wrote:
> ken-matsui wrote:
> > I am not sure if this typo was intended.
> > 
> > When I renamed `elsif` to `elif`, `#error "_OPENMP has incorrect value"` on line `13` was evaluated.
> > 
> > Therefore, if this typo was intended, just suppressing the warning with `expected-warning` would be better. However, if this typo was NOT intended, I think I should make `_OPENMP` equal to `201107`. It is also possible that this test was mistakenly written.
> I tracked down the root cause here and fixed the bug in 50b51b1860acbfb775d5e2eee3310e25c635d667, so when you rebase on main you won't have to deal with this any longer. Good catch!
Thank you for fixing the bug! I could confirm the bug was fixed upstream.

================
Comment at: clang/test/Preprocessor/suggest-typoed-directive.c:10
+// expected-warning at +11 {{'#elfidef' directive not found, did you mean '#elifdef'?}}
+// expected-warning at +11 {{'#elfindef' directive not found, did you mean '#elifdef'?}}
+// expected-warning at +11 {{'#elsi' directive not found, did you mean '#else'?}}
----------------
aaron.ballman wrote:
> It's interesting that this one suggested `#elifdef` instead of `#elifndef` -- is there anything that can be done for that?
> 
> Also, one somewhat interesting question is whether we want to recommend `#elifdef` and `#elifndef` outside of C2x/C++2b mode. Those directives only exist in the latest language standard, but Clang supports them as a conforming extension in all language modes. Given that this diagnostic is about typos, I think I'm okay suggesting the directives even in older language modes. That's as likely to be a correct suggestion as not, IMO.
> It's interesting that this one suggested `#elifdef` instead of `#elifndef` -- is there anything that can be done for that?

I found I have to use `std::min_element` instead of `std::max_element` because we are finding the nearest (most minimum distance) string. Meanwhile, `#elfindef` has 2 distance with both `#elifdef` and `#elifndef`. After replacing `std::max_element` with `std::min_element`, I could suggest `#elifndef` from `#elfinndef`.

> Also, one somewhat interesting question is whether we want to recommend `#elifdef` and `#elifndef` outside of C2x/C++2b mode. Those directives only exist in the latest language standard, but Clang supports them as a conforming extension in all language modes. Given that this diagnostic is about typos, I think I'm okay suggesting the directives even in older language modes. That's as likely to be a correct suggestion as not, IMO.

I agree with you because Clang implements those directives, and the suggested code will also be compilable. I personally think only not compilable suggestions should be avoided. (Or, we might place additional info for outside of C2x/C++2b mode like `this is a C2x/C++2b feature but compilable on Clang`?)

---

According to the algorithm of `std::min_element`, we only get an iterator of the first element even if there is another element that has the same distance. So, `#elfindef` only suggests `#elifdef` in accordance with the order of `Candidates`, and I don't think it is beautiful to depend on the order of candidates. I would say that we can suggest all the same distance like the following, but I'm not sure this is preferable:

```
#elfindef // diag: unknown directive, did you mean #elifdef or #elifndef?
```

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124726/new/

https://reviews.llvm.org/D124726