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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet 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();
----------------
njames93 wrote:
> 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.
> 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`.


================
Comment at: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp:805
 
+Expected<SourceRange> sourceRangeInMainFile(SourceManager &SM, Range R) {
+  if (auto Beg = sourceLocationInMainFile(SM, R.start)) {
----------------
sorry if i was vague in preivous comment, i was suggesting making this function return a SourceRange all the time by wrapping `sourceLocationInMainFile`s with `cantFail`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97123