[PATCH] D54796: [clangd] C++ API for emitting file status

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry for the delays. Not sure about emitting `Queued` on the main thread, see the corresponding comment. Otherwise looks very good.



================
Comment at: clangd/ClangdServer.h:49
+  /// Called whenever the file status is updated.
+  virtual void onFileUpdated(PathRef File, const TUStatus &Status){};
 };
----------------
hokein wrote:
> ilya-biryukov wrote:
> > hokein wrote:
> > > ilya-biryukov wrote:
> > > > Have we thought about the way we might expose statuses via the LSP?
> > > > The `TUStatus` seems a bit too clangd-specific (i.e. `BuildingPreamble`, `ReuseAST` is not something that makes sense in the protocol). Which might be fine on the `ClangdServer` layer, but it feels like we should generalize before sending it over via the LSP
> > > > The TUStatus seems a bit too clangd-specific (i.e. BuildingPreamble, ReuseAST is not something that makes sense in the protocol). 
> > > 
> > > Yes, this is by design. `TUStatus` presents the internal states of clangd, and would not be exposed via LSP.
> > > 
> > > Some ways of exposing status via the LSP
> > > - define a separate structure e.g. `FileStatus` in LSP,  render `TUStatus` to it, and sending it to clients (probably we might want to add a new extension `textDocument/filestatus`)
> > > - reuse some existing LSP methods (`window/showMessage`, `window/logMessage`), render `TUStatus` to one of these methods' params.
> > LG, I believe the `BuildingPreamble` status in particular might be useful on the C++ API level for us.
> > Not sure `showMessage` or `logMessage` is a good idea, the first one will definitely be annoying if it'll be popping up all the time. Having a new method in the LSP LG.
> `showMessage` seems work well via vim YCM, but it is annoying in vscode (as it pops up diags) -- we need to customize this behavior in our own extensions (showing messages in status bar). We need to revisit different ways. 
I'd err on the side of getting a new LSP method/extension that's supposed to show messages in the status bar.
It seems `showMessage` was specifically designed for user interactions and not status bar updates. But that's out of the scope of this patch, so definitely not blocking it.


================
Comment at: clangd/TUScheduler.cpp:268
+  /// Status of the TU.
+  TUStatus Status; /* GUARDED_BY(DiagMu) */
 };
----------------
hokein wrote:
> ilya-biryukov wrote:
> > Is `Status` actually ever read from multiple threads?
> > I assume it's only accessed on the worker thread, right? I think we can leave out the locks around it and put beside other fields only accessed by the worker thread, i.e. `DiagsWereReported`, etc.
> > 
> > PS Sorry go going back and forth, I didn't think about it in the previous review iteration.
> Unfortunately not, most statuses are emitted via worker thread, but we emit `Queued` status in the main thread...
Hm, that sounds surprising.
What if the rebuild is in progress on the worker thread and we emit the queued status on the main thread at the same time? We might get weird sequences of statuses, right? 
E.g. `Queued → BuildingPreamble → [new update on the main thread] Queued → [processing request on a worker thread] RunningAction('XRefs')`

The `Queued` request status between `BuildingPreamble` and `RunningAction` looks **really** weird.
Maybe we could avoid that by setting emitting the `Queued` status on the worker thread whenever we're about to sleep on the debounce timeout?


================
Comment at: clangd/TUScheduler.cpp:636
+      if (Requests.empty())
+        emitTUStatus({TUAction::Idle, /*Name*/ ""});
     }
----------------
hokein wrote:
> ilya-biryukov wrote:
> > Maybe do it outside the lock? The less dependencies between the locks we have, the better and this one seems unnecessary.
> Is it safe to read the `Requests` out of the lock here?
No, but we could store the value of `.empty()` in a local var under the lock. The code looks a bit weirder, but that's a small price to pay for less locks being held at the same time.


