[Lex] Fix `fixits` for typo-corrections of preprocessing directives within skipped blocks

[akyrtzi]

Summary

The EndLoc parameter was always unset so no fixit was emitted. But it is also unnecessary for determining the range so we can remove it.

[akyrtzi]

No need to add '#' for the fixit since we have the range of the directive identifier to fix.

[akyrtzi]

Remove the parameter from documentation comment as well.

[akyrtzi]

Disable clang-format checks for the test file.

[aaron.ballman]

Good catch, thank you for fixing this up! LGTM!

clang/test/Preprocessor/suggest-typoed-directive.c

14–15

You can remove this -- we don't require test files to be formatted properly, and formatting comments can quickly become distracting.

[akyrtzi]

clang/test/Preprocessor/suggest-typoed-directive.c

14–15

The debian bot failed for this patch with clang-format complaining about the test file. Was this unintentional?

[akyrtzi]

clang/test/Preprocessor/suggest-typoed-directive.c

14–15

For reference the bot failure is here: https://buildkite.com/llvm-project/premerge-checks/builds/96481#01814013-61ed-4a53-a9d0-4d1a183ac360

[aaron.ballman]

clang/test/Preprocessor/suggest-typoed-directive.c

14–15

Yeah -- the precommit CI bots still check it and report back failures, which is incredibly frustrating given how much noise that generates. I've filed https://github.com/llvm/llvm-project/issues/55982 to track that.

[akyrtzi]

Remove // clang-format off annotation.

clang/include/clang/Lex/Preprocessor.h

private:
   /// Return true if an error occurs parsing the arg list.
   bool ReadMacroParameterList(MacroInfo *MI, Token& LastTok);
 
   /// Provide a suggestion for a typoed directive. If there is no typo, then
   /// just skip suggesting.
   ///
   /// \param Tok - Token that represents the directive
   /// \param Directive - String reference for the directive name
-  /// \param EndLoc - End location for fixit
-  void SuggestTypoedDirective(const Token &Tok,
-                              StringRef Directive,
-                              const SourceLocation &EndLoc) const;
+  void SuggestTypoedDirective(const Token &Tok, StringRef Directive) const;
 
   /// We just read a \#if or related directive and decided that the
   /// subsequent tokens are in the \#if'd out portion of the
   /// file.  Lex the rest of the file, until we see an \#endif.  If \p
   /// FoundNonSkipPortion is true, then we have already emitted code for part of
   /// this \#if directive, so \#else/\#elif blocks should never be entered. If
   /// \p FoundElse is false, then \#else directives are ok, if not, then we have
   /// already seen one so a \#else directive is a duplicate.  When this returns,

clang/lib/Lex/PPDirectives.cpp

SourceLocation Preprocessor::CheckEndOfDirective(const char *DirType,
   if ((LangOpts.GNUMode || LangOpts.C99 || LangOpts.CPlusPlus) &&
       !CurTokenLexer)
     Hint = FixItHint::CreateInsertion(Tmp.getLocation(),"//");
   Diag(Tmp, diag::ext_pp_extra_tokens_at_eol) << DirType << Hint;
   return DiscardUntilEndOfDirective().getEnd();
 }
 
 void Preprocessor::SuggestTypoedDirective(const Token &Tok,
-                                          StringRef Directive,
-                                          const SourceLocation &EndLoc) const {
+                                          StringRef Directive) const {
   // If this is a `.S` file, treat unknown # directives as non-preprocessor
   // directives.
   if (getLangOpts().AsmPreprocessor) return;
 
   std::vector<StringRef> Candidates = {
       "if", "ifdef", "ifndef", "elif", "else", "endif"
   };
   if (LangOpts.C2x || LangOpts.CPlusPlus2b)
     Candidates.insert(Candidates.end(), {"elifdef", "elifndef"});
 
   if (Optional<StringRef> Sugg = findSimilarStr(Directive, Candidates)) {
-    CharSourceRange DirectiveRange =
-        CharSourceRange::getCharRange(Tok.getLocation(), EndLoc);
-    std::string SuggValue = Sugg.getValue().str();
+    // Directive cannot be coming from macro.
+    assert(Tok.getLocation().isFileID());
+    CharSourceRange DirectiveRange = CharSourceRange::getCharRange(
+        Tok.getLocation(),
+        Tok.getLocation().getLocWithOffset(Directive.size()));
+    StringRef SuggValue = Sugg.getValue();
 
-    auto Hint = FixItHint::CreateReplacement(DirectiveRange, "#" + SuggValue);
+    auto Hint = FixItHint::CreateReplacement(DirectiveRange, SuggValue);
     Diag(Tok, diag::warn_pp_invalid_directive) << 1 << SuggValue << Hint;
   }
 }
 
 /// SkipExcludedConditionalBlock - We just read a \#if or related directive and
 /// decided that the subsequent tokens are in the \#if'd out portion of the
 /// file.  Lex the rest of the file, until we see an \#endif.  If
 /// FoundNonSkipPortion is true, then we have already emitted code for part of
