[Basic] Emit warning flag suggestion only in case there's existing flag *similar* to the unknown one

[jkorous]

Summary

In case user specified non-existent warning flag clang was suggesting an existing on that was in some cased very different. This patch should ensure that either a relevant warning flag is suggested or none (which is in line with unknown generic command line option handling).

Examples:
warning: unknown warning option '-Whiskey'; did you mean '-Wpacked'?
warning: unknown warning option '-Weverthin'; did you mean '-Wsection'?

rdar://problem/37755482

I changed interface of DiagnosticIDs::getNearestOption() to return the edit distance between nonexistent flag and the closest exiting one. In principle this code is analogous to OptTable::findNearest() for generic command line options but I don't find them similar enough in order to factor out some shared implementation. I tentatively selected maximum edit distance as 2 (CompilerInvocation::CreateFromArgs() uses maximum edit distance of 1 for generic case).

[arphaman]

Thanks for working on this!
It looks like we only have one client of getNearestOption. Wouldn't it make sense to fold the check into the function itself?

[jkorous]

I like that idea! It looks like it's living in a wrong place anyway as it doesn't need access to any of implementation details (private members) of DiagnosticID. I would still like to preserve it as a function so this block of code has clear semantics and interface.
How about I refactor it to a static free function in Warnings.cpp?

[arphaman]

Sounds good

[jkorous]

I tried to move the getNearestOption() to it's only client - EmitUnknownDiagWarning() but it turned out to be a significant change because of clang/Basic/DiagnosticGroups.inc use in DiagnosticIDs.cpp.
I suggest we leave that for eventual future refactoring since it would blow up scope of this patch a lot.

[jkorous]

Ping.

[arphaman]

Thanks for working on this, some minor comments:

Basic/DiagnosticIDs.cpp

612

Do we a test case that covers this line?

Basic/Warnings.cpp

34

It would be helpful to have a brief comment explaining the rationale behind using the number 2.

35

NIT: Missing //end anonymous namespace.

48

Is it ever possible to have an edit distance of 0? Wouldn't it mean the strings are the same?

[vsapsai]

