[clangd] Support FixIts that use InsertFromRange instead of inserting raw text

[njames93]

Summary

Currently clangd will ignore FixItHint::InsertFromRange when computing edits.
This leads to malformed fixes for diagnostics that choose to use it.
In this patch we will try to grab source text if CodeToInsert is empty.

[kadircet]

mostly LG, can you also add some tests?

clang-tools-extra/clangd/Diagnostics.cpp

695

nit: I would rather make !Invalid a condition and just make use of it in here as well.

clang-tools-extra/clangd/SourceCode.cpp

555

it feels like correct semantics would be to make this function fail now. as the resulting value will likely be used for editing the source code and we don't want to propagate some garbage.

[kadircet]

clang-tools-extra/clangd/SourceCode.cpp

552

oh btw, i think the condition should only check for validness of the InsertFromRange, and then we should assert for the emptyness of CodeToInsert. (same for the piece in Diagnostics.cpp)

[njames93]

clang-tools-extra/clangd/Diagnostics.cpp

695

Good point, that's much nicer.

clang-tools-extra/clangd/SourceCode.cpp

555

While I agree with this in principal. We currently don't handle the case where RemoveRange doesn't correspond to a FileCharRange which would likely need propagating. Though likely in a separate patch.

[njames93]

Sort of added tests. Though they seem hacked in and I can't find a simple way to test the synthetic diagnostic messages.

[kadircet]

clang-tools-extra/clangd/SourceCode.cpp

555

i discussed the situation with sam offline (as you've also noticed most of the sourcelocation -> lsp conversations within this file doesn't really check for errors), and it looks like this was intentional. we are trying to cover as much ground as possible in diagnostics.cpp, especially in https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/Diagnostics.cpp#L677. we should do the same for InsertFromRange. all the macroarg logic isn't necessary (until we notice it is), so just bailing out when the InsertFromRange is a macroid or outside mainfile in AddFix should be enough.

as for testing it, looks like you can invoke a fixit with InsertFromRange via:

struct Foo {};
struct Bar : public [[noreturn]] Foo {};

it would be nice to also check with an attribute coming from a macro expansions.

clang-tools-extra/clangd/unittests/SourceCodeTests.cpp

826 ↗

(On Diff #325515)

nit: I would just wrap these in llvm::cantFail, similarly for sourceRangeInMainFile. these are just tests, and we don't really expect them to be outside mainfile.

[njames93]

Update tests.

[njames93]

clang-tools-extra/clangd/SourceCode.cpp

555

Can you elaborate on why we should disregard InsertFromRange if it points outside the main file. That seems like an unnecessary restriction. It's probably also safe if its a macro ID, though there's more likely to be some edge case there.

As for the diagnostic. I don't think there's any value testing that. The test in SourceCodeTests covers inserting from range pretty well. It would be nice to test the synthetic messages, but I don't think any diagnostic in clang/clang-tidy contains just 1 fix-it that's an InsertFromRange.

[kadircet]

clang-tools-extra/clangd/SourceCode.cpp

555

Can you elaborate on why we should disregard InsertFromRange if it points outside the main file. That seems like an unnecessary restriction.

because we build preamble and main file separately, the sourcemanager is likely missing files apart from the main file. hence when one tries to access a sourcelocation in them, the file will be re-read from disk and if contents on disk change for some reason, clangd might crash when the offsets are off.

It's probably also safe if its a macro ID, though there's more likely to be some edge case there.

safeness is a little wrinkly here due to the edge cases (that are hard to predict, until we see them in practice) you point out as well. but moreover, do we really want to introduce a piece of text coming from a macro expansion into source code? is there any use cases for that? (i'd say it should be the macro name that should be inserted, not its expansion.)

As for the diagnostic. I don't think there's any value testing that.

i wasn't suggesting to introduce a diagnostic test to check the behaviour of toTextEdit. it was for testing the new logic we'll introduce into diagnostics.cpp, sorry if i wasn't clear:/. the tests you currently have are totally fine for testing toTextEdit.

clang-tools-extra/clangd/unittests/SourceCodeTests.cpp

805 ↗

(On Diff #325826)

sorry if i was vague in preivous comment, i was suggesting making this function return a SourceRange all the time by wrapping sourceLocationInMainFiles with cantFail.

clang-tools-extra/clangd/Diagnostics.cpp

auto AddFix = [&](bool SyntheticMessage) -> bool {
     llvm::SmallString<64> Message;
     // If requested and possible, create a message like "change 'foo' to 'bar'".
     if (SyntheticMessage && FixIts.size() == 1) {
       const auto &FixIt = FixIts.front();
       bool Invalid = false;
       llvm::StringRef Remove =
           Lexer::getSourceText(FixIt.RemoveRange, SM, *LangOpts, &Invalid);
       llvm::StringRef Insert = FixIt.CodeToInsert;
+      if (Insert.empty() && FixIt.InsertFromRange.isValid()) {
+        bool InvalidInsert = false;
+        Insert = Lexer::getSourceText(FixIt.InsertFromRange, SM, *LangOpts,
+                                      &InvalidInsert);
+        Invalid |= InvalidInsert;
+      }
       if (!Invalid) {
         llvm::raw_svector_ostream M(Message);
         if (!Remove.empty() && !Insert.empty()) {
           M << "change '";
           writeCodeToFixMessage(M, Remove);
           M << "' to '";
           writeCodeToFixMessage(M, Insert);
           M << "'";

clang-tools-extra/clangd/SourceCode.cpp

 }
 
 TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M,
                     const LangOptions &L) {
   TextEdit Result;
   Result.range =
       halfOpenToRange(M, Lexer::makeFileCharRange(FixIt.RemoveRange, M, L));
   Result.newText = FixIt.CodeToInsert;
+  if (Result.newText.empty() && FixIt.InsertFromRange.isValid()) {
+    bool Invalid = false;
+    auto Insert = Lexer::getSourceText(FixIt.InsertFromRange, M, L, &Invalid);
+    if (!Invalid)
+      Result.newText = Insert.str();
+  }
   return Result;
 }
 
 FileDigest digest(llvm::StringRef Content) {
   uint64_t Hash{llvm::xxHash64(Content)};
   FileDigest Result;
   for (unsigned I = 0; I < Result.size(); ++I) {
     Result[I] = uint8_t(Hash);