Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upSupport visualization of scopes, borrows and moves #387
Conversation
Nashenas88
added some commits
Jun 14, 2017
Nashenas88
reviewed
Jun 30, 2017
| rls-data = "0.7" | ||
| rls-span = { version = "0.4" , features = ["serialize-serde"] } | ||
| rls-vfs = { version = "0.4", features = ["racer-impls"] } | ||
| rls-analysis = { path = "../rls-analysis" } |
This comment has been minimized.
This comment has been minimized.
Nashenas88
Jun 30, 2017
Author
Contributor
Pending dependency updates mentioned in top-level comment.
Nashenas88
reviewed
Jun 30, 2017
| @@ -435,6 +435,35 @@ impl ActionHandler { | |||
| } | |||
| } | |||
|
|
|||
| pub fn borrow_info<O: Output>(&self, id: usize, params: TextDocumentPositionParams, out: O) { | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Nashenas88
reviewed
Jun 30, 2017
| out.success(id, ResponseData::BorrowInfo(r)); | ||
| }, | ||
| Ok(Err(e)) => { | ||
| out.failure(id, &format!("BorrowInfo not available for that location: {:?}", e)[..]); |
This comment has been minimized.
This comment has been minimized.
Nashenas88
Jun 30, 2017
Author
Contributor
I'm betting this is not the way to handle a "nothing to see here" type of message. How should I fix this?
This comment has been minimized.
This comment has been minimized.
nrc
Jul 6, 2017
Member
We should probably be able to return something morally equivalent to out.success(id, None) and also use debug! to output a message for devs
Nashenas88
reviewed
Jun 30, 2017
| out.failure(id, &format!("BorrowInfo not available for that location: {:?}", e)[..]); | ||
| } | ||
| Err(_) => { | ||
| out.failure(id, "BorrowInfo failed to complete successfully"); |
This comment has been minimized.
This comment has been minimized.
Nashenas88
Jun 30, 2017
Author
Contributor
Other code wasn't logging errors. Should we start to do that? Is there a file we can write to in case writing to stderr interferes with the IDEs?
This comment has been minimized.
This comment has been minimized.
nrc
Jul 6, 2017
Member
Just using debug! is fine. The client can hide, display, or redirect to a file. We should be logging all errors with debug!.
This comment has been minimized.
This comment has been minimized.
Nashenas88
Jul 7, 2017
•
Author
Contributor
Should I output the object in the Err too? I noticed some other were following this pattern of throwing away the actual error, e.g. hover.
This comment has been minimized.
This comment has been minimized.
nrc
Jul 7, 2017
Member
I've been trying to output the error in the debug! log, but not in the failure message we send back to the LSP
Nashenas88
reviewed
Jun 30, 2017
| @@ -258,4 +277,8 @@ fn help() { | |||
| println!(" hover file_name line_number column_number"); | |||
| println!(" textDocument/hover"); | |||
| println!(" used for 'hover'"); | |||
| println!(""); | |||
| println!(" borrows file_name line_number column_number"); | |||
| println!(" textDocument/borrowInfo"); | |||
This comment has been minimized.
This comment has been minimized.
Nashenas88
reviewed
Jun 30, 2017
| @@ -0,0 +1,77 @@ | |||
| use lsp_data::Range; | |||
This comment has been minimized.
This comment has been minimized.
Nashenas88
Jun 30, 2017
Author
Contributor
Since the language server types crate seemed to only be for types that are part of the spec, this seemed to be the best place for these structs. Are there any concerns with this setup?
This comment has been minimized.
This comment has been minimized.
nrc
Jul 6, 2017
Member
I'm not clear what the difference between these types and the rls-data types is? Ideally we'd just use one set of types. If that is not possible, then this setup seems fine.
This comment has been minimized.
This comment has been minimized.
Nashenas88
Jul 7, 2017
Author
Contributor
If I remember correctly it's the serialization derives. One is RustcSerialize and the other is Serializable. I'll double check after work.
This comment has been minimized.
This comment has been minimized.
nrc
Jul 7, 2017
Member
We ought to be able to derive both if we need to (I think we have feature gates for that, or maybe that is span and we could add them for data)
This comment has been minimized.
This comment has been minimized.
|
@nrc should we close this one too until the issues in the other crates are resolved? If we do close it, could I get responses to some of the questions above so I can get them fixed before the next PR? |
This comment has been minimized.
This comment has been minimized.
|
Answers inline. I don't mind leaving open or closing, whichever is easier for you. |
This comment has been minimized.
This comment has been minimized.
|
So given where some of the conversations have gone in rust#42733 I think we should close this. I took a look at the non-lexical lifetimes (NLL) RFC, and the way it manages lifetimes is very different. The front end code I have shouldn't change much since I'm already accounting for all sorts of edge cases there, but I think the compiler code and rls code will require changes to most of the design. Given that, it doesn't make sense to try to push this through, especially since it might complicate the implementation of NLL. The good news is @nikomatsakis asked to mentor me in the implementation of NLL! I'll do my best to take the visualizer into consideration when working on that project. |
Nashenas88 commentedJun 30, 2017
This depends on rls-data#6, rust#42733, and rls-analysis#71.