if (Directive.startswith("if")) {
           Sub == "ndef") {  // "ifndef"
         // We know the entire #if/#ifdef/#ifndef block will be skipped, don't
         // bother parsing the condition.
         DiscardUntilEndOfDirective();
         CurPPLexer->pushConditionalLevel(Tok.getLocation(), /*wasskipping*/true,
                                        /*foundnonskip*/false,
                                        /*foundelse*/false);
       } else {
-        SuggestTypoedDirective(Tok, Directive, endLoc);
+        SuggestTypoedDirective(Tok, Directive);
       }
     } else if (Directive[0] == 'e') {
       StringRef Sub = Directive.substr(1);
       if (Sub == "ndif") {  // "endif"
         PPConditionalInfo CondInfo;
         CondInfo.WasSkipping = true; // Silence bogus warning.
         bool InCond = CurPPLexer->popConditionalLevel(CondInfo);
         (void)InCond;  // Silence warning in no-asserts mode.
if (Directive.startswith("if")) {
           }
           // If this condition is true, enter it!
           if (static_cast<bool>(MI) == IsElifDef) {
             CondInfo.FoundNonSkip = true;
             break;
           }
         }
       } else {
-        SuggestTypoedDirective(Tok, Directive, endLoc);
+        SuggestTypoedDirective(Tok, Directive);
       }
     } else {
-      SuggestTypoedDirective(Tok, Directive, endLoc);
+      SuggestTypoedDirective(Tok, Directive);
     }
 
     CurPPLexer->ParsingPreprocessorDirective = false;
     // Restore comment saving mode.
     if (CurLexer) CurLexer->resetExtendedTokenMode();
   }
 
   // Finally, if we are out of the conditional (saw an #endif or ran off the end

clang/test/Preprocessor/suggest-typoed-directive.c

 // RUN: %clang_cc1 -fsyntax-only -verify=pre-c2x-cpp2b %s
 // RUN: %clang_cc1 -std=c2x -fsyntax-only -verify=c2x-cpp2b %s
 // RUN: %clang_cc1 -x c++ -std=c++2b -fsyntax-only -verify=c2x-cpp2b %s