Have you considered the same approach as typo correction? I.e. for the max allowed edit distance use percentage of the input size. For example, something similar to [TypoCorrectionConsumer::addName](https://github.com/apple/swift-clang/blob/4d16b315bc18647ca6715b7b4ea95a26a68fb5ed/lib/Sema/SemaLookup.cpp#L3992-L4008).

Basic/DiagnosticIDs.cpp

 
 void DiagnosticIDs::getAllDiagnostics(diag::Flavor Flavor,
                                       std::vector<diag::kind> &Diags) {
   for (unsigned i = 0; i != StaticDiagInfoSize; ++i)
     if (StaticDiagInfo[i].getFlavor() == Flavor)
       Diags.push_back(StaticDiagInfo[i].DiagID);
 }
 
-StringRef DiagnosticIDs::getNearestOption(diag::Flavor Flavor,
-                                          StringRef Group) {
-  StringRef Best;
+unsigned DiagnosticIDs::getNearestOption(diag::Flavor Flavor,
+                                          StringRef Group,
+                                          std::string& NearestOption) {
   unsigned BestDistance = Group.size() + 1; // Sanity threshold.
   for (const WarningOption &O : OptionTable) {
     // Don't suggest ignored warning flags.
     if (!O.Members && !O.SubGroups)
       continue;
 
     unsigned Distance = O.getName().edit_distance(Group, true, BestDistance);
     if (Distance > BestDistance)
       continue;
 
     // Don't suggest groups that are not of this kind.
     llvm::SmallVector<diag::kind, 8> Diags;
     if (::getDiagnosticsInGroup(Flavor, &O, Diags) || Diags.empty())
       continue;
 
     if (Distance == BestDistance) {
       // Two matches with the same distance, don't prefer one over the other.
-      Best = "";
+      NearestOption = "";
     } else if (Distance < BestDistance) {
       // This is a better match.
-      Best = O.getName();
+      NearestOption = O.getName();
       BestDistance = Distance;
     }
   }
 
-  return Best;
+  return BestDistance;
 }
 
 /// ProcessDiag - This is the method used to report a diagnostic that is
 /// finally fully formed.
 bool DiagnosticIDs::ProcessDiag(DiagnosticsEngine &Diag) const {
   Diagnostic Info(&Diag);
 
   assert(Diag.getClient() && "DiagnosticClient not set!");

Basic/Warnings.cpp

 #include "clang/Basic/AllDiagnostics.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include <algorithm>
 #include <cstring>
 #include <utility>
 using namespace clang;
 
+namespace {
+  constexpr unsigned MaxLevenshteinDistForWarningSuggestion = 2;
+}
+
 // EmitUnknownDiagWarning - Emit a warning and typo hint for unknown warning
 // opts
 static void EmitUnknownDiagWarning(DiagnosticsEngine &Diags,
                                    diag::Flavor Flavor, StringRef Prefix,
                                    StringRef Opt) {
-  StringRef Suggestion = DiagnosticIDs::getNearestOption(Flavor, Opt);
+  std::string Suggestion;
+  const unsigned OptToSuggestionEditDist
+    = DiagnosticIDs::getNearestOption(Flavor, Opt, Suggestion);
   Diags.Report(diag::warn_unknown_diag_option)
     << (Flavor == diag::Flavor::WarningOrError ? 0 : 1) << (Prefix.str() += Opt)
-    << !Suggestion.empty() << (Prefix.str() += Suggestion);
+    << (!Suggestion.empty()
+      && OptToSuggestionEditDist > 0
+      && OptToSuggestionEditDist <= MaxLevenshteinDistForWarningSuggestion)
+    << (Prefix.str() += Suggestion);
 }
 
 void clang::ProcessWarningOptions(DiagnosticsEngine &Diags,
                                   const DiagnosticOptions &Opts,
                                   bool ReportDiags) {
   Diags.setSuppressSystemWarnings(true);  // Default to -Wno-system-headers
   Diags.setIgnoreAllWarnings(Opts.IgnoreWarnings);
   Diags.setShowOverloads(Opts.getShowOverloads());

Driver/cc-log-diagnostics.c

 // CHECK:   <key>dwarf-debug-flags</key>
 // CHECK:   <string>{{.*}}clang{{.*}}-fsyntax-only{{.*}}</string>
 // CHECK:   <key>diagnostics</key>
 // CHECK:   <array>
 // CHECK:     <dict>
 // CHECK:       <key>level</key>
 // CHECK:       <string>warning</string>
 // CHECK:       <key>message</key>
-// CHECK:       <string>unknown warning option &apos;-Wfoobar&apos;; did you mean &apos;-W{{.*}}&apos;?</string>
+// CHECK:       <string>unknown warning option &apos;-Wfoobar&apos;</string>
 // CHECK:     </dict>
 // CHECK:     <dict>
 // CHECK:       <key>level</key>
 // CHECK:       <string>warning</string>
 // CHECK:       <key>filename</key>
 // CHECK:       <string>{{.*}}cc-log-diagnostics.c</string>
 // CHECK:       <key>line</key>
 // CHECK:       <integer>7</integer>
 // CHECK:       <key>column</key>
 // CHECK:       <integer>11</integer>
 // CHECK:       <key>message</key>
 // CHECK:       <string>control reaches end of non-void function</string>
 // CHECK:     </dict>
 // CHECK:   </array>
 // CHECK: </dict>

Frontend/warning-options.cpp

 // RUN: %clang_cc1 -Wmonkey -Wno-monkey -Wno-unused-command-line-arguments \
 // RUN:        -Wno-unused-command-line-argument -Wmodule-build -Rmodule-built %s 2>&1 | FileCheck %s
 // CHECK: unknown warning option '-Wmonkey'
 // CHECK: unknown warning option '-Wno-monkey'
 // CHECK: unknown warning option '-Wno-unused-command-line-arguments'; did you mean '-Wno-unused-command-line-argument'?
-// CHECK: unknown warning option '-Wmodule-build'; did you mean '-Wmodule-conflict'?
+// CHECK: unknown warning option '-Wmodule-build'
 // CHECK: unknown remark option '-Rmodule-built'; did you mean '-Rmodule-build'?

clang/Basic/DiagnosticIDs.h

public:
   bool getDiagnosticsInGroup(diag::Flavor Flavor, StringRef Group,
                              SmallVectorImpl<diag::kind> &Diags) const;
 
   /// Get the set of all diagnostic IDs.
   static void getAllDiagnostics(diag::Flavor Flavor,
                                 std::vector<diag::kind> &Diags);
 
   /// Get the diagnostic option with the closest edit distance to the
-  /// given group name.
-  static StringRef getNearestOption(diag::Flavor Flavor, StringRef Group);
+  /// given Group name and return their edit distance.
+  /// \param [out] NearestOption - The nearest option string found.
+  ///
+  /// \return The edit distance between Group and the nearest option found.
+  static unsigned getNearestOption(diag::Flavor Flavor, StringRef Group,
+                                   std::string& NearestOption);
 
 private:
   /// Classify the specified diagnostic ID into a Level, consumable by
   /// the DiagnosticClient.
   /// 
   /// The classification is based on the way the client configured the
   /// DiagnosticsEngine object.
   ///