[Clangd] Added hidden command line option -tweaks to specify which tweaks to enable

[SureYeaah]

Summary

• Only for testing purposes
• Disabled tweaks in fixits-duplications test

[sammccall]

This seems somewhat inflexible; how can we use this to test tweaks?

[sammccall]

Sorry, hit submit too early...

We could use -tweaks=comma,separated,list and have the ClangdServer::Options member be a function<bool(StringRef)> TweakFilter or so

[SureYeaah]

Replaced the boolean flag with a list of tweaks flag

[SureYeaah]

Removed extra comment

[SureYeaah]

Whitespace formatting

[sammccall]

clang-tools-extra/clangd/ClangdServer.h

145

ClangdServer::Options needs to be default-constructible with sensible default options.

Can you either inline-initialize this = [](llvm::StringRef) { return true; } (if possible), or have the code handle TweakFilter==nullptr as accepting anything?

clang-tools-extra/clangd/tool/ClangdMain.cpp

541

it would be clearer to set this only if TweakList.getNumOccurrences(), rather than using it inside the lambda, I think

543

return llvm::find(TweakList, TweakToSearch) != TweakList.end() ?

[SureYeaah]

Refactored code

clang-tools-extra/clangd/ClangdServer.h

struct Options {
     bool HiddenFeatures = false;
 
     /// Clangd will execute compiler drivers matching one of these globs to
     /// fetch system include path.
     std::vector<std::string> QueryDriverGlobs;
 
     /// Enable semantic highlighting features.
     bool SemanticHighlighting = false;
+    
+    /// Disable tweaks for testing purposes
+    bool DisableTweaks = false;
   };
   // Sensible default options for use in tests.
   // Features like indexing must be enabled if desired.
   static Options optsForTest();
 
   /// Creates a new ClangdServer instance.
   ///
   /// ClangdServer uses \p CDB to obtain compilation arguments for parsing. Note
private:
 
   // When set, provides clang-tidy options for a specific file.
   ClangTidyOptionsBuilder GetClangTidyOptions;
 
   // If this is true, suggest include insertion fixes for diagnostic errors that
   // can be caused by missing includes (e.g. member access in incomplete type).
   bool SuggestMissingIncludes = false;
   bool EnableHiddenFeatures = false;
+  bool DisableTweaks = false;
   
   // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
   llvm::StringMap<llvm::Optional<FuzzyFindRequest>>
       CachedCompletionFuzzyFindRequestByFile;
   mutable std::mutex CachedCompletionFuzzyFindRequestMutex;
 
   llvm::Optional<std::string> WorkspaceRoot;
   // WorkScheduler has to be the last member, because its destructor has to be

clang-tools-extra/clangd/ClangdServer.cpp

ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
                            const Options &Opts)
     : FSProvider(FSProvider),
       DynamicIdx(Opts.BuildDynamicSymbolIndex
                      ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex)
                      : nullptr),
       GetClangTidyOptions(Opts.GetClangTidyOptions),
       SuggestMissingIncludes(Opts.SuggestMissingIncludes),
       EnableHiddenFeatures(Opts.HiddenFeatures),