+// RUN: %clang_cc1 -x c++ -std=c++2b -fsyntax-only %s -fdiagnostics-parseable-fixits 2>&1 | FileCheck %s
 
 // id:        pre-c2x-cpp2b-warning@+12 {{invalid preprocessing directive, did you mean '#if'?}}
 // ifd:       pre-c2x-cpp2b-warning@+12 {{invalid preprocessing directive, did you mean '#if'?}}
 // ifde:      pre-c2x-cpp2b-warning@+12 {{invalid preprocessing directive, did you mean '#ifdef'?}}
 // elf:       pre-c2x-cpp2b-warning@+12 {{invalid preprocessing directive, did you mean '#elif'?}}
 // elsif:     pre-c2x-cpp2b-warning@+12 {{invalid preprocessing directive, did you mean '#elif'?}}
 // elseif:    pre-c2x-cpp2b-warning@+12 {{invalid preprocessing directive, did you mean '#elif'?}}
 // elfidef:   not suggested to '#elifdef'
 // elfindef:  not suggested to '#elifdef'
 // elfinndef: not suggested to '#elifndef'
 // els:       pre-c2x-cpp2b-warning@+12 {{invalid preprocessing directive, did you mean '#else'?}}
 // endi:      pre-c2x-cpp2b-warning@+12 {{invalid preprocessing directive, did you mean '#endif'?}}
 #ifdef UNDEFINED
 #id
 #ifd
 #ifde
-#elf
-#elsif
+# elf
+#  elsif
 #elseif
 #elfidef
 #elfindef
 #elfinndef
 #els
 #endi
 #endif
 // id:        c2x-cpp2b-warning@-12 {{invalid preprocessing directive, did you mean '#if'?}}
 // ifd:       c2x-cpp2b-warning@-12 {{invalid preprocessing directive, did you mean '#if'?}}
 // ifde:      c2x-cpp2b-warning@-12 {{invalid preprocessing directive, did you mean '#ifdef'?}}
 // elf:       c2x-cpp2b-warning@-12 {{invalid preprocessing directive, did you mean '#elif'?}}
 // elsif:     c2x-cpp2b-warning@-12 {{invalid preprocessing directive, did you mean '#elif'?}}
 // elseif:    c2x-cpp2b-warning@-12 {{invalid preprocessing directive, did you mean '#elif'?}}
 // elfidef:   c2x-cpp2b-warning@-12 {{invalid preprocessing directive, did you mean '#elifdef'?}}
 // elfindef:  c2x-cpp2b-warning@-12 {{invalid preprocessing directive, did you mean '#elifdef'?}}
 // elfinndef: c2x-cpp2b-warning@-12 {{invalid preprocessing directive, did you mean '#elifndef'?}}
 // els:       c2x-cpp2b-warning@-12 {{invalid preprocessing directive, did you mean '#else'?}}
 // endi:      c2x-cpp2b-warning@-12 {{invalid preprocessing directive, did you mean '#endif'?}}
 
+// CHECK: fix-it:{{.*}}:{[[@LINE-24]]:2-[[@LINE-24]]:4}:"if"
+// CHECK: fix-it:{{.*}}:{[[@LINE-24]]:2-[[@LINE-24]]:5}:"if"
+// CHECK: fix-it:{{.*}}:{[[@LINE-24]]:2-[[@LINE-24]]:6}:"ifdef"
+// CHECK: fix-it:{{.*}}:{[[@LINE-24]]:3-[[@LINE-24]]:6}:"elif"
+// CHECK: fix-it:{{.*}}:{[[@LINE-24]]:4-[[@LINE-24]]:9}:"elif"
+// CHECK: fix-it:{{.*}}:{[[@LINE-24]]:2-[[@LINE-24]]:8}:"elif"
+// CHECK: fix-it:{{.*}}:{[[@LINE-24]]:2-[[@LINE-24]]:9}:"elifdef"
+// CHECK: fix-it:{{.*}}:{[[@LINE-24]]:2-[[@LINE-24]]:10}:"elifdef"
+// CHECK: fix-it:{{.*}}:{[[@LINE-24]]:2-[[@LINE-24]]:11}:"elifndef"
+// CHECK: fix-it:{{.*}}:{[[@LINE-24]]:2-[[@LINE-24]]:5}:"else"
+// CHECK: fix-it:{{.*}}:{[[@LINE-24]]:2-[[@LINE-24]]:6}:"endif"
+
 #ifdef UNDEFINED
 #i // no diagnostic
 #endif
 
 #if special_compiler
 #special_compiler_directive // no diagnostic
 #endif