[PATCH] D124726: Suggest typoed directives in preprocessor conditionals

Tue May 3 07:08:31 PDT 2022

aaron.ballman added a subscriber: rsmith.
aaron.ballman added a comment.

Thank you for working on this!

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

+// Typoed directive warnings
+def TypoedDirective : DiagGroup<"typoed-directive">;
+
----------------
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).

================
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>;
----------------
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.)

================
Comment at: clang/include/clang/Tooling/LevDistance.h:1
+//===--- LevDistance.h ------------------------------------------*- C++ -*-===//
+//
----------------
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.

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

================
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
----------------
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!

================
Comment at: clang/test/Preprocessor/suggest-typoed-directive.c:1
+// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify -pedantic %s
+
----------------
I'd drop the `-pedantic` from here, then we don't need the expected warning on the last line.

I think you should drop the `-std=c99` as well so there's a RUN line that's not tied to a specific release, then I'd add a RUN line explicitly for c2x.

================
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'?}}
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124726