Format ranges by kogai · Pull Request #346 · rust-lang/rls

kogai · 2017-06-13T12:56:26Z

Implements #333

This PR request Rustfmt to format a range of text.

However, with the return value from Rustfmt as a factor, in response to LanguageClient, the text of the whole file is sent.

nrc · 2017-06-13T23:08:47Z

src/actions/mod.rs

+        };
        let input = match self.vfs.load_file(path) {
-            Ok(FileContents::Text(s)) => FmtInput::Text(s),
+            Ok(FileContents::Text(_)) => FmtInput::File(path.clone()),


This is an incorrect change - it will mean any changes in memory are ignored in favour of the old version on disk

nrc · 2017-06-13T23:10:09Z

src/actions/mod.rs

+            "file": "{}",
+            "range": [{}, {}]
+        }}]"#, path.to_str().unwrap(), range.start.line + 1, range.end.line + 1);
+        config.set().file_lines(file_lines.parse().unwrap());


I guess we need a new API for rustfmt to take an optional range over a text input, rather than using this file/range approach

nrc · 2017-06-13T23:11:29Z

src/actions/mod.rs

+        let file_lines = format!(r#"[{{
+            "file": "{}",
+            "range": [{}, {}]
+        }}]"#, path.to_str().unwrap(), range.start.line + 1, range.end.line + 1);


If this range is from rls-span, then you should use the to_one_indexed/to_zero_indexed API rather than manually adding 1. If it is not, then you should convert the range to an rls-span range in server.rs

nrc · 2017-06-13T23:12:39Z

src/actions/mod.rs

+
+                    // If Rustfmt returns range of text that changed,
+                    // we will be able to pass only range of changed text to the client.
+                    let range = ls_util::range_from_vfs_file(&self.vfs, path);


Could move and rename this variable and only make one call to range_from_vfs_file

kogai · 2017-06-14T13:46:39Z

Thank you for reviewing! and I fixed up some codes.

nrc

Thanks for the changes, I have a few more comments from re-review.

nrc · 2017-06-15T00:04:15Z

src/actions/mod.rs

+            "file": "stdin",
+            "range": [{}, {}]
+        }}]"#, range_rls.row_start.0, range_rls.row_end.0);
+        config.set().file_lines(file_lines.parse().unwrap());


COuld we move all of this stuff inside the match/Some branch (and the decl of config before the match)? I'd like to avoid using the file_lines change if no range was given.

nrc · 2017-06-15T00:05:34Z

src/test/mod.rs

 }

+#[test]
+fn test_reformat_with_range() {


You'll need to duplicate the test_data directory so each test is pointing at its own directory (since tests run concurrently, they'll otherwise race)

nrc · 2017-06-15T00:06:40Z

src/test/mod.rs

