Page MenuHomePhabricator

[clangd] Implemented DraftFileSystem
Changes PlannedPublic

Authored by LutsenkoDanil on Nov 3 2018, 12:36 PM.

Details

Summary

Patch changes behavior of preamble building. If headers was changed in editor, preamble will be built with draft versions of them.

Diff Detail

Event Timeline

LutsenkoDanil created this revision.Nov 3 2018, 12:36 PM

Thanks for the patch! I believe many people I talked to want this behavior (myself included).
Some people like what we do now more. It feels like it depends on the workflow: for people who auto-save *all* files before build (some editors do it automatically?) the new behavior is the right one, for people who only save current file and rerun build on the console the old behavior is the correct one.
It all boils down to the argument "I want to see same errors that running the compiler would produce".

@klimek, @arphaman, @simark, WDYT?

clangd/FS.h
82

This assumes the Path is absolute and vfs::FileSystem can be called with non-absolute paths too.
One way to make it work with relative paths is to create an InMemoryFileSystem with the headers (it handles the absolute paths conversions) and create an OverlayFileSystem on top of it and the RealFileSystem.

This would also make more complicated things work, e.g. getting the same files when traversing parent directories, etc.

Could we try this approach? WDYT?

klimek added a comment.Nov 5 2018, 4:15 AM

Thanks for the patch! I believe many people I talked to want this behavior (myself included).
Some people like what we do now more. It feels like it depends on the workflow: for people who auto-save *all* files before build (some editors do it automatically?) the new behavior is the right one, for people who only save current file and rerun build on the console the old behavior is the correct one.
It all boils down to the argument "I want to see same errors that running the compiler would produce".

@klimek, @arphaman, @simark, WDYT?

I'm in yet another camp: I carefully save when I have something that is correct enough syntax, so I only want errors from with changes from the exact file I'm editing and the rest of the files in saved state.

Disclaimer: I'm on a train with family today, and haven't actually read the
patch...

So I have concerns :-)

  1. There's the usual concern that the current behavior is reasonable and

people like it, so adding a second reasonable behavior provides a small
amount of value to the userbase as a whole (because 99%+ will use the
default). If the benefit is small, the comparison to support cost may be
unfavorable.

  1. This needs to invalidate preambles more often, which brings both

performance and complexity questions. E.g. we will at some point invalidate
preambles and regenerate diagnostics based on file writes. After this
change, we'll be obligated to do so for edits too. (Remember, LSP has no
concept of "the foreground file"). Invalidating a preamble is expensive,
and edits come rapidly and may invalidate multiple TUs. The current
behavior seems more compatible with being both simple and fast.

So mostly I'd like to be convinced that this is important (or that it's
simple, but that seems unlikely at first glance).

Cheers, Sam

There's the usual concern that the current behavior is reasonable and people like it.

I think it would be reasonable to say that a large portion of C++ users are used to the behavior that this patch introduces.
Specifically, all IDEs (Clion, Eclipse, Visual Studio for certain, not sure about XCode) will see the contents of the headers that the user typed and not the other way around.
It does not cause any inconsistencies there, the IDE will save all your open files when you hit "build". That also means reading files from the filesystem gives non-useful errors in that case, because the build will actually see the files from the editors.

LutsenkoDanil planned changes to this revision.Nov 5 2018, 6:56 AM

@ilya-biryukov, For example, VSCode saves all changed files by default when you press Ctrl+Shift+B (if build task configured for project).

@klimek If behavior will be configurable, is it ok for you?

@sammccall Current behavior may confuse new users, since, other IDEs mostly (all?) shows diagnostics for edited files instead of saved one. And it's unexpected that headers have 2 states - visible and saved (which cannot be viewed in IDE at all). Looks like performance will be same like usage of 'auto save after delay' feature in editor, if we make debounce delay configurable, what do you think?

I suggest inroduce following changes to the patch:

  • Changes proposed by @ilya-biryukov
  • Configurable behavior
  • Debounce timeout option
clangd/FS.h
82

Sounds good. Thank you for review!

klimek added a comment.Nov 5 2018, 7:05 AM

@klimek If behavior will be configurable, is it ok for you?

I have the same concerns as Sam for making this an option.

