[clangd] A tool to evaluate cross-file rename.

[hokein]

Summary

This is a tool that helps us to evaluate the rename results, we may not
submit this tool to the code repository, but it has value, helps us find
out bugs in AST/Refs/Rename code.

[merge_guards_bot]

Build result: pass - 60496 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

[hokein]

add more symbols.

[hokein]

Fix empty lines.

[merge_guards_bot]

Build result: pass - 60496 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

[merge_guards_bot]

Build result: pass - 60496 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

[kbobyrev]

For some reason, running this patch on a recent version of the source tree fails. The AST parsing fails with fatal diagnostics, but I can't understand why:

E[15:14:33.084] source file unknown type name 'PathRef' is illformed: 1
E[15:15:02.831] source file unknown type name 'NestedNameSpecifierLoc'; did you mean 'std::clang::NestedNameSpecifierLoc'? is illformed: 1
E[15:14:02.912] source file unknown type name 'Location' is illformed: 1
E[15:14:47.797] source file 'index' is not a class, namespace, or enumeration is illformed: 1
E[15:14:17.658] source file no template named 'vector' in namespace 'std' is illformed: 1

I've had many unsuccessful attempts to track down the bug and use multiple different configurations (i.e. I thought something might be wrong with my index location or directory structure, so in the end I replicated the setup that I can infer from the code), but I still didn't manage to run the renames.

In order to make it easier for you to understand what went wrong I have logged my commands and output so that one could understand what happens. I've tried several versions of LLVM, the log is for the most recent one, https://github.com/kirillbobyrev/llvm-project/commit/c7dc4734d23f45f576ba5af2aae5be9dfa2d3643. I've made sure to run check-all to validate correctness of source tree and to generate all test files so that indexer does not crash. I've made sure that all the commands ran without any errors.

Because the output is quite verbose, I'm submitting another file which only lists the commands instead of the one with verbose output. If you need the full log, please let me know. Attached file:

log.txt937 BDownload

Please let me know if I have done something incorrectly.

clang-tools-extra/clangd/eval-rename/RenameMain.cpp

35

Maybe just Apply? Fix seems slightly confusing.

clang-tools-extra/clangd/eval-rename/eval-rename.py

2