+
+    assert_eq!(ls_server::LsService::handle_message(server.clone()),
+               ls_server::ServerStateChange::Continue);
+    expect_messages(results.clone(), &[ExpectedMessage::new(Some(42)).expect_contains(r#"{"start":{"line":0,"character":0},"end":{"line":11,"character":69}}"#)


Does the test ensure that code outside of the range (or at least the AST node that encloses the range) is not reformatted?

kogai · 2017-06-15T12:10:03Z

src/test/mod.rs

+    assert_eq!(ls_server::LsService::handle_message(server.clone()),
+               ls_server::ServerStateChange::Continue);
+    expect_messages(results.clone(), &[ExpectedMessage::new(Some(42)).expect_contains(r#"{"start":{"line":0,"character":0},"end":{"line":14,"character":5}}"#)
+                                            .expect_contains(r#"newText":"// Copyright 2017 The Rust Project Developers. See the COPYRIGHT\n// file at the top-level directory of this distribution and at\n// http://rust-lang.org/COPYRIGHT.\n//\n// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or\n// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license\n// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your\n// option. This file may not be copied, modified, or distributed\n// except according to those terms.\n\npub fn main() {\n    let world1 = \"world\";\n    println!(\"Hello, {}!\", world1);\nlet world2 = \"world\"; println!(\"Hello, {}!\", world2);\n}"#)]);


Passing the range to rustfmt, it format the line feed correctly only within the range,
but the indent seems to be modified across the entire text.
Is it intended behavior of rustfmt (or, am I doing something wrong)❓

It is not intended. However... what Rustfmt does with the range is that it rounds it up to the nearest Item ast node (a function or module, etc) and reformats the entire item. I'm not sure if that is happening here, but it might explain why stuff outside the range is formatted.

Could you copy the before and after for what Rustfmt is doing please?

Yes, here is copy of what Rustfmt do.

For simplicity, i execute rustfmt command like follows, instead of call from rls.
cargo fmt -- --file-lines '[{"file": "path/to/file", "range": [2,3]}]'

before

pub fn main() { let world1 = "world"; println!("Hello, {}!", world1); let world2 = "world"; println!("Hello, {}!", world2); }

after

pub fn main() { let world1 = "world"; println!("Hello, {}!", world1); let world2 = "world"; println!("Hello, {}!", world2); }

This looks like the first 3 lines have been formatted and so has the last line, but the forth line has not. This seems like rustfmt is being a bit buggy, but that your stuff (the RLS end) is working fine. I can't actually explain why the forth line has been skipped.

I understood. And then, I modified a test to avoid that part. 965cd33

kogai · 2017-06-15T12:12:15Z

Thank you a lot for reviewing ❗️
I fixed up some codes and could you please review again 🙏

kogai · 2017-06-18T01:18:36Z

src/test/mod.rs

    assert_eq!(ls_server::LsService::handle_message(server.clone()),
               ls_server::ServerStateChange::Continue);
-    expect_messages(results.clone(), &[ExpectedMessage::new(Some(42)).expect_contains(r#"{"start":{"line":0,"character":0},"end":{"line":11,"character":69}}"#)
+    expect_messages(results.clone(), &[ExpectedMessage::new(Some(42)).expect_contains(r#"{"start":{"line":0,"character":0},"end":{"line":12,"character":0}}"#)


Fixed that test failed when rebasing current master.
It seems to be able to fix #350.

nrc

Thanks for the changes, and sorry to make you rebase. I just had two minor things left to address.

nrc · 2017-06-18T03:41:12Z

src/actions/mod.rs

-    pub fn reformat(&self, id: usize, doc: TextDocumentIdentifier, out: &Output, opts: &FormattingOptions) {
-        trace!("Reformat: {} {:?} {} {}", id, doc, opts.tab_size, opts.insert_spaces);
+    pub fn reformat(&self, id: usize, doc: TextDocumentIdentifier, selection: Option<Range>, out: &Output, opts: &FormattingOptions) {
+        trace!("Reformat: {} {:?}", id, doc);


Please could you keep the options in this trace!

Oh..., I apologize for failing to rebase 🙇

nrc · 2017-06-18T03:42:57Z

src/actions/mod.rs

+            let file_lines = format!(r#"[{{
+                "file": "stdin",
+                "range": [{}, {}]
+            }}]"#, range.row_start.0, range.row_end.0);


Could you use a struct and serialise it, rather than using format! here please? (Ideally, you could use a struct from Rustfmt, but it's ok to create your own here if that is not possible).

I create Adopter of Rustfmt::FileLines and avoid to serialize FileLines in reformat method directory, daaac8e but if it does not reflect what you pointed out, please tell me 🙏

nrc · 2017-06-18T22:22:27Z

I create Adopter of Rustfmt::FileLines and avoid to serialize FileLines in reformat method directory, daaac8e but if it does not reflect what you pointed out, please tell me

I think that to create the FileLines, rather than use the parse method, you can create the object directly

kogai · 2017-06-19T00:31:00Z

I am sorry if I misunderstood something, but it doesn't seem to be able to create FileLines directly because Rustfmt doesn't export it's own Range struct as a public one that depended by FileLines struct 🤔

nrc · 2017-06-19T00:59:42Z

Ah, you are right, I fixed it - rust-lang/rustfmt@34fa428 I'll make a release of Rustfmt with that change later today.

kogai · 2017-06-19T12:20:44Z

Thank you so much for responding.
But for this purpose, it seems necessary to expose several constructors like this.
May I send this patch to Rustfmt?

nrc · 2017-06-20T00:10:49Z

May I send this patch to Rustfmt?

Yes please!

kogai · 2017-06-20T00:48:46Z

I have created a PR.
Could you review it, please?
rust-lang/rustfmt#1735

kogai · 2017-06-20T04:43:58Z

src/actions/mod.rs

+            ranges.insert("stdin".to_owned(), vec![range]);
+            let file_lines = FileLines::from_ranges(ranges);
+            config.set().file_lines(file_lines);
+        };


Fix up to create FileLines directly instead of using format!.
At present, it destroys the build, until this patch is released 🙏

Released 0.1.5 with that patch

I think you'll need to cargo update and commit the Cargo.lock to get the latest rustfmt version

Updated Cargo.lock, and pushed it ❗️

nrc · 2017-06-20T07:16:15Z

Thank you!

kogai force-pushed the format-ranges branch from 5795d10 to f8762e5 Compare June 13, 2017 12:59

kogai changed the title ~~WIP: Format ranges~~ Format ranges Jun 13, 2017

nrc reviewed Jun 13, 2017

View reviewed changes

kogai force-pushed the format-ranges branch from b0ed800 to e89f9f6 Compare June 14, 2017 13:44

nrc reviewed Jun 15, 2017

View reviewed changes

kogai commented Jun 15, 2017

View reviewed changes

kogai force-pushed the format-ranges branch 2 times, most recently from aec86f8 to 965cd33 Compare June 18, 2017 01:06

kogai commented Jun 18, 2017

View reviewed changes

nrc reviewed Jun 18, 2017

View reviewed changes

kogai force-pushed the format-ranges branch from bf783f8 to daaac8e Compare June 18, 2017 05:49

kogai mentioned this pull request Jun 20, 2017

Public file-lines constructor rust-lang/rustfmt#1735

Merged

kogai added 6 commits June 20, 2017 13:24

Add spec for reformatting with range

418c148

Support format with range

b175891

Fix: incorrect parameter for format_input

9435ba0

Fix: mutate config only if it necessary

e4dbd88

Fix: avoid checking inside of closure

b08ed7e

Fix unintended rebase result

b8438fb

nrc mentioned this pull request Jun 20, 2017

Fix test_reformat expected range #358

Closed

Create FileLines directly instead of using format! macro

068f0e1

kogai force-pushed the format-ranges branch from daaac8e to 068f0e1 Compare June 20, 2017 04:39

kogai commented Jun 20, 2017

View reviewed changes

Update depenencies

d327cd3

nrc merged commit 4e359ad into rust-lang:master Jun 20, 2017

Avi-D-coder mentioned this pull request Jul 16, 2017

Using rustfmt should be more embraced intellij-rust/intellij-rust#1087

Closed

Conversation

kogai commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kogai commented Jun 14, 2017

Uh oh!

nrc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kogai Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

before

after

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kogai Jun 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kogai commented Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrc commented Jun 18, 2017

Uh oh!

kogai commented Jun 19, 2017

Uh oh!

nrc commented Jun 19, 2017

Uh oh!

kogai commented Jun 19, 2017

Uh oh!

nrc commented Jun 20, 2017

Uh oh!

kogai commented Jun 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kogai Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrc commented Jun 20, 2017

Uh oh!

Reviewers

kogai commented Jun 13, 2017 •

edited

Loading

kogai Jun 15, 2017 •

edited

Loading

kogai Jun 18, 2017 •

edited

Loading

kogai commented Jun 15, 2017 •

edited

Loading

kogai Jun 20, 2017 •

edited

Loading