Use languageserver-types crate #79

bruno-medeiros · 2016-10-26T19:33:49Z

Of note, need to review lsp_data::to_usize and ActionHandler::on_change when range is None

Note that a case of DidChangeTextDocumentParams need handling. Closes rust-lang#52

bruno-medeiros · 2016-10-26T19:34:28Z

I ran the tests locally and some fail, but I think it's because it's a Windows machine...

nrc · 2016-10-26T20:17:53Z

src/actions_ls.rs

@@ -212,7 +228,7 @@ impl ActionHandler {
        let vfs: &Vfs = &self.vfs;
        let result: Vec<CompletionItem> = panic::catch_unwind(move || {
            let pos = adjust_vscode_pos_for_racer(params.position);
-            let file_path = &params.textDocument.file_name();
+            let file_path = &uri_string_to_file_name(&params.text_document.uri);


Could we reduce the code dup here?

What code dup? You mean accessing .uri ? As in, do you want a function that only takes &params.text_document as an argument?

Perhaps a macro that works on anything with a uri field (or even a textDocument field, depending on what the other uses are like)? Not the end of the world if not. I guess it is not much code dup, it just looks like a bit more complicated than the old version.

Alternatively have a FileUri newtype that impls Deserialize to convert a string to a PathBuf via Url::to_file_path ?

Hum, I think the code dup here is minimal, insignificant almost. If anything, what was nicer about the original version was that it was an method of the LSP type, instead of a free function.

I think maybe we could move it back to a method if we introduce a dependency on the URL crate in languageserver-types, @Marwes what do you think of that? I think any LS would end up needing URL/URL parsing anyways, so it seems ok to me.

The rustls code would then be like this:

let file_path = &params.text_document.file_name().unwrap();

uri_string_to_file_name already assumes it's going to be given a file:// URI that it can convert to a PathBuf. What else could that field contain and how would rls be expected to handle it?

I just meant that the uri field should not be restricted to file://, it is perfectly fine for rls to just handle file:// but in general it could be an identifier for something else.

@Arnavion remember that languageserver-types (and RustLSP too BTW) is meant to be usable by servers other than just rustls (indeed there is https://github.com/gluon-lang/gluon_language-server , and I also hope to use languageserver-types in another project) . So even if rustls only support file:// resource, languageserver-types should support any valid LSP usage.

So, I'm happy to keep this as is, since it seems there is no universally better solution. Do we want to change these fields to Uri upstream? Before landing this?

@nrc Yeah I'm up for adding that, although ATM I'm getting bogged down in the confusion of URLs vs. URIs 😖

nrc · 2016-10-26T20:18:27Z

src/actions_ls.rs

+    pub fn on_change(&self, change: DidChangeTextDocumentParams, out: &Output) {
+        let fname: PathBuf = Url::parse(&change.text_document.uri).unwrap().to_file_path().unwrap();
+        let changes: Vec<Change> = change.content_changes.iter().map(move |i| {
+        	let range = match i.range {


Please use spaces, not tabs

nrc · 2016-10-26T20:19:34Z

src/actions_ls.rs

@@ -370,16 +386,17 @@ impl ActionHandler {

            let mut contents = vec![];
            if !docs.is_empty() {
-                contents.push(MarkedString { language: "markdown".into(), value: docs });
+                contents.push(MarkedString::LanguageString { language: "markdown".into(), value: docs });


Could this get a ctor function?

nrc · 2016-10-26T20:21:31Z

src/ls_server.rs

@@ -61,8 +62,8 @@ enum Method {

 #[derive(Debug)]
 enum Notification {
-    CancelRequest(usize),
-    Change(ChangeParams),
+    CancelRequest(NumberOrString),


Is this an id? I think there is another PR that uses Id rather than NumberOrString, it would be good to rebase on top of that

Yeah, it's an id. (Well, one that is not null at least. Tecnically in JSON-RPC null is allowed as an id, but discouraged)
But, if we want to use the same type, the other PR would have to use the languageserver-types type, not the other way around, because that would require adding a dependency on rustls to languageserver-types, which would not be suitable.

Anyways, it's likely that Id will be replaced eventually, if/when a PR comes to use RustLSP or another JSON-RPC library.

ok cc #73 and @craftytrickster

I haven't had time yet to make the test changes you wanted, which is why it has been a bit stagnant. I will close it and just make the PR on top of this crate instead.

I see that bruno already added the deserializaton for NumberOrString, I guess there's no need for me to make any changes.

nrc · 2016-10-26T20:23:23Z

src/ls_server.rs

-                    resolveProvider: true,
-                    triggerCharacters: vec![".".to_string()],
-                },
+                text_document_sync: Some(TextDocumentSyncKind::Incremental),


There seem to be a lot more Options all over the place, that seems unfortunate - why is that?

It's just the way the LSP protocol is modelled. Arguably the protocol itself could be simplified in this regard, but otherwise I think we should model the protocol types precisely (even if it has redundancies). That is not to say we couldn't create a shortcut ctor function, or even an intermediate type (say, to ServerCapabilities), that would not have the Options. I'll give that a try.

Where there is an Option could you add a null case to the enum and ensure it gets serialised as 0 or null or whatever the protocol wants?

@nrc What do you mean, do you want it to serialize to the JSON "text_document_sync":null if text_document_sync field is None? That be achieved if we remove the #[serde(skip_serializing_if="Option::is_none")] attributes.

I've brought up the issue of null vs missing properties (undefined in JSON terms) with LSP guys here microsoft/language-server-protocol#87 and argued that they should be semantically equivalent and the protocol should reflect that.

But serializing to null might be clearer, yeah.

Where there is an Option could you add a null case to the enum and ensure it gets serialised as 0 or null or whatever the protocol wants?

The enum itself is not defined with a "null" case so adding one to the enum would just be more confusing it kind of smells like a null pointer.

I mean instead of having

enum Foo { Bar, Baz, }

and using Option<Foo>, you have:

enum Foo { Bar, Baz, None, // or Null or Missing or whatever }

and use Foo directly.

smells like a null pointer.

There is nothing wrong with null pointers, the problem is when they are implicit and thus un-tested. With this pattern, they are just as explicit and thus safe to use.

the problem is when they are implicit and thus un-tested. With this pattern, they are just as explicit and thus safe to use.

That is part of it (and the most troublesome part), but I'd argue that adding an extra value just because it is possible and simplifies some parts is similar to how null pointer got so prevalent as well.

Ugh, I also am quite averse to that suggestion of adding None/Missing variants. Feels very wrong: it makes it impossible to specify a non-null Foo, which I think is an inferior abstraction. I don't think expressive power should be sacrificed just so that we save some minor boilerplate code when matching against Option<Foo> .

I guess it depends on what you are modelling - if there is a case for a non-null Foo, then I would agree, if the Foo intrinsically has a null case then I think it is better to reflect that in the enum. Does the spec always allow null for these enums? If so I'd argue that using Option rather than extending Foo is unnecessary complexity (YAGNI). If there are places in the spec where you are allowed non-null, then keep them as is.

nrc · 2016-10-26T20:37:16Z

src/lsp_data.rs

+
+pub fn from_usize(pos: usize) -> u64 {
+	TryFrom::try_from(pos).unwrap() // XXX: Should we do error handling or assume it's ok?
+}


SHould just use a cast rather than a function for this.

You sure? It would silently overflow/truncate instead of panicking... of course this would only matter for 128-bit architectures... 😅

nrc · 2016-10-26T20:41:37Z

src/lsp_data.rs

-    pub version: u64,
-    pub uri: String
-}
+pub type HoverParams = TextDocumentPositionParams;


Can probably do without this

nrc · 2016-10-26T20:42:18Z

src/lsp_data.rs

-#[derive(Debug, Deserialize)]
-pub struct TextDocumentIdentifier {
-    pub uri: String
+pub fn new_completion_item(label: String, detail: String) -> CompletionItem {


Could this have a more descriptive name? It is not really a CompletionItem ctor fn

nrc · 2016-10-26T20:42:55Z

src/lsp_data.rs

-    pub kind: u32, // can be made to numeric enum
-    pub location: Location,
-}
+/* ----------------- These are not LSP types: ----------------- */


This comment doesn't seem helpful

The comment is a reflection that those types should probably be moved elsewhere (I guess it's more of a TODO). But I don't know where.

Could you add that to the comment please?

nrc · 2016-10-26T20:44:34Z

src/lsp_data.rs

-#[derive(Debug, Deserialize)]
-pub struct Document {
-    pub uri: String
+pub fn sk_from_def_kind(k: raw::DefKind) -> SymbolKind {


s/sk/symbol_kind

nrc · 2016-10-26T20:45:53Z

src/ide.rs

@@ -74,6 +74,7 @@ pub fn parse_string(input: &[u8]) -> Result<String, serde_json::Error> {
    serde_json::from_str(&s)
 }

+// TODO: deprecate/remove in favor of ls_types::SymbolKind ?


Is it still used anywhere?

Yes, in actions_http::Symbol and further up by the HTTP server. The serge::Serialize implementation is completely different than the LS type BTW, so it would not even be compatible to replace it with the LS type. (unless the HTTP client is changed as well)

OK, could you add to the TODO that it is only used by the http server please? (We plan to remove the http mode soon-ish).

nrc · 2016-10-26T20:47:39Z

Test failures look legit from Travis. Missing capabilities in the init reply?

Arnavion · 2016-10-26T23:47:08Z

I ran the tests locally and some fail, but I think it's because it's a Windows machine...

Tests should pass on Windows as well. Make sure you set RUST_TEST_THREADS=1 and SYS_ROOT=$(rustc --print sysroot) env vars.

Edit: ... and, for the moment, that the project you test this on is in a path without spaces.

Arnavion · 2016-10-26T23:53:54Z

src/lsp_data.rs

-pub struct Position {
-    pub line: usize,
-    pub character: usize,
+pub fn uri_string_to_file_name(uri: &str) -> PathBuf {


Conflicts: src/ls_server.rs

bruno-medeiros · 2016-10-27T12:09:54Z

Some thoughts:

For the new util module, is there a way to not have to duplicate the use statements from parent module?
I've changed rustls to use Position ctor, should I also use the newly added ctors for other basic LS types like Range, Diagnostic, etc. ?
Is there a plan to have URI parsing into the standard lib? Seems like this would be useful for Rust in general, and for here specifically, it would allow to move some URI relate ctors into languageserver-types crate itself.

nrc · 2016-10-27T20:17:16Z

For the new util module, is there a way to not have to duplicate the use statements from parent module?

No

I've changed rustls to use Position ctor, should I also use the newly added ctors for other basic LS types like Range, Diagnostic, etc. ?

Yeah, I would use those ctors where it makes sense (i.e., where it makes the code nicer). I wouldn't say to do it all the time, necessarily.

Is there a plan to have URI parsing into the standard lib? Seems like this would be useful for Rust in general, and for here specifically, it would allow to move some URI relate ctors into languageserver-types crate itself.

Not that I know of.

Marwes · 2016-10-28T11:32:48Z

src/actions_ls.rs

+            results.map(|comp| new_completion_item(
+                comp.matchstr.clone(),
+                comp.contextstr.clone(),
+            )).collect()


Alternatively

CompletionItem { label: comp.matchstr.clone(), detail: comp.contextstr.clone(), .. CompletionItem::default() }

bruno-medeiros · 2016-11-01T18:57:07Z

In my opinion, since there will always be differing opinions on which methods to add to the types I'd prefer to have no methods or just the bare minimum number of methods lest the API is likely going to end up bloated by "wouldn't it be nice if..." methods.

Yes, there will be different opinions on which utility methods to add to types, but I don't think that is a justification for doing just the bare minimum. Yes, the bare minimum is a much more objective metric for measuring inclusion rather than how "useful" or "common" a utility method is in used in 3rd party code, but again I don't think that justifies doing the bare minimum.

If a method might be "useful" for 3rd party developers (and is general enough, and has minimal maintenance overheard), I think it's worth including. Yes, with this approach you have the problem of deciding if an utility method is "useful" enough, which is a subjective and imprecise judgement. You can have some guidelines on that, but ultimately this is one of those things that is left to the judgement of a software craftsman. (the "art" in software engineering?)

It's a bit like testing: Testing does not guarantee absence of bugs, so how do decide how much tests are enough? You can try to go for a certain coverage percentage for you component under test, but coverage % is not a precise measure of test quality, far from it. Ultimately you have to rely on your own judgement, there is no other way...

Anyway, I don't want to get in a big philosophical debate here, suffice to say I prefer to err on the "bloated" side of things. We have namespaces, so what harm is there in adding utility methods to a type if someone is willing to write them? Again, assuming those methods have low maintenance overhead and are general enough - these are the only concerns I would have.

(I am a bit skeptic towards adding the constructor methods actually as they will basically be redundant once rust-lang/rust#37340 is implemented and gets to stable).

Can always remove them once that feature is in.

For the record I should probably mention I don't usually follow Rust RFCs - I only learn about new languages features once they are released in Rust stable. I'm not that up to date... 😅

bruno-medeiros · 2016-11-08T16:58:54Z

URI stuff up: gluon-lang/lsp-types#3
@nrc @Marwes feedback welcome.

Also note my conclusions on the URL/URI ambiguity:

"
This library uses the URL crate for parsing URIs.
Note that there is some confusion on the meaning of URLs vs URIs:
http://stackoverflow.com/a/28865728/393898 .
According to that information, on the classical sense of "URLs", "URLs" are a subset of URIs,
But on the modern/new meaning of URLs, they are the same as URIs.
The important take-away aspect is that the URL crate should be able to parse any URI,
such as urn:isbn:0451450523
"

Conflicts: Cargo.lock

Conflicts: src/test/mod.rs

Conflicts: src/ls_server.rs src/lsp_data.rs

Conflicts: src/actions_ls.rs src/ls_server.rs

Conflicts: Cargo.toml src/actions.rs src/server.rs

bruno-medeiros · 2016-11-11T17:31:16Z

Any outstanding requests/comments for this PR? It gets a bit hard to track in Github after many changes are added.

nrc · 2016-11-15T19:03:30Z

src/actions.rs

+        let root_path = match root_path {
+            Some(some) => some, 
+            None => return
+        };


This should be done by the caller, not here.

nrc · 2016-11-15T19:07:46Z

src/actions.rs

+                    let end_pos = Position::new(0, 0);
+                    Range{ start : Position::new(0, 0), end : end_pos }
+                }
+            };


Seems to like making a range for a whole file should be a util function

nrc · 2016-11-15T19:08:41Z

src/actions.rs

-                    location: Location::from_span(&s.span),
+                    kind: source_kind_from_def_kind(s.kind),
+                    location: ls_util::location_from_span(&s.span),
+                    container_name: None // TODO: more info could be added here


nit: prefer FIXME over TODO for things being left in the code (i.e., no TODOs should get committed)

nrc · 2016-11-15T19:09:32Z

src/actions.rs

                            &session)
                .and_then(|mtch| {
                    let source_path = &mtch.filepath;
                    if mtch.point != 0 {
                        let (line, col) = session.load_file(source_path)
                                                 .point_to_coords(mtch.point)
                                                 .unwrap();
-                        Some(Location::from_position(source_path,
+                        Some(ls_util::location_from_position(source_path,
                                                     adjust_racer_line_for_vscode(line),


nit: indents need adjusting

nrc · 2016-11-15T19:11:04Z

src/lsp_data.rs

-    pub line: usize,
-    pub character: usize,
+pub fn parse_file_path(uri: &Url) -> Result<PathBuf, Box<Error>> {
+


nit: blank line

nrc · 2016-11-15T19:11:53Z

src/lsp_data.rs

+
+    if uri.scheme() != "file" {
+        return Err("URI scheme is not `file`".into());
+    }


nit: prefer if ... == ... { ... } else { ... } (no return)

nrc · 2016-11-15T19:12:10Z

src/lsp_data.rs

-    /// End position is exclusive
-    pub end: Position,
+pub fn from_usize(pos: usize) -> u64 {
+    TryFrom::try_from(pos).unwrap() // XXX: Should we do error handling or assume it's ok?


prefer FIXME to XXX

also, no, this will always succeed. And you can just use a cast here.

nrc · 2016-11-15T19:13:53Z

src/lsp_data.rs

-impl Range {
-    pub fn from_span(span: &Span) -> Range {
+pub fn to_usize(pos: u64) -> usize {
+    TryFrom::try_from(pos).unwrap() // FIXME: for this one we definitely need to add error checking


Just use a cast, not TryFrom

nrc · 2016-11-15T19:14:45Z

src/lsp_data.rs

-    pub kind: u32, // can be made to numeric enum
-    pub location: Location,
-}
+/* ----------------- These are not LSP types: ----------------- */


Could you add that to the comment please?

bruno-medeiros · 2016-11-16T14:09:41Z

Could you add that to the comment please?

What do you mean? Add "These are not LSP types" message to the doc of CompilerMessageCode and CompilerMessage?

bruno-medeiros · 2016-11-16T14:14:13Z

What do you mean? Add "These are not LSP types" message to the doc of CompilerMessageCode and CompilerMessage?

It might be preferable to just move those types elsewhere (maybe action.rs?), and keep lsp_data.rs with just LSP related stuff.

Conflicts: Cargo.lock

nrc · 2016-11-16T23:34:41Z

What do you mean?

I meant could you change the comment in the code from "these types are no LSP types" or whatever to something like "FIXME: these types are not LSP types and don't really belong in this module"

nrc · 2016-11-23T19:31:34Z

Thanks for all the changes! I think this is ready to merge now. I believe the test failures are just due to compiler/Serde mismatches, so I'll merge. Praying it doesn't explode.

matklad · 2016-11-24T09:15:45Z

Looks like it did 💥 #108 :)

FYI, maintainers can restart Travis builds for PRs from the Travis UI, contributors can restart the build by just closing and reopening a PR (to clarify, you don't need to open a new pull request, you need just to click to buttons on the PR page).

bruno-medeiros · 2016-11-24T10:11:03Z

Added #110 to fix the Cargo.lock and tests

matklad · 2016-11-24T10:13:59Z

Thanks!

bruno-medeiros added 2 commits October 26, 2016 20:22

Changes to use languageserver-types crate

ae7e850

Complete usage of languageserver-types.

d87d7db

Note that a case of DidChangeTextDocumentParams need handling. Closes rust-lang#52

nrc reviewed Oct 26, 2016

View reviewed changes

nrc suggested changes Oct 26, 2016

View reviewed changes

Arnavion reviewed Oct 26, 2016

View reviewed changes

bruno-medeiros added 8 commits October 27, 2016 09:47

Replaced tabs

ea098da

Use MarkedString constructor

d6af2ed

Use Position::new

abee269

symbol_kind_from_def_kind and ls_util module

59044aa

CompletionItem::new_simple, removed HoverParams alias

627bb03

fix tests

6eb2ce4

Cargo.lock

038087e

Merge remote-tracking branch 'origin/master'

d1dcacf

Conflicts: src/ls_server.rs

languageserver-types 0.4.0, which has some fixes.

75fa8e5

comment about VscodeKind deprecation

0440f36

Marwes reviewed Oct 28, 2016

View reviewed changes

Merge remote-tracking branch 'origin/master'

e9e73e5

bruno-medeiros added 6 commits November 8, 2016 19:45

ls-types 0.5.0 (URI fields)

a08f071

Merge commit 'cad93c0acac4348f18a538357d2cc7d6a9f487ce'

4d5f95f

Conflicts: Cargo.lock

Merge commit '67e165876eb9b648385bffb8d91c246b9d336234'

2d58e16

Conflicts: src/test/mod.rs

Merge remote-tracking branch 'origin/master'

1cdfe92

Conflicts: src/ls_server.rs src/lsp_data.rs

Merge remote-tracking branch 'origin/master'

36d1e81

Conflicts: src/actions_ls.rs src/ls_server.rs

Removed VscodeKind since it was only used by HTTP server

a6487c8

bruno-medeiros added 4 commits November 9, 2016 13:06

fix test failures?

c3f6670

Merge commit '0b613439ef2b78feefeb56a5fbc228868aae4b4d'

1cd10a8

Merge remote-tracking branch 'origin/master'

260b536

Conflicts: Cargo.toml src/actions.rs src/server.rs

fix tests for logging crate

0916577

nrc mentioned this pull request Nov 11, 2016

Use RustLSP library. #96

Closed

nrc suggested changes Nov 15, 2016

View reviewed changes

Merge remote-tracking branch 'origin/master'

f9c3a91

Conflicts: Cargo.lock

Second round of review changes.

948eb0a

nrc merged commit d134eee into rust-lang:master Nov 23, 2016

liamdawson mentioned this pull request Oct 7, 2017

jsonrpc conformance #62

Closed

Use languageserver-types crate #79

Use languageserver-types crate #79

Conversation

bruno-medeiros commented Oct 26, 2016

bruno-medeiros commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marwes Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc commented Oct 26, 2016

Arnavion commented Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

bruno-medeiros commented Oct 27, 2016

nrc commented Oct 27, 2016

Marwes Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

bruno-medeiros commented Nov 1, 2016

bruno-medeiros commented Nov 8, 2016

bruno-medeiros commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-medeiros commented Nov 16, 2016

bruno-medeiros commented Nov 16, 2016

nrc commented Nov 16, 2016

nrc commented Nov 23, 2016

matklad commented Nov 24, 2016 • edited Loading

bruno-medeiros commented Nov 24, 2016

matklad commented Nov 24, 2016

Marwes Oct 28, 2016 •

edited

Loading

Arnavion commented Oct 26, 2016 •

edited

Loading

Marwes Oct 28, 2016 •

edited

Loading

matklad commented Nov 24, 2016 •

edited

Loading