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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.


================
Comment at: clangd/Protocol.h:52
 
 struct URIForFile {
+  URIForFile() = default;
----------------
sammccall wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > 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?
> > > 
> > > 
> > I agree that this change would help debugging, so I don't really have objection. I'll leave approval to Sam though, as he had some suggestions.
> > 
> > I wonder if we want to make the assertion more aggressive. Currently, this only helps with assertion on. Would it make sense to also check this in production code and provide useful error message so that it would be easier for us to spot the problem when users report a bug? 
> I'm not going to block this change, but I somewhat prefer the current code without it (and assertions in the places that populate URIForFile instead).
> 
> > It seems useful to catch misbehaving code as early as possible. What are the other invariants that could be useful?
> 
> At a quick scan of `protocol.h` from the top:
> 
> Position.line/character >= 0
> Range.start <= Range.end
> Location.uri non-empty
> TextDocumentItem.uri non empty (and lots of other uris)
> FormattingOptions.tabSize >= 0
> Diagnostic.message non-empty
> ExecuteCommandParams.command has a valid value
> ...
> 
After looking at the code, all instances of `URIForFile` are created with the assertion enforced, other structs you mention can be created in arbitrary state by `fromJSON` functions.
I'm not sure if making `fromJSON` functions more complicated are a good trade-off for enforcing those assertions.

`URIForFile` is different from other structs, it already has logic to convert file paths to uri (and back indirectly, via `fromJSON` function). Others merely directly capture the information from the LSP spec.


================
Comment at: clangd/Protocol.h:28
 #include "JSONExpr.h"
+#include "Path.h"
 #include "URI.h"
----------------
sammccall wrote:
> can we hold off on adding `Path` to more places in this patch?
> I'd like to see if others find it useful - it mostly seems obscure to me.
Done.

I was the one who introduced them and I (obviously) like them. I find the code using them a bit easier to read, they don't add any type-safety, though.
Let's discuss whether others find them useful. If I'm the only one who feels this way, we can remove them.


================
Comment at: clangd/Protocol.h:57
+  /// Retrieves absolute path to the file.
+  PathRef getFile() const { return File; }
+
----------------
ioeric wrote:
> nit: I like `file()` that Sam suggested a bit more. 
Done.

Note that this technically breaks  [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | LLVM Style Guide ]]: "Function names should be verb phrases".
But I think we're breaking it in multiple places already (including here, e.g. `uri()`).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43246