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

Correctly format extern crate conflict resolution help #45820

Closed
wants to merge 1 commit into from
Closed

Correctly format extern crate conflict resolution help #45820

wants to merge 1 commit into from

Conversation

Cldfire
Copy link
Contributor

@Cldfire Cldfire commented Nov 7, 2017

Closes #45799.

Following the advice given in the issue, I changed the parsing of extern crate statements to grab the span before it includes the semicolon, fixing the formatting issue. All of the UI tests, oddly enough, passed on the first run without any modification (hopefully CI tells the same story).

There was one small problem that I had to work around. If extern crate std; was placed on the very first line of the file, the outputted suggestion formatting was malformed. I solved this by checking to make sure that the new_binding's span didn't equal DUMMY_SP when determining which binding to display the suggestion for. Note that, although unrelated to this particular PR, the output for line 1 vs. any other line still differs:

~/p/local-rust-dev-testing ❯❯❯ cargo +local-s1 build
   Compiling local-rust-dev-testing v0.1.0 (file:///home/cldfire/programming_projects/local-rust-dev-testing)
error[E0259]: the name `std` is defined multiple times
  |
  = note: `std` must be defined only once in the type namespace of this module
help: You can use `as` to change the binding name of the import
  |
1 | extern crate std as Otherstd;
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
~/p/local-rust-dev-testing ❯❯❯ cargo +local-s1 build
   Compiling local-rust-dev-testing v0.1.0 (file:///home/cldfire/programming_projects/local-rust-dev-testing)
error[E0259]: the name `std` is defined multiple times
 --> src/main.rs:2:1
  |
2 | extern crate std;
  | ^^^^^^^^^^^^^^^^ `std` reimported here
  |
  = note: `std` must be defined only once in the type namespace of this module
help: You can use `as` to change the binding name of the import
  |
2 | extern crate std as Otherstd;
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

It's an incredibly niche circumstance, though, so I don't think it's worth opening an issue about. If anyone believes otherwise, feel free to do so.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Cldfire
Copy link
Contributor Author

Cldfire commented Nov 7, 2017

r? @estebank

@rust-highfive rust-highfive assigned estebank and unassigned pnkfelix Nov 7, 2017
@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 7, 2017
@estebank
Copy link
Contributor

estebank commented Nov 7, 2017

@bors r+ rollup

Thank you @Cldfire!

@bors
Copy link
Contributor

bors commented Nov 7, 2017

📌 Commit 7eaa758 has been approved by estebank

@kennytm
Copy link
Member

kennytm commented Nov 7, 2017

rustfmt's test has failed in #45822. I suspect this PR is responsible as the diffs all talk about extern crate.

[01:25:08]      Running build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/deps/system-05e4e3e335aa8f84
[01:25:08] 
[01:25:08] running 10 tests
[01:25:08] test format_lines_errors_are_reported ... ok
[01:25:08] test coverage_tests ... ok
[01:25:08] test checkstyle_test ... ok
[01:25:08] test rustfmt_diff_make_diff_tests ... ok
[01:25:08] test rustfmt_diff_no_diff_test ... ok
[01:25:08] test stdin_formatting_smoke_test ... ok
[01:25:08] test string_eq_ignore_newline_repr_test ... ok
[01:25:09] test system_tests ... FAILED
[01:25:09] test idempotence_tests ... FAILED
[01:25:09] test self_tests ... FAILED
[01:25:09] 
[01:25:09] failures:
[01:25:09] 
[01:25:09] ---- system_tests stdout ----
[01:25:09] 	
[01:25:09] Mismatch at tests/source/extern.rs:1:
[01:25:09]  // rustfmt-normalize_comments: true⏎
[01:25:09]  ⏎
[01:25:09] +extern crate foo⏎
[01:25:09]  extern crate foo as bar;⏎
[01:25:09] -extern crate foo;⏎
[01:25:09]  ⏎
[01:25:09] -extern crate chrono;⏎
[01:25:09] -extern crate dotenv;⏎
[01:25:09] -extern crate futures;⏎
[01:25:09] +extern crate chrono⏎
[01:25:09] +extern crate dotenv  // ;⏎
[01:25:09] +extern crate futures // ;;⏎
[01:25:09]  ⏎
[01:25:09] -extern crate bar;⏎
[01:25:09] -extern crate foo;⏎
[01:25:09] +extern crate bar⏎
[01:25:09] +extern crate foo // ;;⏎
[01:25:09]  ⏎
[01:25:09]  extern "C" {⏎
[01:25:09]      fn c_func(x: *mut *mut libc::c_void);⏎
[01:25:09] 
[01:25:09] Mismatch at tests/source/multiple.rs:6:
[01:25:09]  ⏎
[01:25:09]  ⏎
[01:25:09]  #[attr1]⏎
[01:25:09] -extern crate foo;⏎
[01:25:09] +extern crate foo // ;⏎
[01:25:09]  #[attr2]⏎
[01:25:09]  #[attr3]⏎
[01:25:09] -extern crate foo;⏎
[01:25:09] +extern crate foo // ;⏎
[01:25:09]  #[attr1]⏎
[01:25:09] -extern crate foo;⏎
[01:25:09] +extern crate foo // ;⏎
[01:25:09]  #[attr2]⏎
[01:25:09]  #[attr3]⏎
[01:25:09]  extern crate foo;⏎
[01:25:09] 
[01:25:09] Mismatch at tests/source/multiple.rs:55:
[01:25:09]      extern crate foo;⏎
[01:25:09]      #[attr2]⏎
[01:25:09]      #[attr3]⏎
[01:25:09] -    extern crate foo;⏎
[01:25:09] -}⏎
[01:25:09] +    extern crate foo;}⏎
[01:25:09]  ⏎
[01:25:09]  #[rustfmt_skip]⏎
[01:25:09]  fn qux(a: dadsfa,   // Comment 1⏎
[01:25:09] Ran 289 system tests.
[01:25:09] thread 'system_tests' panicked at 'assertion failed: `(left == right)`
[01:25:09]   left: `2`,
[01:25:09]  right: `0`: 2 system tests failed', /checkout/src/tools/rustfmt/tests/system.rs:52:4
[01:25:09] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:25:09] 
[01:25:09] ---- idempotence_tests stdout ----
[01:25:09] 	
[01:25:09] Mismatch at tests/target/extern.rs:1:
[01:25:09]  // rustfmt-normalize_comments: true⏎
[01:25:09]  ⏎
[01:25:09] -extern crate foo as bar;⏎
[01:25:09] -extern crate foo;⏎
[01:25:09] +extern crate foo⏎
[01:25:09] +extern crate foo as bar // ;;⏎
[01:25:09]  ⏎
[01:25:09] -extern crate chrono;⏎
[01:25:09] -extern crate dotenv;⏎
[01:25:09] +extern crate chrono // ;⏎
[01:25:09] +extern crate dotenv // ;⏎
[01:25:09]  extern crate futures;⏎
[01:25:09]  ⏎
[01:25:09] -extern crate bar;⏎
[01:25:09] +extern crate bar // ;⏎
[01:25:09]  extern crate foo;⏎
[01:25:09]  ⏎
[01:25:09]  extern "C" {⏎
[01:25:09] 
[01:25:09] Mismatch at tests/target/attrib-extern-crate.rs:1:
[01:25:09]  // Attributes on extern crate.⏎
[01:25:09]  ⏎
[01:25:09]  #[Attr1]⏎
[01:25:09] -extern crate Bar;⏎
[01:25:09] +extern crate Bar ;⏎
[01:25:09]  #[Attr2]⏎
[01:25:09]  #[Attr2]⏎
[01:25:09] -extern crate Baz;⏎
[01:25:09] +extern crate Baz ;⏎
[01:25:09]  extern crate Foo;⏎
[01:25:09]  ⏎
[01:25:09]  fn foo() {⏎
[01:25:09] 
[01:25:09] Mismatch at tests/target/attrib-extern-crate.rs:13:
[01:25:09]      #[Attr2]⏎
[01:25:09]      #[Attr2]⏎
[01:25:09]      extern crate Baz;⏎
[01:25:09] -    extern crate Foo;⏎
[01:25:09] -}⏎
[01:25:09] +    extern crate Foo;}⏎
[01:25:09] 
[01:25:09] Mismatch at tests/target/multiple.rs:6:
[01:25:09]  ⏎
[01:25:09]  ⏎
[01:25:09]  #[attr1]⏎
[01:25:09] -extern crate foo;⏎
[01:25:09] +extern crate foo // ;⏎
[01:25:09]  #[attr2]⏎
[01:25:09]  #[attr3]⏎
[01:25:09] -extern crate foo;⏎
[01:25:09] +extern crate foo // ;⏎
[01:25:09]  #[attr1]⏎
[01:25:09] -extern crate foo;⏎
[01:25:09] +extern crate foo // ;⏎
[01:25:09]  #[attr2]⏎
[01:25:09]  #[attr3]⏎
[01:25:09]  extern crate foo;⏎
[01:25:09] 
[01:25:09] Mismatch at tests/target/multiple.rs:55:
[01:25:09]      extern crate foo;⏎
[01:25:09]      #[attr2]⏎
[01:25:09]      #[attr3]⏎
[01:25:09] -    extern crate foo;⏎
[01:25:09] -}⏎
[01:25:09] +    extern crate foo;}⏎
[01:25:09]  ⏎
[01:25:09]  #[rustfmt_skip]⏎
[01:25:09]  fn qux(a: dadsfa,   // Comment 1⏎
[01:25:09] Ran 339 idempotent tests.
[01:25:09] thread 'idempotence_tests' panicked at 'assertion failed: `(left == right)`
[01:25:09]   left: `3`,
[01:25:09]  right: `0`: 3 idempotent tests failed', /checkout/src/tools/rustfmt/tests/system.rs:113:4
[01:25:09] 
[01:25:09] ---- self_tests stdout ----
[01:25:09] 	
[01:25:09] Mismatch at src/bin/cargo-fmt.rs:13:
[01:25:09]  #![cfg(not(test))]⏎
[01:25:09]  #![deny(warnings)]⏎
[01:25:09]  ⏎
[01:25:09] -extern crate getopts;⏎
[01:25:09] +extern crate getopts ;⏎
[01:25:09]  extern crate serde_json as json;⏎
[01:25:09]  ⏎
[01:25:09]  use std::env;⏎
[01:25:09] 
[01:25:09] Mismatch at src/bin/rustfmt-format-diff.rs:14:
[01:25:09]  ⏎
[01:25:09]  #![deny(warnings)]⏎
[01:25:09]  ⏎
[01:25:09] -extern crate env_logger;⏎
[01:25:09] -extern crate getopts;⏎
[01:25:09] +extern crate env_logger ;⏎
[01:25:09] +extern crate getopts    ;⏎
[01:25:09]  #[macro_use]⏎
[01:25:09] -extern crate log;⏎
[01:25:09] -extern crate regex;⏎
[01:25:09] +extern crate log ;⏎
[01:25:09] +extern crate regex      ;⏎
[01:25:09]  #[macro_use]⏎
[01:25:09] -extern crate serde_derive;⏎
[01:25:09] +extern crate serde_derive ;⏎
[01:25:09]  extern crate serde_json as json;⏎
[01:25:09]  ⏎
[01:25:09]  use std::{env, fmt, process};⏎
[01:25:09] 
[01:25:09] Mismatch at src/bin/rustfmt.rs:11:
[01:25:09]  #![cfg(not(test))]⏎
[01:25:09]  ⏎
[01:25:09]  ⏎
[01:25:09] -extern crate env_logger;⏎
[01:25:09] -extern crate getopts;⏎
[01:25:09] +extern crate env_logger ;⏎
[01:25:09] +extern crate getopts    ;⏎
[01:25:09]  extern crate rustfmt_nightly as rustfmt;⏎
[01:25:09]  ⏎
[01:25:09]  use std::{env, error};⏎
[01:25:09] 
[01:25:09] Mismatch at tests/system.rs:9:
[01:25:09]  // except according to those terms.⏎
[01:25:09]  ⏎
[01:25:09]  #[macro_use]⏎
[01:25:09] -extern crate log;⏎
[01:25:09] -extern crate regex;⏎
[01:25:09] -extern crate rustfmt_nightly as rustfmt;⏎
[01:25:09] +extern crate log ;⏎
[01:25:09] +extern crate regex                      ;⏎
[01:25:09] +extern crate rustfmt_nightly as rustfmt ;⏎
[01:25:09]  extern crate term;⏎
[01:25:09]  ⏎
[01:25:09]  use std::collections::HashMap;⏎
[01:25:09] 
[01:25:09] Mismatch at src/lib.rs:10:
[01:25:09]  ⏎
[01:25:09]  #![feature(rustc_private)]⏎
[01:25:09]  ⏎
[01:25:09] -extern crate diff;⏎
[01:25:09] +extern crate diff ;⏎
[01:25:09]  #[macro_use]⏎
[01:25:09] -extern crate log;⏎
[01:25:09] -extern crate regex;⏎
[01:25:09] -extern crate rustc_errors as errors;⏎
[01:25:09] -extern crate serde;⏎
[01:25:09] +extern crate log ;⏎
[01:25:09] +extern crate regex ;⏎
[01:25:09] +extern crate rustc_errors as errors ;⏎
[01:25:09] +extern crate serde ;⏎
[01:25:09]  #[macro_use]⏎
[01:25:09] -extern crate serde_derive;⏎
[01:25:09] -extern crate serde_json;⏎
[01:25:09] -extern crate strings;⏎
[01:25:09] -extern crate syntax;⏎
[01:25:09] -extern crate term;⏎
[01:25:09] +extern crate serde_derive ;⏎
[01:25:09] +extern crate serde_json ;⏎
[01:25:09] +extern crate strings ;⏎
[01:25:09] +extern crate syntax ;⏎
[01:25:09] +extern crate term ;⏎
[01:25:09]  extern crate unicode_segmentation;⏎
[01:25:09]  ⏎
[01:25:09]  use std::collections::HashMap;⏎
[01:25:09] Ran 6 self tests.
[01:25:09] thread 'self_tests' panicked at 'assertion failed: `(left == right)`
[01:25:09]   left: `5`,
[01:25:09]  right: `0`: 5 self tests failed', /checkout/src/tools/rustfmt/tests/system.rs:133:4
[01:25:09] 
[01:25:09] 
[01:25:09] failures:
[01:25:09]     idempotence_tests
[01:25:09]     self_tests
[01:25:09]     system_tests
[01:25:09] 
[01:25:09] test result: FAILED. 7 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out
[01:25:09] 
[01:25:09] error: test failed, to rerun pass '--test system'
[01:25:09] 
[01:25:09] 
[01:25:09] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--manifest-path" "/checkout/src/tools/rustfmt/Cargo.toml"
[01:25:09] expected success, got: exit code: 101
[01:25:09] 
[01:25:09] 
[01:25:09] You can disable the tool in `src/tools/toolstate.toml`
[01:25:09] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/cargo src/tools/rls src/tools/rustfmt src/tools/miri src/test/pretty src/test/run-pass/pretty src/test/run-fail/pretty src/test/run-pass-valgrind/pretty src/test/run-pass-fulldeps/pretty src/test/run-fail-fulldeps/pretty
[01:25:09] Build completed unsuccessfully in 0:34:18
[01:25:09] Makefile:54: recipe for target 'check-aux' failed
[01:25:09] make: *** [check-aux] Error 1

@Cldfire
Copy link
Contributor Author

Cldfire commented Nov 7, 2017

Yeah, that would make sense. This means I need to go fix rustfmt now following the steps in the contribution guidlines, correct?

@kennytm
Copy link
Member

kennytm commented Nov 7, 2017

@Cldfire Yes. If this can only be fixed by updating rustfmt, you should

  1. Edit src/tools/toolstate.toml and change the rustfmt entry to "Compiling"
  2. Ping @nrc
  3. Submit a PR to https://github.com/rust-lang-nursery/rustfmt/.
  4. The maintains will update both sides afterwards.

@bors r-

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2017
@nrc
Copy link
Member

nrc commented Nov 8, 2017

@Cldfire I don't think this approach is correct. AFAIK, item spans generally include the terminating semi-colon, so it would be weird to make extern crate different (to, e.g., struct Foo;). Logically too, I think we should include the whole item in its span because it's easier to find a smaller span, but hard to find a larger one.

I think a better solution is that when you need the span without the ; you synthesise it using the start of the item's span and the end of the ident's span. I'm not sure of the best way to do that. You might be able to use the tokens field on Item or you might need to add a span for the Ident to the ExternCrate variant.

@Cldfire
Copy link
Contributor Author

Cldfire commented Nov 8, 2017

@nrc alright, thank you for your input. I agree that that is probably a better idea.

I'll close this PR and open a new one that doesn't change extern crate's span when I have time.

@Cldfire Cldfire closed this Nov 8, 2017
bors added a commit that referenced this pull request Jan 28, 2018
Correctly format `extern crate` conflict resolution help

Closes #45799. Follow up to @Cldfire's #45820.

If the `extern` statement that will have a suggestion ends on a `;`, synthesize a new span that doesn't include it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[resolve] use rename suggestion is not well-formatted
7 participants