@sammccall Current behavior may confuse new users, since, other IDEs mostly (all?) shows diagnostics for edited files instead of saved one. And it's unexpected that headers have 2 states - visible and saved (which cannot be viewed in IDE at all). Looks like performance will be same like usage of 'auto save after delay' feature in editor, if we make debounce delay configurable, what do you think?

don't most IDEs show whether a file is saved or just modified?

simark added a comment.Nov 5 2018, 7:26 AM

I'm in yet another camp: I carefully save when I have something that is correct enough syntax, so I only want errors from with changes from the exact file I'm editing and the rest of the files in saved state.

That sounds like the current behavior, doesn't it?

Personally, I would like the behavior proposed by this patch. I think it perfectly in line with the idea of showing the same diagnostics as the compiler would produce. It's just that it's what the compiler would produce if compiling the files as seen in the editor.

Also, in the LSP, after a didOpen and until the corresponding didClose, the source of truth for that file is supposed to be what is in the editor buffer. So I think it would make sense that if other files include the edited file, they should see the content from the editor.

Of course, that is dependent of having an efficient cancelling mechanism. Even with some debouncing, an update to a header file can trigger the re-processing of many TUs, so if I do another edit to the same header shortly after, all the queued jobs from the first edit should be dropped. But I think we already have that, don't we?

I don't think either point 1 or 2 above have been addressed, and so I'm not comfortable moving forward with this change.

I think it would be reasonable to say that a large portion of C++ users are used to the behavior that this patch introduces.

I agree, the new behavior is reasonable. The old behavior is also reasonable. This is the basis for point 1 above.

Current behavior may confuse new users, since, other IDEs mostly (all?) shows diagnostics for edited files instead of saved one. And it's unexpected that headers have 2 states - visible and saved (which cannot be viewed in IDE at all).

Maybe part of the issue is culture clash between "IDEs" and "editors" - the distinction is somewhat artificial, but I think the current behavior in editors is somewhat traditional, and certainly the two-states is what editor users expect (the saved state is what you get when you run the compiler).

Looks like performance will be same like usage of 'auto save after delay' feature in editor, if we make debounce delay configurable, what do you think?

I think there will be substantial performance loss *and* additional complexity from this mode once we start updating diagnostics on file change. (Point 2 above).
Additionally, if this needs to be configurable, that adds further complexity.

Of course, that is dependent of having an efficient cancelling mechanism. Even with some debouncing, an update to a header file can trigger the re-processing of many TUs, so if I do another edit to the same header shortly after, all the queued jobs from the first edit should be dropped. But I think we already have that, don't we?