It probably makes sense to migrate this to Python 3 instead (shouldn't be too hard, from what I can see there are Python 2 print statements, but nothing else I can find).

12

Maybe some comments regarding what each field in symbol_to_rename.txt corresponds to and how it is parsed would be useful.

22

Would be useful to add an argument to the build directory, I typically don't put build/ into llvm/.

24

nit: run?

83

git reset --hard might be dangerous if compile-commands are symlinked to the build directory: ninja -C build would re-generate them and git reset --hard will e.g. erase add_subdirectory(eval-rename) if it's not committed. Maybe this should be mentioned somewhere in the comments/user guide.

[hokein]

• rebase to master
• fix the breakage of the rename tool
• add more symbol to the tests
• some tweaks on the python script

[hokein]

sorry for the delay, I just saw the comment today.

In D71110#1799960, @kbobyrev wrote:

For some reason, running this patch on a recent version of the source tree fails. The AST parsing fails with fatal diagnostics, but I can't understand why:

E[15:14:33.084] source file unknown type name 'PathRef' is illformed: 1
E[15:15:02.831] source file unknown type name 'NestedNameSpecifierLoc'; did you mean 'std::clang::NestedNameSpecifierLoc'? is illformed: 1
E[15:14:02.912] source file unknown type name 'Location' is illformed: 1
E[15:14:47.797] source file 'index' is not a class, namespace, or enumeration is illformed: 1
E[15:14:17.658] source file no template named 'vector' in namespace 'std' is illformed: 1

I encountered the same error when rebasing to master. It was work before. To parse the file correctly, we need to inject the clang builtin header directory (via resource-dir) to the compile command, it was done by OverlayCDB, but there was a patch changing this behavior in December, which broke the tool.
It should be fixed now.

clang-tools-extra/clangd/eval-rename/eval-rename.py

2

good point.

22

yeah. the current script currently assumes the directory layout:

llvm-project
  - build/
    - bin/...
  - clang-tools-extra
  - ...

83

agree. it is dangerous, the script is expected to be ran on a clean client. we could improve it, e.g. prompt a confirm dialog, or abandon if there are dirty changes in the client.

[kbobyrev]

Thanks! Now I can build it.

I was able to get some rename failures through this tool, though :( I had to disable the check for number of affected files first, but then this seems to be problematic:

llvm/include/llvm/ADT/ArrayRef.h llvm::ArrayRef clangd
llvm/include/llvm/ADT/StringRef.h llvm::StringRef clangd

[kbobyrev]

Renaming llvm::Optional also fails (clangd-rename says the symbol is not of a supported kind even though it's a class).

Renaming llvm::None is also not working: there seem to be many problems but a lot of them are connected with function's argument default values (void foo(llvm::Optional<int> Bar = None).

llvm/include/llvm/ADT/Optional.h llvm::Optional clangd
llvm/include/llvm/ADT/None.h llvm::None clangd

[hokein]

Thanks for the feedback!

Yeah, currently template classes are not supported in cross-file rename, see https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clangd/refactor/Rename.cpp#L187.

So llvm::Optional and llvm::Optional should fail to rename, but StringRef and llvm::None should work (if you remove the hard-coded max limit 50).

[kbobyrev]

In D71110#1871896, @hokein wrote:

Thanks for the feedback!

Yeah, currently template classes are not supported in cross-file rename, see https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clangd/refactor/Rename.cpp#L187.

So llvm::Optional and llvm::Optional should fail to rename, but StringRef and llvm::None should work (if you remove the hard-coded max limit 50).

Ah, I see, thank you for the explanation! I think it would be super useful to have the error message reflecting that!

Also, the llvm::Twine fails too, many of the failures are related to the forward declarations (I think the problem is that they are treated differently from the original symbol and are not being renamed at all in the corresponding files, but that's just my guess). It`s probably a good idea to keep track of all the entries that can break global rename in some issue on the Github so that they're not lost?

[hokein]

Also, the llvm::Twine fails too, many of the failures are related to the forward declarations (I think the problem is that they are treated differently from the original symbol and are not being renamed at all in the corresponding files, but that's just my guess). It`s probably a good idea to keep track of all the entries that can break global rename in some issue on the Github so that they're not lost?

Yeap, that sounds good to me. Just create an issue, and put all details there, attach the detailed log would be helpful. I suspect we may encounter bugs in xrefs/rename code.

[hokein]

rebase to master.

clang-tools-extra/clangd/CMakeLists.txt

 add_subdirectory(refactor/tweaks)
 if (${CMAKE_SYSTEM_NAME} STREQUAL "Linux")
   # FIXME: Make fuzzer not use linux-specific APIs, build it everywhere.
   add_subdirectory(fuzzer)
 endif()
 add_subdirectory(tool)
 add_subdirectory(indexer)
 add_subdirectory(index/dex/dexp)
+add_subdirectory(eval-rename)
 
 if (LLVM_INCLUDE_BENCHMARKS)
   add_subdirectory(benchmarks)
 endif()
 if ( CLANGD_BUILD_XPC )
   add_subdirectory(xpc)
 endif ()
 
 if(CLANG_INCLUDE_TESTS)
 add_subdirectory(test)
 add_subdirectory(unittests)
 endif()

clang-tools-extra/clangd/eval-rename/CMakeLists.txt

+include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../)
+
+set(LLVM_LINK_COMPONENTS
+    Support
+    )
+
+add_clang_tool(clangd-rename
+  RenameMain.cpp
+  )
+
+clang_target_link_libraries(clangd-rename
+  PRIVATE
+  clangAST
+  clangBasic
+  clangFrontend
+  clangIndex
+  clangLex
+  clangTooling
+)
+target_link_libraries(clangd-rename
+  PRIVATE
+  clangDaemon
+)

clang-tools-extra/clangd/eval-rename/RenameMain.cpp

+//===--- RenameMain.cpp ------------------------------------------*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "CompileCommands.h"
+#include "Compiler.h"
+#include "FS.h"
+#include "FSProvider.h"
+#include "FindTarget.h"
+#include "GlobalCompilationDatabase.h"
+#include "Logger.h"
+#include "ParsedAST.h"
+#include "SourceCode.h"
+#include "index/MemIndex.h"
+#include "index/Serialization.h"
+#include "refactor/Rename.h"
+#include "clang/AST/Decl.h"
+#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/ArgumentsAdjusters.h"
+#include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Tooling/ReplacementsYaml.h"
+#include "llvm/ADT/STLExtras.h"
+
+static llvm::cl::opt<std::string>
+    IndexPath("index-path", llvm::cl::desc("Path to the index file"),
+              llvm::cl::init(""));
+static llvm::cl::opt<std::string>
+    RenameSymbol("rename-symbol",
+                 llvm::cl::desc("Qualified symbol name to rename"),
+                 llvm::cl::Required);
+static llvm::cl::opt<bool>
+    Fix("fix", llvm::cl::desc("Apply the rename edits to disk files"),
+        llvm::cl::init(false));
+
+namespace clang {
+namespace clangd {
+namespace {
+
+llvm::Expected<std::string> getAbsolutePath(StringRef File,
+                                            const FileSystemProvider &FS) {
+  SmallString<1024> AbsolutePath = File;
+  if (auto EC = FS.getFileSystem()->makeAbsolute(AbsolutePath))
+    return llvm::errorCodeToError(EC);
+  return clangd::removeDots(AbsolutePath.str());
+}
+
+class RenameExecutor {
+public:
+  explicit RenameExecutor(llvm::StringRef SymbolOnRename,
+                          llvm::StringRef FileOnRename,
+                          const FileSystemProvider &FSProvider,
+                          const GlobalCompilationDatabase &CDB,
+                          std::unique_ptr<SymbolIndex> Index)
+      : QualifiedName(SymbolOnRename), FileName(FileOnRename),
+        FSProvider(FSProvider), CDB(CDB), Index(std::move(Index)) {}
+
+  ParsedAST buildAST() {
+    ParseInputs PInputs;
+    PInputs.CompileCommand = *CDB.getCompileCommand(FileName);
+    auto Buffer = FSProvider.getFileSystem()->getBufferForFile(FileName);
+    llvm::StringRef Content = Buffer->get()->getBuffer();
+    PInputs.Contents = Content.str();
+    PInputs.FS = FSProvider.getFileSystem();
+    IgnoreDiagnostics Diags;
+
+    auto CI = buildCompilerInvocation(PInputs, Diags);
+    assert(CI && "Failed to build compilation invocation.");
+    auto Preamble =
+        buildPreamble(FileName, *CI,
+                      /*OldPreamble=*/nullptr,
+                      /*OldCompileCommand=*/PInputs.CompileCommand, PInputs,
+                      /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
+    auto AST = ::clang::clangd::buildAST(FileName, std::move(CI), {}, PInputs,
+                                         Preamble);
+    assert(AST && "Failed to build AST.");
+    return std::move(*AST);
+  }
+
+  llvm::Expected<FileEdits> execute(ParsedAST &AST) {
+    const auto *RenameDecl = findRenameDecl(AST, QualifiedName);
+    auto &SM = AST.getSourceManager();
+    assert(RenameDecl);
+
+    auto Pos =
+        clang::clangd::sourceLocToPosition(SM, RenameDecl->getLocation());
+    RenameOptions RenameOpts;
+    RenameOpts.AllowCrossFile = true;
+    RenameOpts.LimitFiles = 0; // unlimited.
+    RenameOpts.WantFormat = true;
+    clang::clangd::RenameInputs Inputs{Pos,      "_test_rename_", AST,
+                                       FileName, Index.get(), RenameOpts};
+    return clang::clangd::rename(Inputs);
+  }
+
+  const clang::NamedDecl *findRenameDecl(const clang::clangd::ParsedAST &AST,
+                                         llvm::StringRef QName) {
+    const NamedDecl *TargetDecl = nullptr;
+    findExplicitReferences(AST.getASTContext(), [&](ReferenceLoc Loc) {
+      if (!Loc.IsDecl)
+        return;
+      if (TargetDecl)
+        return;
+      for (const auto *D : Loc.Targets) {
+        if (const auto *ND = llvm::dyn_cast<NamedDecl>(D)) {
+          if (ND->getQualifiedNameAsString() == QName) {
+            TargetDecl = ND;
+            break;
+          }
+        }
+      }
+    });
+    return TargetDecl;
+  }
+
+private:
+  llvm::StringRef QualifiedName;
+  llvm::StringRef FileName;
+  const FileSystemProvider &FSProvider;
+  const GlobalCompilationDatabase &CDB;
+  std::unique_ptr<SymbolIndex> Index;
+};
+
+std::unique_ptr<SymbolIndex> openIndex(llvm::StringRef IndexPath) {
+  if (IndexPath.empty()) // create an empty one.
+    return clang::clangd::MemIndex::build({}, {}, {});
+  auto S = loadIndex(IndexPath, /*UseDex=*/true);
+  return S;
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
+
+int main(int Argc, const char **Argv) {
+  clang::tooling::CommonOptionsParser OP(Argc, Argv, llvm::cl::GeneralCategory);
+  assert(OP.getSourcePathList().size() == 1 && "Expect exactly one file path.");
+  clang::clangd::StreamLogger Logger(llvm::errs(),
+                                     clang::clangd::Logger::Error);
+  clang::clangd::LoggingSession LoggingSession(Logger);
+
+  clang::clangd::RealFileSystemProvider FSProvider;
+  clang::clangd::DirectoryBasedGlobalCompilationDatabase DCDB(llvm::None);
+  // inject the standard resource directory.
+  auto Mangler = clang::clangd::CommandMangler::detect();
+  clang::clangd::OverlayCDB OCDB(&DCDB, {},
+                                 clang::tooling::ArgumentsAdjuster(Mangler));
+
+  auto AbsFile =
+      clang::clangd::getAbsolutePath(OP.getSourcePathList()[0], FSProvider);
+  if (!AbsFile) {
+    clang::clangd::elog("Error on getting an absolute path {0}: {1}",
+                        OP.getSourcePathList()[0], AbsFile.takeError());
+    return 1;
+  }
+  clang::clangd::RenameExecutor RExecutor(RenameSymbol, *AbsFile, FSProvider,
+                                          OCDB,
+                                          clang::clangd::openIndex(IndexPath));
+  clang::clangd::ParsedAST AST = RExecutor.buildAST();
+  auto FatalDiag =
+      llvm::find_if(AST.getDiagnostics(), [](const clang::clangd::Diag &D) {
+        return D.Severity != clang::DiagnosticsEngine::Fatal;
+      });
+  if (FatalDiag != AST.getDiagnostics().end()) {
+    clang::clangd::elog("source file {0} is illformed: {1}",
+                        *AbsFile,
+                        FatalDiag->Message);
+    return 1;
+  }
+
+  auto RenameResults = RExecutor.execute(AST);
+  if (!RenameResults) {
+    clang::clangd::elog("Failed to rename: {0}", RenameResults.takeError());
+    return 1;
+  }
+
+  if (Fix) {
+    auto &SM = AST.getSourceManager();
+    clang::Rewriter Rewriter(SM, AST.getLangOpts());
+    bool Succeed = true;
+    for (const auto &R : *RenameResults) {
+      bool Success = clang::tooling::applyAllReplacements(
+          R.getValue().Replacements, Rewriter);
+      // Succeed &= Success;
+      if (!Success) {
+        clang::clangd::elog("Failed to apply replacements for file: {0}",
+                            R.first());
+        Succeed = false;
+        continue;
+      }
+    }
+    Rewriter.overwriteChangedFiles();
+    return Succeed ? 0 : 1;
+  }
+  // Emit the serialized rename edit (YAML format) to the stdout.
+  llvm::yaml::Output YAML(llvm::outs());
+  for (auto &RenameEditForFile : *RenameResults) {
+    clang::tooling::TranslationUnitReplacements Output;
+    Output.MainSourceFile = RenameEditForFile.first().str();
+    Output.Replacements = {RenameEditForFile.second.Replacements.begin(),
+                           RenameEditForFile.second.Replacements.end()};
+    YAML << Output;
+  }
+  return 0;
+}

clang-tools-extra/clangd/eval-rename/eval-rename.py

+#!/usr/bin/python
+"""
+A script to perform cross-file rename and evalute the results.
+
+Usage:
+
+$ cd <path>/llvm-project
+$ ninja -C build clangd-indexer
+$ ./build/bin/clangd-indexer -format=binary -executor=all-TUs . > llvm-index.idx
+$ ninja -C build clangd-rename
+$ clang-tools-extra/clangd/eval-rename/eval-rename.py --index=llvm-index.idx --file=clang-tools-extra/clangd/eval-rename/symbol_to_rename.txt
+"""
+
+from __future__ import print_function
+
+import argparse
+import os
+import re
+import subprocess
+import sys
+import tempfile
+
+RENAME_EXECUTOR='build/bin/clangd-rename'
+
+def Run(cmd):
+  s = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+  print('>', ' '.join(cmd))
+  [stdout, stderr] = s.communicate()
+  return (s.returncode, stdout, stderr)
+
+if __name__ == '__main__':
+  if not os.getcwd().endswith('llvm-project'):
+    sys.exit('The tool must be run from llvm-project/ root.')
+
+  ap = argparse.ArgumentParser()
+  ap.add_argument(
+      '-f',
+      '--file',
+      required=True,
+      help='A file containing rename symbols to be evaluted.'
+  )
+  ap.add_argument(
+      '-idx',
+      '--index',
+      required=True,
+      help='A path to the index file'
+  )
+  args = ap.parse_args()
+
+  with open(args.file) as f:
+    out = f.read()
+  test_cases = [line.strip().split() for line in out.split('\n') \
+     if line and not line.startswith('#') and line.strip()]
+
+  log_output_dir = tempfile.mkdtemp(prefix='eval-rename_')
+  success_cnt = 0
+  for file_name, rename_symbol, verify_target in test_cases:
+    Run(['git', 'reset', '--hard'])
+    execute_rename_cmd = [
+      RENAME_EXECUTOR,
+       '--index-path=%s' % args.index,
+       '--rename-symbol=%s' % rename_symbol,
+       '-fix',
+       file_name,
+    ]
+    rename_results = Run(execute_rename_cmd)
+    if rename_results[0] != 0:
+      log_file = open(os.path.join(
+        log_output_dir, rename_symbol.replace("::", "_") + '_RenameFailed'), 'w')
+      log_file.write(rename_results[1]) # stdout
+      log_file.write(rename_results[2]) # stderr
+      log_file.close()
+      continue
+
+    build_results = Run(['ninja', '-C', 'build', verify_target])
+    if build_results[0] != 0:
+      print('failed on renaming %s' % rename_symbol)
+      log_file = open(os.path.join(
+        log_output_dir, rename_symbol.replace("::", "_") + '_BuildFailed'), 'w')
+      log_file.write(build_results[1]) # stdout
+      log_file.write(build_results[2]) # stderr
+      log_file.close()
+      continue
+    print('succeed on renaming %s' % rename_symbol)
+    success_cnt += 1
+  Run(['git', 'reset', '--hard'])
+  print('Evaluated rename on %d symbols, %d symbol succeeded, go %s for failure logs' %
+      (len(test_cases), success_cnt, log_output_dir))

clang-tools-extra/clangd/eval-rename/symbol_to_rename.txt

+# normal functions
+clang-tools-extra/clangd/XRefs.h clang::clangd::findReferences clangd
+# normal class methods
+clang-tools-extra/clangd/index/Index.h clang::clangd::SwapIndex::reset clangd
+# normal classes
+clang-tools-extra/clangd/index/Index.h clang::clangd::SymbolIndex clangd
+clang-tools-extra/clangd/ClangdServer.h clang::clangd::ClangdServer clangd
+clang-tools-extra/clangd/GlobalCompilationDatabase.h clang::clangd::GlobalCompilationDatabase clangd
+clang-tools-extra/clangd/GlobalCompilationDatabase.h clang::clangd::OverlayCDB clangd
+clang-tools-extra/clangd/Protocol.h clang::clangd::CodeAction clangd
+# rename enum
+clang-tools-extra/clangd/index/Ref.h clang::clangd::RefKind clangd
+# rename enum constants
+clang-tools-extra/clangd/FindTarget.h clang::clangd::DeclRelation::Alias clangd
+# class with template constructors
+clang-tools-extra/clangd/index/MemIndex.h clang::clangd::MemIndex clangd
 No newline at end of file