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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG if we want to do this
(please getFile -> file though!)



================
Comment at: clangd/Protocol.h:52
 
 struct URIForFile {
+  URIForFile() = default;
----------------
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
...



================
Comment at: clangd/Protocol.h:28
 #include "JSONExpr.h"
+#include "Path.h"
 #include "URI.h"
----------------
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43246