================
Comment at: clangd/TUScheduler.h:57
+  enum State {
+    Unknown,
+    Queued,           // The TU is pending in the thread task queue to be built.
----------------
hokein wrote:
> ilya-biryukov wrote:
> > Maybe remove this value and leave out the default constructor too?
> > That would ensure we never fill have `State` field with undefined value without having this enumerator.
> > And FWIW, putting **any** value as the default seems fine even if we want to keep the default ctor, all the users have to set the State anyway. 
> We need the default constructor... When ASTWorker is constructed, we don't know the any state, I'm nervous about putting **any value** as default value (it will hurt the code readability, future readers might be confused -- why the status is `Queued` when constructing).
> 
> Adding an `Unknown` seems ok to me, it is a common practice...
`Idle` would be the valid state for an `ASTWorker` with an empty queue. Why not use it instead?

Can't disagree using `Unknown` is a common practice, but is it a good one? It forces every caller to handle this value, even though it's semantics are always unclear.
Why not use `Optional<EnumWithoutUnkown>` if I really want to indicate the lack of value in those cases? Otherwise every `enum` type implements its own version of `Optional`.


================
Comment at: clangd/TUScheduler.h:60
+    BuildingPreamble, // The preamble of the TU is being built.
+    BuildingFile,     // The TU is being built.
+    Idle, // Indicates the worker thread is idle, and ready to run any upcoming
----------------
hokein wrote:
> ilya-biryukov wrote:
> > hokein wrote:
> > > ilya-biryukov wrote:
> > > > What's the fundamental difference between `BuildingFile` and `RunningAction`?
> > > > We will often rebuild ASTs while running various actions that read the preamble.
> > > > 
> > > > Maybe we could squash those two together? One can view diagnostics as an action on the AST, similar to a direct LSP request like findReferences.
> > > They are two different states, you can think `RunningAction` means the AST worker starts processing a task (for example `Update`, `GoToDefinition`) and `BuildingFile` means building the AST (which is one possible step when processing the task).  
> > > 
> > > In the current implementation, `BuildingPreamble` and `BuildingFile` are only emitted when AST worker processes the `Update` task (as we are mostly interested in TU states of `ASTWorker::update`); for other AST tasks, we just emit `RunningAction` which should be enough.
> > > 
> > > Given the following requests in the worker queue:
> > > `[Update]`
> > > `[GoToDef]`
> > > `[Update]`
> > > `[CodeComplete]`
> > > 
> > > statuses we emitted are
> > > 
> > > `RunningAction(Update) BuildingPreamble BuildingFile`
> > > `RunningAction(GoToDef)`
> > > `RunningAction(Update)  BuildingPreamble BuildingFile`
> > > `RunningAction(GetCurrentPreamble)`
> > > `Idle`
> > > 
> > That's the confusing part: clangd might be building the ASTs inside `RunningAction` too, but we only choose to report `BuildingFile` during updates.
> > I would get rid of `BuildingFile` and change it to `RunningAction(Diagnostics)` instead, it feels like that would allow avoiding some confusion in the future.
> > 
> `BuildingFile` might be more natural (understandable) than `Diagnostics`. Two possible ways here:
> 
> - we can also report `BuildingFile` for other AST tasks, does it reduce the confusing part? 
> - or if you think `Diagnostic` is an important state of `Update`, how about adding a new state `RunningDiagnostic`? `RunningAction` is a higher level information IMO, clangd is running diagnostic inside `RunningAction(Update).`
> 
> WDYT?
If you look at the `TUScheduler` as a black box, diagnostics are not that much different from other actions reading the AST. 
I'd err on the side of not sending updates unless something interesting is actually going on.
What we're currently doing  makes sense (sending a single update per action), I think it doesn't really matter if we call it `BuildingFile` or `RunningAction("Diagnostics")` or `RunningDiagnostics`. Pick the one you like.

Let's put a comment on `BuildingFile` that it's only emitted when building the AST for diagnostics and the read actions emit `RunningAction` instead



Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54796