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

[Nathan James via Phabricator via cfe-commits]

njames93 marked an inline comment as done.
njames93 added inline comments.


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:555
+    auto Insert = Lexer::getSourceText(FixIt.InsertFromRange, M, L, &Invalid);
+    if (!Invalid)
+      Result.newText = Insert.str();
----------------
kadircet wrote:
> njames93 wrote:
> > kadircet wrote:
> > > 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.
> > 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. 
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97123/new/

https://reviews.llvm.org/D97123