Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to format inline functions with dependencies through stdin #1290

Closed
andreastt opened this issue Jan 26, 2017 · 14 comments
Closed

Ability to format inline functions with dependencies through stdin #1290

andreastt opened this issue Jan 26, 2017 · 14 comments
Labels

Comments

@andreastt
Copy link

andreastt commented Jan 26, 2017

(Please excuse my lack of proper internals parlance, and I apologise that I am probably mixing a few issues here.)

I often only format selections of a file by passing these to rustfmt by stdin and replacing the selection with the stdout that comes back. This works great for top-level functions that are not part of an implentation, but not so good for functions that have dependencies such as self. In this case, rustfmt trips up because &self is missing a type definition.

Given this code:

impl ToJson for WebDriverError {
    fn to_json(&self) -> Json {
        let mut data = BTreeMap::new();
        data.insert("error".to_string(), self.status_code().to_json());
        data.insert("message".to_string(), self.message.to_json());
        Json::Object(data)
    }
}

Selecting lines 2 through 7 (the to_json function) and passing it through rustfmt yields the following error:

error: expected one of `::` or `:`, found `)`
 --> stdin:1:21
  |rustfmt: exit 2
|
1 |     fn to_json(&self) -> Json {
  |                     ^

If I’m formatting one such function in the middle of a long impl block, I often find myself adding a impl F { … } to wrap the function so that I will not have to format potentially several hundred lines of code.

What is also problematic is that selecting the inner function deletes the selection because the stdout is nothing. It would be less annoying if stdout was equal to stdin in so the selection could be preserved whenever there is an error.


In other cases where the function is syntactically correct and without dependencies, rustfmt disregards the indentation of the lines because it thinks it is a top-level function. When rustfmt runs in stdin mode, it should preserve this whitespace:

impl Foo {
    pub fn bar() {
        let incorrectly_indented =
            42;
    }
}

Selecting lines 2 through 4 in the above example will result in this:

impl Foo {
pub fn bar() {
    let incorrectly_indented = 42;
}

}

Whereas I would expect this to happen:

impl Foo {
    pub fn bar() {
        let incorrectly_indented = 42;
    }
}

Let me know if I should break this up into multiple issues. Because I’m unfamiliar with the rustfmt terminology, I understand some of this might be replicated in other issues or be available as config options.

@nrc
Copy link
Member

nrc commented Jan 26, 2017

SO, the problem here is that rustfmt uses the Rust parser and that expects to start at the module level, so anything you pass it must be an item that can appear in a module. Passing a method doesn't work because they must be inside an impl.

IDEs have a similar problem when you try to reformat a selection, so it is something I very much want to solve. There are a few solutions:

  • round up the selection to the nearest module-level AST node, in this case an impl. The downside of this is that it can include a lot of code - you might want to format a one line expression and end up with a 1000 line impl.
  • be smarter about how we use the parser. We'd have to pass info to Rustfmt saying what context to start the parser in, e.g., "expect an item inside an impl" rather than always defaulting to "expect an item inside a module". I'm not sure how you'd specify or compute that info, and you'd still need to 'round up' to an AST node, but at least the AST node could be smaller.
  • use a more flexible parser than the Rust one - this would be a lot of work

On top of any of these you'd still need to solve the 'starting indent' problem, but that at least should be very easy.

@andreastt
Copy link
Author

I would’ve guessed this to be a complicated issue, and this is why I have deliberately delayed filing it until rustfmt achieved some maturity (-:

I suspect the easiest thing I can do here as a user is to write a shell script that places anything I pass by stdin that begins with fn inside an impl X { … }, stripping that on its way out.

About the two other issues, should I file separate issues for these?

  • On encountering an error in stdin mode, to not have it replace the selection with nothing
  • In stdin mode (I guess?), preserve whitespace or indentation level at the beginning of each line

Despite these issues, I want to use this opportunity to say how much I appreciate the work you and others have put into rustfmt. It makes it an enjoyable experience to work with Rust in editors that rely heavily on integrating with the surrounding system.

@nrc
Copy link
Member

nrc commented Jan 26, 2017

It's probably worth filing an issue for the error on stdin stuff. The whitespace thing is tied very closely to knowing the context for parsing so can just be dealt with here.

Thanks, I'm glad its helpful!

@topecongiro
Copy link
Contributor

Triage: they both seems to be fixed on the current master.

@andreastt
Copy link
Author

Both issues I described above still reproduces on master at the time of writing this:

error: expected one of `::` or `:`, found `)`
 --> |rustfmt: exit 2
stdin:1:21
  |
1 |     fn to_json(&self) -> Json {
  |                     ^
impl Foo {
pub fn bar() {
    let incorrectly_indented = 42;
}

}

@topecongiro
Copy link
Contributor

@andreastt Are you using rustfmt-nightly? I could not reproduce the issues with it. Could you please try with it?

@andreastt
Copy link
Author

I used cargo install -f from 5b1a84c on the master branch:

% git rev-parse master
5b1a84cc1e97d865e87df5a4700d668ddf962c5d
% cargo install -f
  Installing rustfmt v0.7.1 (file:///home/ato/src/rustfmt)
    Finished release [optimized] target(s) in 0.0 secs
   Replacing /home/ato/.cargo/bin/rustfmt
   Replacing /home/ato/.cargo/bin/cargo-fmt
% rustfmt --version
0.7.1 (5b1a84c)

If I try to install rustfmt-nightly through cargo install -f rustfmt-nightly, the binary isn’t executable:

% cargo install -f rustfmt-nightly
% rustfmt --version
rustfmt: error while loading shared libraries: libsyntax-1e6936bb0e2e42c3.so: cannot open shared object file: No such file or directory

Using rustfmt built from 5b1a84c:

% cat <<EOF | rustfmt
>     fn to_json(&self) -> Json {
>         let mut data = BTreeMap::new();
>         data.insert("error".to_string(), self.status_code().to_json());
>         data.insert("message".to_string(), self.message.to_json());
>         Json::Object(data)
>     }
> 
> EOF
error: expected one of `::` or `:`, found `)`
 --> stdin:1:21
  |
1 |     fn to_json(&self) -> Json {
  |                     ^

@nrc
Copy link
Member

nrc commented Jul 25, 2017

@andreastt 5b1a84c is pretty ancient. To get rustfmt-nightly running, see the last tip here https://github.com/rust-lang-nursery/rustfmt#tips

@andreastt
Copy link
Author

It appears that my upstream remote was https://github.com/nrc/rustfmt.git (and not https://github.com/rust-lang-nursery/rustfmt.git) which explains why 5b1a84c was at the HEAD of master after I pulled.

However, using the nightly rustc, I’m not able to start rustfmt:

% rustfmt
rustfmt: error while loading shared libraries: libsyntax-aa7bda3744948ec2.so: cannot open shared object file: No such file or directory

@nrc
Copy link
Member

nrc commented Jul 27, 2017

@andreastt you want the last tip, here - https://github.com/rust-lang-nursery/rustfmt#tips

@andreastt
Copy link
Author

andreastt commented Jul 28, 2017

@nrc By modifying the LD_LIBRARY_PATH I was able to get it running, but the result is still the same:

% git rev-parse master
f0cc060b4147ad7cfc6666dc1986adb213a0ceaf
% LD_LIBRARY_PATH=$(rustc --print sysroot)/lib:$LD_LIBRARY_PATH ./target/debug/rustfmt --version
0.2.1-nightly (f0cc060 2017-07-27)
% LD_LIBRARY_PATH=$(rustc --print sysroot)/lib:$LD_LIBRARY_PATH ./target/debug/rustfmt 
    fn to_json(&self) -> Json {
        let mut data = BTreeMap::new();
        data.insert("error".to_string(), self.status_code().to_json());
        data.insert("message".to_string(), self.message.to_json());
        Json::Object(data)
    }
^D
error: expected one of `::` or `:`, found `)`
 --> stdin:1:21
  |
1 |     fn to_json(&self) -> Json {
  |                     ^ expected one of `::` or `:` here

% LD_LIBRARY_PATH=$(rustc --print sysroot)/lib:$LD_LIBRARY_PATH ./target/debug/rustfmt 
    pub fn bar() {
        let incorrectly_indented =
            42;
    }
^D
pub fn bar() {
    let incorrectly_indented = 42;
}

@nrc
Copy link
Member

nrc commented Jul 28, 2017

Glad you got it working. I would actually expect to still see these issues, at least the way you are testing. If you format the whole file (including the impl), but just select the to_json function using the file_lines option, then you should get a decent reformatting (though not perfect, I think we will round up to the impl, though I might be wrong). I realise this is not quite what you want, but I think it is perhaps close enough? We can certainly improve the support for formatting selections too.

@andreastt
Copy link
Author

Let me explain my use case a bit better. The reason I want rustfmt to (1) preserve the default whitespace when formatting a selection of indented code and (2) be able to format selections that by themselves are invalid without an encasing impl F { … }, is that I want to pass a selection from my editor and pass it through by stdin to rustfmt. I then receive the stdout from rustfmt and replace the selection.

Quite often you have a long implementation where you made a change to a single function. In this case you don’t want to enforce a code style on the rest of the file you didn’t change, but you want the changed code to be conforming to the rustfmt coding style.

@nrc
Copy link
Member

nrc commented Aug 1, 2017

That is pretty much the use-case for the 'file-lines' feature existing and is how we do it in the RLS (although not using stdin/out directly). The one thing we don't really aim to do is 'learn' the indentation from the surrounding file - we assume you've indented the surrounding file properly (so we'll be indented the right amount for the selection, without reformatting the rest of the file).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants