This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Use dirty filesystem when performing cross file tweaks
ClosedPublic

Authored by njames93 on Jan 2 2021, 10:14 AM.

Details

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

Diff Detail

Event Timeline

njames93 created this revision.Jan 2 2021, 10:14 AM
njames93 requested review of this revision.Jan 2 2021, 10:14 AM

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 added a comment.EditedJan 6 2021, 6:52 PM

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 updated this revision to Diff 315070.Jan 7 2021, 1:55 AM

Use VFS instead of DraftStore

njames93 edited the summary of this revision. (Show Details)Jan 7 2021, 1:55 AM

Thanks, this looks much better!

clang-tools-extra/clangd/ClangdServer.cpp
553

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
55

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

71

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.
71

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

72

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

clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
415–416

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

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

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 marked 9 inline comments as done.Jan 7 2021, 1:59 PM
njames93 added inline comments.
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
55

We could, but it would messy code elsewhere.

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

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 updated this revision to Diff 315235.Jan 7 2021, 2:01 PM
njames93 marked an inline comment as done.

Address most comments.

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 planned changes to this revision.Jan 19 2021, 3:48 PM

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 updated this revision to Diff 317862.Jan 20 2021, 6:27 AM

Change implementation to use DirtyFS instead.

njames93 updated this revision to Diff 317864.Jan 20 2021, 6:33 AM

Fixed last series being a little corrupted.

njames93 retitled this revision from [clangd] DefineOutline doesn't require implementation file being saved to [clangd] Use dirty filesystem when performing cross file tweaks.
njames93 edited the summary of this revision. (Show Details)
njames93 added inline comments.Mar 10 2021, 6:38 AM
clang-tools-extra/clangd/ClangdServer.cpp
584

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 accepted this revision.Apr 14 2021, 12:44 AM

Sorry, I thought this had already gone in!

LG, sorry for the back and forth.

clang-tools-extra/clangd/ClangdServer.cpp
584

Yes, I think so.

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

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
68

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

This revision is now accepted and ready to land.Apr 14 2021, 12:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2021, 12:44 AM
njames93 updated this revision to Diff 338294.Apr 17 2021, 4:50 AM
njames93 marked 4 inline comments as done.

Address comments.

njames93 updated this revision to Diff 338518.Apr 19 2021, 7:29 AM

Fix up compilation failures and test failures.