[PATCH] D43246: [clangd] Assert path is absolute when assigning to URIForFile.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D43246#1006525, @ioeric wrote:

> I think another option to prevent the bug in r325029 from happening would be providing a canonical approach for retrieving (absolute) paths from `FileManager`. We already have code in symbol collector that does this, and we have similar code in XRefs.cpp now. We should probably move them to `clangd/Path.h` and share the code?


I agree with you, we should do this.
But this change is not a mitigation for r325029 specifically. URIForFile would still be tremendously easy to misuse in other cases as well.



================
Comment at: clangd/Protocol.h:52
 
 struct URIForFile {
+  URIForFile() = default;
----------------
sammccall wrote:
> I don't like how the API changes here take us further away from the other structs in this file.
> Why does this one enforce invariants, when the others don't?
> 
> If we do want to make this more "safe", I'd suggest making it look more like a URI string (since that's what its protocol role is), like:
> 
> ```
> class LSPURI {
>   LSPURI(StringRef URI) { assert if bad }
>   static LSPURI ForFile(StringRef AbsPath) { assert if bad }
>   operator string() { return URI::createFile(File).toString(); }
>   StringRef file() { return AbsFile; }
>   operator bool() { return !File.empty(); } 
>   // move/copy constructor/assignment defaulted
> 
>  private:
>   string AbsFile;
> }
> ```
> 
> But the approach Eric suggested does seem promising: more consistent with how we handle invariants in protocol structs.
I've updated `URIForFile` to follow the interface you proposed. (Instead of `operator string()` we have `uri()`, but everything else is there). Haven't changed the name yet, happy to do it to.

> Why does this one enforce invariants, when the others don't?
It seems useful to catch misbehaving code as early as possible. What are the other invariants that could be useful?

> But the approach Eric suggested does seem promising: more consistent with how we handle invariants in protocol structs.
I agree with Eric's approach, I wasn't trying to fix that specific bug with this commit.

I'm trying to argue that we should check for non-absolute paths as early as possible and fail with assertions to make writing code easier in the future. Do you agree that would be useful or make we shouldn't do it?




Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43246