Not for preambles (as we don't yet even update diagnostics on file events), and that case is significantly more complicated than main-files:

  • there are multiple TUs to consider as you say
  • preambles can be 100x more expensive than mainfiles
  • this affects background files, so is "usually" unimportant work
  • this happens while latency-sensitive operations like code-complete happen in the header file itself

Someone mentioned to me that the interaction-between-features argument wasn't clear here:

  • we don't currently update diagnostics for A.cc when A.h is edited
  • we should, this seems more obvious & important than what we do with drafts
  • this interacts badly with using draft state, as this patch proposes - there are too many edits
ioeric added a comment.Nov 6 2018, 1:31 AM

Someone mentioned to me that the interaction-between-features argument wasn't clear here:

  • we don't currently update diagnostics for A.cc when A.h is edited
  • we should, this seems more obvious & important than what we do with drafts
  • this interacts badly with using draft state, as this patch proposes - there are too many edits

FWIW, one of my "pain points" when using vim+clangd is:

  • Edit A.h in a buffer (and forget to save)
  • Switch to A.cc in another buffer
  • Realize that I forgot to save A.h
  • Go back to save A.h
  • Jump back to A.cc file.

I would be very happy if A.cc can see the unsaved A.h when I am editing A.cc. I think the update should be relatively less frequent with this approach, as people don't usually update multiple files at the same time. Not sure if I want all files that depend on A.h to be updated when I edit A.h though; it seems much more complicated and less important (to me).

klimek added a comment.Nov 6 2018, 1:52 AM

Someone mentioned to me that the interaction-between-features argument wasn't clear here:

  • we don't currently update diagnostics for A.cc when A.h is edited
  • we should, this seems more obvious & important than what we do with drafts
  • this interacts badly with using draft state, as this patch proposes - there are too many edits

FWIW, one of my "pain points" when using vim+clangd is:

  • Edit A.h in a buffer (and forget to save)
  • Switch to A.cc in another buffer
  • Realize that I forgot to save A.h
  • Go back to save A.h
  • Jump back to A.cc file.

    I would be very happy if A.cc can see the unsaved A.h when I am editing A.cc. I think the update should be relatively less frequent with this approach, as people don't usually update multiple files at the same time. Not sure if I want all files that depend on A.h to be updated when I edit A.h though; it seems much more complicated and less important (to me).

I can see this is a problem when you forget to save. On the other hand, one case I encounter frequently enough is:
edit A.h, save
go to B.cc (that includes it), see errors that it causes (here and in other files)
go to A.h, fix error, save
go to B.cc (or a different file that includes A.h) and wanting to see the error gone

nik added a subscriber: nik.Nov 6 2018, 1:55 AM

I would be very happy if A.cc can see the unsaved A.h when I am editing A.cc.

We have that in Qt Creator (with libclang) and that's quite handy as it can save you some build cycles.

Not sure if I want all files that depend on A.h to be updated when I edit A.h though; it seems much more complicated and less important (to me).

If it helps (and the LSP allows it): In case A.h is edited, we flag it dirty. If the user makes some file depending on it visible (or the current file), we trigger a reparse for that. Not sure whether the LSP has a notion of current or visible files...

nik added a comment.Nov 6 2018, 1:59 AM
In D54077#1288413, @nik wrote:

If it helps (and the LSP allows it): In case A.h is edited, we flag it dirty. If the user makes some file depending on it visible (or the current file), we trigger a reparse for that. Not sure whether the LSP has a notion of current or visible files...

Huch..., I meant that we flag the files dirty that depend on A.h.

@sammccall Thank you for detailed explanation!

Someone mentioned to me that the interaction-between-features argument wasn't clear here:

  • we don't currently update diagnostics for A.cc when A.h is edited
  • we should, this seems more obvious & important than what we do with drafts
  • this interacts badly with using draft state, as this patch proposes - there are too many edits

I already made a patch which introduces such behavior (not uploaded here yet), and looks like it works well with draft fs: diagnostics updates for depended files in 'real-time' on typing for opened files and no seen performance glitches, in multi-threaded mode at least.
I suggest continue discussion when/if dependencies tracking will be implemented and real performance reduce introduced by this patch can be checked with real code.

don't most IDEs show whether a file is saved or just modified?

They do, but whenever you run the build from them, they will save all modified files before actually running it.

I already made a patch which introduces such behavior (not uploaded here yet), and looks like it works well with draft fs: diagnostics updates for depended files in 'real-time' on typing for opened files and no seen performance glitches, in multi-threaded mode at least.
I suggest continue discussion when/if dependencies tracking will be implemented and real performance reduce introduced by this patch can be checked with real code.

Exciting! Please send it out! I'm also starting to look into this, but my work is mostly focused on watching files in the filesystem.

I already made a patch which introduces such behavior (not uploaded here yet), and looks like it works well with draft fs: diagnostics updates for depended files in 'real-time' on typing for opened files and no seen performance glitches, in multi-threaded mode at least.

This does indeed sound exciting! I'm a little skeptical that it "just works", there are some tricky cases:

  • TUs where the preamble takes multiple seconds to rebuild
  • "thundering herd" problems if several TUs are active that include the edited file
  • redundant background work does have costs (e.g. battery)

If it helps (and the LSP allows it): In case A.h is edited, we flag it dirty. If the user makes some file depending on it visible (or the current file), we trigger a reparse for that. Not sure whether the LSP has a notion of current or visible files...

It doesn't. Maybe there's a way to make this idea work anyway (nothing comes to mind).

I suggest continue discussion when/if dependencies tracking will be implemented and real performance reduce introduced by this patch can be checked with real code.

Sounds reasonable. I think maybe the shortest path there is to implement on top of the LSP file change notifications, and have it push a "maybe rebuild" item on every TUScheduler worker.