[clangd] Use dirty filesystem when performing cross file tweaks

[njames93]

Summary

Cross file tweaks can now use the dirty buffer contents easily when performing cross file effects.
This can be noted on the DefineOutline tweak, now working when the target file is unsaved

[sammccall]

Ah, thanks for working on this!

A few thoughts:

• when we're pseudoparsing the file we're going to modify as we do here, using the new content is strictly better, no downside :-)
• the old method here of accessing the content through the FS attached to the AST is a hack
• however I think DraftStore is the wrong abstraction here and VFS is the right one.
  • each tweak shouldn't have to deal with falling back from DraftStore to VFS (and rename also has an implementation of this!)
  • C++ embedders don't have a DraftStore lying around
  • bug: this code won't find a named but unsaved implementation file (since getCorrespondingHeaderOrSource uses VFS)
• (also Tweak::Selection isn't a pure abstraction, we can jam the FS in there instead of passing it around as a separate param everywhere)

So I think the right way to expose this to tweaks is to add a vfs::FileSystem * member to Tweak::Selection, that allows accessing the current FS (as opposed to the one used for building).
The simplest implementation uses OverlayVFS + InMemoryFS + TUScheduler::getAllFileContents(). This isn't as expensive as it sounds as we need only set the FS pointer for apply().
(We can also implement a FS that copies more lazily, though I suspect it's not worth it)

[njames93]

In D93978#2479440, @sammccall wrote:

Ah, thanks for working on this!

A few thoughts:

• when we're pseudoparsing the file we're going to modify as we do here, using the new content is strictly better, no downside :-)
• the old method here of accessing the content through the FS attached to the AST is a hack
• however I think DraftStore is the wrong abstraction here and VFS is the right one.
  • each tweak shouldn't have to deal with falling back from DraftStore to VFS (and rename also has an implementation of this!)
  • C++ embedders don't have a DraftStore lying around
  • bug: this code won't find a named but unsaved implementation file (since getCorrespondingHeaderOrSource uses VFS)
• (also Tweak::Selection isn't a pure abstraction, we can jam the FS in there instead of passing it around as a separate param everywhere)

So I think the right way to expose this to tweaks is to add a vfs::FileSystem * member to Tweak::Selection, that allows accessing the current FS (as opposed to the one used for building).
The simplest implementation uses OverlayVFS + InMemoryFS + TUScheduler::getAllFileContents(). This isn't as expensive as it sounds as we need only set the FS pointer for apply().
(We can also implement a FS that copies more lazily, though I suspect it's not worth it)

I'm getting a little stuck here. I've made a OverlayFS which has the a TFS::view as the base, and an InMemoryFS containing the contents of the TUScheduler. I Passed that in the Selection and it all worked.
However once I changed the getSourceFile function to also use that same FS, it would find the file ok, but then clangd would crash(with no visible trace) when it tried to open the file.

auto Buffer = FS.getBufferForFile(*CCFile); // Here

From what I can getCorrespondingSourceOrHeader is querying the InMemoryFS and maybe those calls to FS::exists are somehow corrupting something.
Whatever it is, it all seems a bit messed up to me.

I'm gonna try with a debug build (and possibly asan) to see If I could possibly get anything more.

scratch that, asan told me that the OverlayFS was getting destroyed as I had it stored as a unique_ptr then it was copied into a RefCntPtr and subsequently destroyed in there

[njames93]

Use VFS instead of DraftStore

[sammccall]

Thanks, this looks much better!

clang-tools-extra/clangd/ClangdServer.cpp

434 ↗

(On Diff #315070)

nit: pass nullptr explicitly rather than as a default arg.

clang-tools-extra/clangd/TUScheduler.cpp

1462 ↗

(On Diff #315070)

nit: say why it's needless: usually 0-1 buffer is ever read.

clang-tools-extra/clangd/TUScheduler.h

239 ↗

(On Diff #315070)

FIXME to remove this?

243 ↗

(On Diff #315070)

I do think this is the best option, but just wanted to spell out the implications to make sure we're on the same page...

• edits that occur while the action is queued will not be seen. (This is bad for the use cases we know of, but minor)
• the optimization of deferring buffer copies (hinted to in the implementation) would actually break the contract, as you may see a *newer* version of the file
• we don't have to worry about locking inside TUScheduler to grab the buffers

246 ↗

(On Diff #315070)

there's no need for both of these to be methods on TUScheduler, pick one?

(I'd lean towards exposing this one only as it simplifies callers and has a better name)

248 ↗

(On Diff #315070)

nit: take const TFS& as arg, as this function is designed for caller convenience and that's always how it'll be used

248 ↗

(On Diff #315070)

nit: return unique_ptr<FileSystem> (which can implicitly convert)

clang-tools-extra/clangd/refactor/Tweak.h

53 ↗

(On Diff #315070)

VFS is a public field, and optional, so we could omit it from the constructor and assign it explicitly instead.

69 ↗

(On Diff #315070)

let's be a bit more explicit here.

/// File system used to access source code (for cross-file tweaks).
/// This overlays the "dirty" contents of files open in the editor, which (in case of headers)
/// may not match the saved contents used for building the AST.

69 ↗

(On Diff #315070)

mention that this is only populated for apply(), not prepare()?

70 ↗

(On Diff #315070)

nit: FS rather than VFS
(the "V" is already implied by the presence of the object)

clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp

410

It's not actually optional for apply, right? We shouldn't need this fallback.

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

206 ↗

(On Diff #315070)

hmm, I see.

What if we make the Selection constructor fill in the default FS from AST, and then just overwrote the public field in ClangdServer::applyTweak?

Then we get the FS always being set (good for tweaks), simple usage here or in tests, and the expensive copy is still explicit...

[njames93]

clang-tools-extra/clangd/TUScheduler.h

248 ↗

(On Diff #315070)

I think the flexibility of passing a Filesystem is better than the ThreadsafeFS.

clang-tools-extra/clangd/refactor/Tweak.h

53 ↗

(On Diff #315070)

We could, but it would messy code elsewhere.

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

206 ↗

(On Diff #315070)

I've gone with an approach that the FS gets passed in the constructor, if nullptr is passed then we use the fallback, helpful for tests

[njames93]

Address most comments.

[njames93]

So I did some tweaking and I have a snapshot filesystem that will hold shared references to the contents, thereby avoiding the need to copy the file contents.
It works for rename and tweaks with no visible issues.
Reasoning behind this a long term goal of letting a user choose between using on disk or dirty buffers when building preambles.
Obviously there, if using dirty buffers, we wouldn't want to be copying every open file (even unused files) during preamble building/checking.

[njames93]

I've decided I much prefer the implementation in D94554, So once thats cleaned up and split up, I'll merge changes in here with there.

[njames93]

Change implementation to use DirtyFS instead.

[njames93]

Fixed last series being a little corrupted.

[njames93]

Rebased.

[njames93]

clang-tools-extra/clangd/ClangdServer.cpp

575 ↗

(On Diff #329649)

Regarding this, Is it wise to set the contract so that Tweak::prepare isn't allowed to do IO so we should just pass a nullptr here, inline with how prepareRename works.

[sammccall]

Sorry, I thought this had already gone in!

LG, sorry for the back and forth.

clang-tools-extra/clangd/ClangdServer.cpp

575 ↗

(On Diff #329649)

Yes, I think so.

clang-tools-extra/clangd/refactor/Tweak.cpp

54 ↗

(On Diff #329649)

I'm no longer convinced this fallback is a good idea.
We want prepare() to do no IO, ideally we'd assert if we do. But by falling back to the AST FS, we'll mask that problem.

There are only two callsites that need this fallback (TweakTesting & Check) - I think we should just inline what we mean there.

clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp

72

why take this as an explicit parameter rather than just accessing (and asserting) Sel.FS?

[njames93]

Address comments.

[njames93]

Fix up compilation failures and test failures.

clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp

 //===--- DefineOutline.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 "AST.h"
+#include "DraftStore.h"
 #include "FindTarget.h"
 #include "HeaderSourceSwitch.h"
 #include "ParsedAST.h"
 #include "Selection.h"
 #include "SourceCode.h"
 #include "refactor/Tweak.h"
 #include "support/Logger.h"
 #include "support/Path.h"

 #include "clang/Tooling/Core/Replacement.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include <cstddef>
 #include <string>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 // Deduces the FunctionDecl from a selection. Requires either the function body
const FunctionDecl *getSelectedFunction(const SelectionTree::Node *SelNode) {
   return nullptr;
 }
 
 llvm::Optional<Path> getSourceFile(llvm::StringRef FileName,
                                    const Tweak::Selection &Sel) {
   if (auto Source = getCorrespondingHeaderOrSource(
           std::string(FileName),
           &Sel.AST->getSourceManager().getFileManager().getVirtualFileSystem()))
     return *Source;
   return getCorrespondingHeaderOrSource(std::string(FileName), *Sel.AST,
                                         Sel.Index);
 }
 
 // Synthesize a DeclContext for TargetNS from CurContext. TargetNS must be empty
 // for global namespace, and endwith "::" otherwise.
 // Returns None if TargetNS is not a prefix of CurContext.
 llvm::Optional<const DeclContext *>
bool prepare(const Selection &Sel, const DraftStore &) override {
     }
 
     // Note that we don't check whether an implementation file exists or not in
     // the prepare, since performing disk IO on each prepare request might be
     // expensive.
     return true;
   }
 
-  Expected<Effect> apply(const Selection &Sel, const DraftStore &) override {
+  Expected<Effect> apply(const Selection &Sel,
+                         const DraftStore &DraftMgr) override {
     const SourceManager &SM = Sel.AST->getSourceManager();
     auto MainFileName =
         getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
     if (!MainFileName)
       return error("Couldn't get absolute path for main file.");
 
     auto CCFile = getSourceFile(*MainFileName, Sel);
     if (!CCFile)
       return error("Couldn't find a suitable implementation file.");
 
+    StringRef Contents;
+    // We need to keep both these alive as Contents can reference the storage
+    // contained in either of them.
+    auto Draft = DraftMgr.getDraft(*CCFile);
+    std::unique_ptr<llvm::MemoryBuffer> Buffer;
+    if (Draft) {
+      Contents = Draft->Contents;
+    } else {
       auto &FS =
           Sel.AST->getSourceManager().getFileManager().getVirtualFileSystem();
-    auto Buffer = FS.getBufferForFile(*CCFile);
+      auto BufferOr = FS.getBufferForFile(*CCFile);
       // FIXME: Maybe we should consider creating the implementation file if it
       // doesn't exist?
-    if (!Buffer)
-      return llvm::errorCodeToError(Buffer.getError());
-    auto Contents = Buffer->get()->getBuffer();
+      if (!BufferOr)
+        return llvm::errorCodeToError(BufferOr.getError());
+      Buffer = std::move(*BufferOr);
+      Contents = Buffer->getBuffer();
+    }
     auto InsertionPoint = getInsertionPoint(
         Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts());
     if (!InsertionPoint)
       return InsertionPoint.takeError();
 
     auto FuncDef = getFunctionSourceCode(
         Source, InsertionPoint->EnclosingNamespace, Sel.AST->getTokens());
     if (!FuncDef)