+      DisableTweaks(Opts.DisableTweaks),
       WorkspaceRoot(Opts.WorkspaceRoot),
       // Pass a callback into `WorkScheduler` to extract symbols from a newly
       // parsed file and rebuild the file index synchronously each time an AST
       // is parsed.
       // FIXME(ioeric): this can be slow and we may be able to index on less
       // critical paths.
       WorkScheduler(
           CDB, Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
auto Action = [this, Sel](decltype(CB) CB, std::string File,
                             Expected<InputsAndAST> InpAST) {
     if (!InpAST)
       return CB(InpAST.takeError());
     auto Selection = tweakSelection(Sel, *InpAST);
     if (!Selection)
       return CB(Selection.takeError());
     std::vector<TweakRef> Res;
     for (auto &T : prepareTweaks(*Selection)) {
-      if (T->hidden() && !EnableHiddenFeatures)
+      if (DisableTweaks || (T->hidden() && !EnableHiddenFeatures))
         continue;
       Res.push_back({T->id(), T->title(), T->intent()});
     }
     CB(std::move(Res));
   };
 
   WorkScheduler.runWithAST("EnumerateTweaks", File,
                            Bind(Action, std::move(CB), File.str()));

clang-tools-extra/clangd/test/fixits-duplication.test

-# RUN: clangd -lit-test -clang-tidy-checks=modernize-use-nullptr,hicpp-use-nullptr < %s | FileCheck -strict-whitespace %s
+# RUN: clangd -lit-test -clang-tidy-checks=modernize-use-nullptr,hicpp-use-nullptr --disable-tweaks < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"codeAction":{"codeActionLiteralSupport":{}}}},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.cpp","languageId":"cpp","version":1,"text":"void foo() { char* p = 0; }"}}}
 #      CHECK:    "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
 # CHECK-NEXT:        "code": "hicpp-use-nullptr",

clang-tools-extra/clangd/tool/ClangdMain.cpp

static llvm::cl::list<std::string> QueryDriverGlobs(
     "query-driver",
     llvm::cl::desc(
         "Comma separated list of globs for white-listing gcc-compatible "
         "drivers that are safe to execute. Drivers matching any of these globs "
         "will be used to extract system includes. e.g. "
         "/usr/bin/**/clang-*,/path/to/repo/**/g++-*"),
     llvm::cl::CommaSeparated);
 
+static llvm::cl::opt<bool> DisableTweaks(
+    "disable-tweaks",
+    llvm::cl::desc("Disable tweak diagnostics for testing purposes."),
+    llvm::cl::init(false), llvm::cl::Hidden);
+
 namespace {
 
 /// \brief Supports a test URI scheme with relaxed constraints for lit tests.
 /// The path in a test URI will be combined with a platform-specific fake
 /// directory to form an absolute path. For example, test:///a.cpp is resolved
 /// C:\clangd-test\a.cpp on Windows and /clangd-test/a.cpp on Unix.
 class TestScheme : public URIScheme {
 public:
AsyncIndexLoad = runAsync<void>([Placeholder] {
         Placeholder->reset(std::move(Idx));
     });
     if (Sync)
       AsyncIndexLoad.wait();
   }
   Opts.StaticIndex = StaticIdx.get();
   Opts.AsyncThreadsCount = WorkerThreadsCount;
   Opts.HiddenFeatures = HiddenFeatures;
-
+  Opts.DisableTweaks = DisableTweaks;
   clangd::CodeCompleteOptions CCOpts;
   CCOpts.IncludeIneligibleResults = IncludeIneligibleResults;
   CCOpts.Limit = LimitResults;
   CCOpts.BundleOverloads = CompletionStyle != Detailed;
   CCOpts.ShowOrigins = ShowOrigins;
   CCOpts.InsertIncludes = HeaderInsertion;
   if (!HeaderInsertionDecorators) {
     CCOpts.IncludeIndicator.Insert.clear();
Opts.GetClangTidyOptions = [&](llvm::vfs::FileSystem &,
       // This function must be thread-safe and tidy option providers are not.
       std::lock_guard<std::mutex> Lock(ClangTidyOptMu);
       // FIXME: use the FS provided to the function.
       return ClangTidyOptProvider->getOptions(File);
     };
   }
   Opts.SuggestMissingIncludes = SuggestMissingIncludes;
   Opts.QueryDriverGlobs = std::move(QueryDriverGlobs);
   llvm::Optional<OffsetEncoding> OffsetEncodingFromFlag;
   if (ForceOffsetEncoding != OffsetEncoding::UnsupportedEncoding)
     OffsetEncodingFromFlag = ForceOffsetEncoding;
   ClangdLSPServer LSPServer(
       *TransportLayer, FSProvider, CCOpts, CompileCommandsDirPath,
       /*UseDirBasedCDB=*/CompileArgsFrom == FilesystemCompileArgs,
       OffsetEncodingFromFlag, Opts);
   llvm::set_thread_name("clangd.main");
   return LSPServer.run() ? 0
                          : static_cast<int>(ErrorResultCode::NoShutdownRequest);
 }