Strip leading | in match arm patterns

[nrc]

See discussion in rust-lang/style-team#119. We might need to revisit this if the discussion changes direction.

[topecongiro]

Should we have an option for preserving/removing the leading |?

[nrc]

I don't think we need an option here. I think we probably want to support #2234 as an option for those who want a leading |.

[Mike-Baker]

I would be interested in fixing this if someone could provide a hint regarding which files to start looking in.

[nrc]

@Mike-Baker awesome! You should look at https://github.com/rust-lang-nursery/rustfmt/blob/master/src/matches.rs in particular rewrite_match_arms. You probably want to look at https://github.com/rust-lang/rust/blob/master/src/libsyntax/ast.rs#L896-L901 too which is where match arms are defined

[Mike-Baker]

@nrc Thank you for the pointers, having a relevant starting point made it a lot easier to get to grips with rustfmt!

I have something I think works (added a new test and all tests pass) here or a squashed version

I based my changes off 84598bd because current master (1415a4d) is failing to build due to errors in rustc-ap-syntax

I am concerned that this looks a lot like an effective revert of ea3c01e and if in future we would like to support #2234 then we would need to revert this revert, also I think I have duplicated a test which was introduced in the aforementioned commit.

Questions:

1. Is it actually worth merging this or should I investigate Feature Request - Matching, no double indent (with/without leading vert) #2234 instead?
2. Should I remove the old test (match_with_beginning_vert) now that it is superseded by the new test (issue_2621) or is there a better kind of testing strategy I should be employing anyway?
3. Should I rebase onto master before creating a pull request or leave it as is?

[nrc]

Current master should be building now, please rebase.

The branch looks good, I don't think you should worry abou reverting ea3c01e - #2234 is a bit different to that in that it would be an option to always insert a leading |, not preserving the way | was used in the source. So I think landing your branch is a better plan than looking at #2234 (though that would be a great follow-up issue, if you are interested).

For the tests, I think match_with_beginning_vert needs renaming. You should have tests for no | at all, trailing | kept if necessary (I'm sure this alreay exists), leading | stripped, and maybe some variations with newlines/comments if you can think of some.

[nrc]

@Mike-Baker Hey, did you get a chance to rebase your branch? Any more questions I can help with?

[Mike-Baker]

@nrc Hi, sorry for the delay.

Previous weekends I have been running into errors

    --> /Users/Mike/.cargo/registry/src/github.com-1ecc6299db9ec823/rustc-ap-rustc_errors-113.0.0/emitter.rs:1393:39
     |
1393 |     (a_start..a_end + extra).contains(&b_start)
     |                                       ^^^^^^^^
     |                                       |
     |                                       expected usize, found &usize
     |                                       help: consider removing the borrow: `b_start`
     |
     = note: expected type `usize`
                found type `&usize`

error: aborting due to 2 previous errors

(Fixed)

and

error[E0599]: no method named `end` found for type `std::ops::RangeInclusive<u128>` in the current scope
   --> /Users/Mike/.cargo/registry/src/github.com-1ecc6299db9ec823/rustc-ap-rustc_target-121.0.0/abi/mod.rs:559:37
    |
559 |         let end = *self.valid_range.end();
    |                                     ^^^ field, not a method
    |
    = help: did you mean to write `self.valid_range.end` instead of `self.valid_range.end(...)`?
    = help: items from traits can only be used if the trait is in scope
    = note: the following trait is implemented but not in scope, perhaps add a `use` for it:
            candidate #1: `use std::ops::RangeBounds;`

error: aborting due to 2 previous errors

(Fixed)

Then

error: process didn't exit successfully: `/Users/Mike/sync/programming/rust/rustfmt/target/debug/deps/rustfmt_nightly-ed88e69f497ca24a` (signal: 11, SIGSEGV: invalid memory reference)

(still in effect)

This is on OSX with

cargo 1.28.0-nightly (f352115d5 2018-05-15)
rustc 1.28.0-nightly (952f344cd 2018-05-18)
commit e83c7ff0056d522a722e26c24e7707f5d84adba4
Merge: dab99b6 912e4bd
Author: Nick Cameron <[email protected]>
Date:   2018-05-19 11:47:36 +1200

    Merge pull request #2723 from topecongiro/rustc-ap-syntax
    
    Update rustc-ap-syntax

[nrc]

@Mike-Baker you'll need to rustup update and rebase to current master so that your compiler libs match the rustfmt libs. I think that should fix these issues, let me know if it doesn't.

[nrc]

@Mike-Baker did you get a chance to rebase and did that solve your problems? Anything else I can help with?

[Mike-Baker]

@nrc Sorry for the delay, I have finally got everything building again so I have rebased and got the tests in line.

Here are the changes since last time.

Here is the pull request

[Mike-Baker]

Minor notes: It seems that things don't build on current nightly (fails with error: failed to run custom build command for `rustc-ap-rustc_target v164.0.0` process didn't exit successfully: `./rustfmt/target/debug/build/rustc-ap-rustc_target-7693b08837a4e8f0/build-script-build` (signal: 11, SIGSEGV: invalid memory reference))

But it does build on nightly-2018-06-20.

I also managed to get to a state somewhere that running cargo test wasn't correctly replacing the rustfmt executable which I fixed by nuking the target directory, I'm sorry to say that if I had figured this out earlier I could have saved a lot of time.

[nrc]

Closed by #2804