[PATCH] D103179: [clangd] Handle queries without an originating file in ProjectAwareIndex

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Sorry for lots of comments on a fairly small patch.

---

I want to mention one alternative we haven't considered. Ultimately the problem here is that we have actions that depend on config but we don't know what file to pull the config from.
This corresponds to passing Path="" to TUScheduler::run (3 calls) or runQuick (0 such calls).
Our decision is to effectively use the config from the last accessed file.
Why don't we put this policy in TUScheduler, rather than inferring it at the edges?
Because TUScheduler isn't threadsafe, I think this is as simple as adding a `std::string LastFile` member, and reading/writing to it in all the run* methods.

---

> But I'd rather not only store Path in the Config since we might end up needing other environment variables in future.

Right. However this patch goes further, and says we must expose *all* other parameters, in the same way and with the same semantics (since we only get to document them once).
I don't find the argument totally convincing, because currently we have 2 params, and:

- fresh time shouldn't be exposed at all, unless i'm missing something
- the path is exposed for fairly subtle reasons, and we want to use it sparingly and so probably give it quite weak semantics (e.g. wouldn't be appropriate to grab "the current filename" for logging purposes, I think)



================
Comment at: clang-tools-extra/clangd/Config.h:60
+    /// slashes. Empty if not configuring a particular file.
+    llvm::StringRef Path;
+    /// Hint that stale data is OK to improve performance (e.g. avoid IO).
----------------
who owns the underlying string, with what lifetime?

The "root" that creates the config doesn't currently have any obligation to keep the params around for as long as the config might be active (which can be async via context). I think in config this should be a std::string


================
Comment at: clang-tools-extra/clangd/Config.h:60
+    /// slashes. Empty if not configuring a particular file.
+    llvm::StringRef Path;
+    /// Hint that stale data is OK to improve performance (e.g. avoid IO).
----------------
sammccall wrote:
> who owns the underlying string, with what lifetime?
> 
> The "root" that creates the config doesn't currently have any obligation to keep the params around for as long as the config might be active (which can be async via context). I think in config this should be a std::string
How do we expect this to be used?
If it's for determining the active project, then I'd expect us to give some weaker semantic like "representative file from the active project" so we can fill in e.g. the main file associated with a TU.

But actually we only use the boolean "is this set". Maybe we should just provide that?
"bool WasConfiguredForPath" or something? Generalizing past that might be premature.


================
Comment at: clang-tools-extra/clangd/Config.h:69
+  /// Information about the environment used when generating the config.
+  Params Parm;
+
----------------
try to avoid using multiple abbreviations of the same word :-)


================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:152
+  // files open from projects with and without an external index.
+  if (C.Index.External || !C.Parm.Path.empty())
+    LastIndex = GetIndexFromSpec(C.Index.External);
----------------
This logic feels unnecessarily tricky: you check whether the spec is set both here and above.
Consider something longer but more direct

```
SymbolIndex *Specified = C.Index.External ? GetIndexFromSpec(*C.Index.External) : nullptr;
if (!Specified && C.Parm.Path.empty()) {
  // There was no index specified, but we're also not sure which path we're using.
  // Use the index from the last operation that had a valid path.
  return LastIndex.
}
// Cache the last-used index, even if it's null ("no external index" is a valid config!)
LastIndex = Specified;
return Specified;
```


================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:154
+    LastIndex = GetIndexFromSpec(C.Index.External);
+  return LastIndex;
 }
----------------
this is racy: if there *is* a well-defined external index for the current config, then we should return it even if another thread is able to concurrently write to LastIndex.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103179