Support using external Rustfmt #990

Xanewok · 2018-08-14T15:22:57Z

Fixes #812. (Uses https://github.com/Xanewok/rustfmt/tree/file-lines-ser-json)

High-level overview:

Introduces trait Formatter { fn format(&self, String, rustfmt::Config) -> Result<String, String>; }
Introduces a Rustfmt enum as a convenient interface which implements the trait, accessible via ctx.formatter()
Rustfmt::Internal variant implementation is what was currently used, running included Rustfmt in-process
Rustfmt::External spawns the provided command, using the Rustfmt stdin/stdout mode.

Assuming that Rust input should be a valid UTF-8 and so Rustfmt should also reformat it to a valid UTF-8, returning Err if that's not the case.

This is not entirely correct yet, since we should pass full Config in a file to executed Rustfmt, since LSP may request additional formatting options and this currently just uses default Rustfmt configuration either via rustfmt.toml or by using default, but with some RLS-specific options.

Have to finish this one bit, but in general this should be ready for a review!

Xanewok · 2018-08-14T15:36:10Z

src/actions/format.rs

 fn gen_config_file(config: &Config) -> Result<(File, PathBuf), String> {
    let (mut file, path) = random_file()?;
-    let toml = config.used_options().to_toml()?;
+    let toml = config.all_options().to_toml()?;


Well, that was easier than expected!

Xanewok · 2018-08-14T15:40:26Z

The drawback to using the external Rustfmt is that we have to pass the entire config via file, so here it's done by creating a temporary file.
However, it's worth noting that we also format hover messages with Rustfmt, which means that for every such invocation we shell out to Rustfmt, create the config file, dump all options etc.

It might be a good idea to just use the statically linked Rustfmt for the tooltips, since most of the time there's almost null work to be done there and any minor difference in indenting might be worth avoiding the overhead and creating these temp files all the time.

jsgf · 2018-08-14T17:53:44Z

cc @sunshowers

jsgf · 2018-08-14T18:01:12Z

It might be a good idea to just use the statically linked Rustfmt for the tooltips, since most of the time there's almost null work to be done there and any minor difference in indenting might be worth avoiding the overhead and creating these temp files all the time.

I think the specific formatting only matters if its persisted in a file. I agree the precise formatting of transient output is less important.

sunshowers · 2018-08-14T18:02:13Z

Thanks, this is definitely the direction I was envisioning.

This is not entirely correct yet, since we should pass full Config in a file to executed Rustfmt, since LSP may request additional formatting options and this currently just uses default Rustfmt configuration either via rustfmt.toml or by using default, but with some RLS-specific options.

What sorts of configs could the LSP pass in here? To some extent, using an external formatter means that you want that to determine what to do. In particular, for the Facebook use case we want a single consistent format result controlled entirely by rustfmt.toml and independent of whatever each user's LSP config.

Could potential version skew be a problem wrt formatter configs? e.g. RLS is newer and passes in a config that an older rustfmt doesn't understand.

Xanewok · 2018-08-16T00:29:46Z

What sorts of configs could the LSP pass in here? To some extent, using an external formatter means that you want that to determine what to do. In particular, for the Facebook use case we want a single consistent format result controlled entirely by rustfmt.toml and independent of whatever each user's LSP config.

Agreed, however the LSP provides FormattingOptions which specifies at least tabSize and insertSpaces, so we try to at least respect that, regardless of the formatting options specified. It'd might be worthwile to change that on the LSP side of things to let the server/formatter figure out the default values itself.

Could potential version skew be a problem wrt formatter configs? e.g. RLS is newer and passes in a config that an older rustfmt doesn't understand.

That's technically possible now, since we use the statically linked Rustfmt config/capabilities and encode it for the external Rustfmt to use.
It seems that we only care about the in-process config when we set the LSP formatting options (indent, tabs/spaces) but in other places where we do we'll prefer to use the internal Rustfmt anyway, so it might be actually possible to omit the config and just rely to the default one (e.g. in rustfmt.toml etc.)

nrc · 2018-08-24T01:56:03Z

The drawback to using the external Rustfmt is that we have to pass the entire config via file, so here it's done by creating a temporary file.

We should add some CLI API to Rustfmt to get around this, e.g., passing a top-level config as an env var

nrc · 2018-08-24T01:58:57Z

Could potential version skew be a problem wrt formatter configs? e.g. RLS is newer and passes in a config that an older rustfmt doesn't understand.

We should only pass stable options to external Rustfmt, since we can't promise that it was built using nightly and therefore might not accept unstable options. Given that, we should only have a problem if we pass new args to an old rustfmt, but I think we should be very conservative about what we pass, so should be OK.

nrc

Code looks good. Please remember to add the new option to the VSCode extension too.

nrc · 2018-08-24T02:00:22Z

Cargo.toml

@@ -12,6 +12,9 @@ categories = ["development-tools"]

 build = "build.rs"

+[patch.crates-io]
+rustfmt-nightly = { git = "https://github.com/Xanewok/rustfmt", branch = "file-lines-ser-json" }


Can we remove this now?

nrc · 2018-08-24T02:02:27Z

src/actions/format.rs

+}
+
+
+pub trait Formatter {


This trait seems like over-abstraction. Since we have the enum why not just use an inherent function on the enum and call different functions. Alternatively, keep the trait but dump the enum, since they are basically doing the same thing.

It somehow felt cleaner doing this this way than writing two bare format_internal(String, Config) -> .. and format_external(PathBuf, PathBuf, String, Config) -> ... to be called from inherent function but you're right that there's no benefit apart from being able to implement the same function signature for different variants; I'll change that.

nrc · 2018-08-24T02:05:09Z

Have to finish this one bit, but in general this should be ready for a review!

Looks like we're still waiting on this? Should I land this and follow up or do you want to wait?

Xanewok · 2018-08-24T08:36:56Z

When trying to use rustfmt 0.99.3 instead of the branch:

error: found removed `do catch` syntax                 
    --> /home/xanewok/.cargo/registry/src/github.com-1ecc6299db9ec823/rustc-ap-syntax-230.0.0/parse/parser.rs:1761:43
     |                                                                    
1761 |             let pat_arg: PResult<'a, _> = do catch {
     |                                           ^^
     |                                                              
     = help: Following RFC #2388, the new non-placeholder syntax is `try`
                                                                  
error: aborting due to previous error            
                                                    
error: Could not compile `rustc-ap-syntax`.

@nrc I believe we need the rustc-ap-*-235 from rust-lang/rustfmt@04d804c (0.99.4? 😢 ) now

Xanewok · 2018-08-24T08:43:01Z

Wait, no, this error happens because of latest rustc
rustc 1.30.0-nightly (63d66494a 2018-08-23)
both here and Rustfmt master

EDIT: Still blocked on rust-lang/rustfmt#2965 (comment)

FIXME: It would be clean to support formatting options passed via Formatter:format() trait method rather than relying on it to 'just' work for now.

External ones have to create temporary files to pass configs and whatnot and we can technically issue a lot of these hover requests fast - these will be short and the exact formatting does not matter, so we'll just use the internal one instead.

Also bump Racer to use single version of rustc-ap-* libs (v370)

Xanewok · 2018-08-27T15:36:55Z

@nrc this should be ready to review now

For rust-lang/rls#990.

nrc · 2018-08-28T02:06:24Z

src/actions/format.rs

+        .map_err(|_| "Config file could not be created".to_string())?)
+}
+
+fn gen_config_file(config: &Config) -> Result<(File, PathBuf), String> {


Could you open an issue for passing a config file as an env var please? And maybe stick a FIXME in here.

nrc · 2018-08-28T02:06:42Z

Thanks, looks good!

@nrc

Update RLS and Rustfmt RLS * Allow project model to download crates ([#1020](rust-lang/rls#1020)) * Support simple external builds ([#988](rust-lang/rls#988)) * Support using external Rustfmt ([#990](rust-lang/rls#990)) Rustfmt (0.99.4) * Format chains with comment ([#2899](https://github.com/rust-lang-nursery/rls/pull/2899)) * Do not show wildcard pattern in slice pattern ([#2912](https://github.com/rust-lang-nursery/rls/pull/2912)) * Impl only use ([#2951](https://github.com/rust-lang-nursery/rls/pull/2951)) * ... and [more](rust-lang/rustfmt@5c9a2b6...1c40881) Bumped in tandem to pull a single version of `rustc-ap-*` libs. r? @nrc

For rust-lang/rls#990.

Xanewok mentioned this pull request Aug 14, 2018

Expose FileLines JSON representation rust-lang/rustfmt#2915

Merged

Xanewok changed the title ~~External rustfmt~~ Support using external Rustfmt Aug 14, 2018

Xanewok commented Aug 14, 2018

View reviewed changes

Xanewok force-pushed the external-rustfmt branch 3 times, most recently from 895488d to 454d013 Compare August 20, 2018 14:30

nrc reviewed Aug 24, 2018

View reviewed changes

Xanewok added 6 commits August 27, 2018 17:00

Support external rustfmt

21e6892

Spawn external rustfmt at project root

97a8a6e

FIXME: It would be clean to support formatting options passed via Formatter:format() trait method rather than relying on it to 'just' work for now.

Use Formatter interface in hover

98632d8

Respect config when formatting with external Rustfmt

92ade8b

Use internal Rustfmt for hover tooltips

f5710b0

External ones have to create temporary files to pass configs and whatnot and we can technically issue a lot of these hover requests fast - these will be short and the exact formatting does not matter, so we'll just use the internal one instead.

Replace Formatter trait with inherent impl

3dc4592

Xanewok force-pushed the external-rustfmt branch from 454d013 to 3dc4592 Compare August 27, 2018 15:09

Use upstream Rustfmt 0.99.4

fe62a7c

Also bump Racer to use single version of rustc-ap-* libs (v370)

Xanewok added a commit to Xanewok/rls-vscode that referenced this pull request Aug 27, 2018

Add rust.rustfmt_path

3a36f2b

For rust-lang/rls#990.

Xanewok mentioned this pull request Aug 27, 2018

Add rust.rustfmt_path rust-lang/vscode-rust#412

Merged

nrc approved these changes Aug 28, 2018

View reviewed changes

nrc merged commit 6bda4d5 into rust-lang:master Aug 28, 2018

Xanewok mentioned this pull request Aug 31, 2018

Update RLS and Rustfmt rust-lang/rust#53870

Merged

fj pushed a commit to fj/rls-vscode that referenced this pull request Feb 17, 2019

Add rust.rustfmt_path

4e9b0cb

For rust-lang/rls#990.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support using external Rustfmt #990

Support using external Rustfmt #990

Xanewok commented Aug 14, 2018

Xanewok Aug 14, 2018

Xanewok commented Aug 14, 2018

jsgf commented Aug 14, 2018

jsgf commented Aug 14, 2018

sunshowers commented Aug 14, 2018 •

edited

Loading

Xanewok commented Aug 16, 2018

nrc commented Aug 24, 2018

nrc commented Aug 24, 2018

nrc left a comment

nrc Aug 24, 2018

nrc Aug 24, 2018

Xanewok Aug 24, 2018

nrc commented Aug 24, 2018

Xanewok commented Aug 24, 2018 •

edited

Loading

Xanewok commented Aug 24, 2018 •

edited

Loading

Xanewok commented Aug 27, 2018

nrc Aug 28, 2018

nrc commented Aug 28, 2018

Support using external Rustfmt #990

Support using external Rustfmt #990

Conversation

Xanewok commented Aug 14, 2018

Xanewok Aug 14, 2018

Choose a reason for hiding this comment

Xanewok commented Aug 14, 2018

jsgf commented Aug 14, 2018

jsgf commented Aug 14, 2018

sunshowers commented Aug 14, 2018 • edited Loading

Xanewok commented Aug 16, 2018

nrc commented Aug 24, 2018

nrc commented Aug 24, 2018

nrc left a comment

Choose a reason for hiding this comment

nrc Aug 24, 2018

Choose a reason for hiding this comment

nrc Aug 24, 2018

Choose a reason for hiding this comment

Xanewok Aug 24, 2018

Choose a reason for hiding this comment

nrc commented Aug 24, 2018

Xanewok commented Aug 24, 2018 • edited Loading

Xanewok commented Aug 24, 2018 • edited Loading

Xanewok commented Aug 27, 2018

nrc Aug 28, 2018

Choose a reason for hiding this comment

nrc commented Aug 28, 2018

sunshowers commented Aug 14, 2018 •

edited

Loading

Xanewok commented Aug 24, 2018 •

edited

Loading

Xanewok commented Aug 24, 2018 •

edited

Loading