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

Add clippy and fix commands to x.py #56595

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@ljedrz
Copy link
Contributor

ljedrz commented Dec 7, 2018

Since they are kind of similar in nature, I have used the same approach as for cargo check. At least some of the boilerplate could probably be shared, but I'd prefer to gather some feedback before I decide to merge them more aggressively.

This works reasonably well for clippy; with -A clippy::all and some extra #![feature(rustc_private)]s almost the whole codebase can be processed. There are some concerns, though:

  • unlike check, in order to be able to traverse all the crates, some of them need to be marked with the #![feature(rustc_private)] attribute
  • -W clippy::all breaks on any error. Is there a way to produce errors but not have them break the progress?
  • I'm not sure how to redirect the errors in a way that would show colors; for now I was able to de-jsonize and print them (something not needed for check)

cargo fix is much more stubborn; it refuses to acknowledge crates like core and std, so it doesn't progress much at all.

Since this is a bit more tricky than I have envisioned, I need some guidance:

  • is this the right approach or am I doing something very wrong ^^?
  • why are the extra rustc_private features necessary? I was hoping for the same treatment as check
  • are changes in clippy and cargo fix needed e.g. in order to produce errors in the same manner as check or did I miss something?
  • do we need this level of file granularity (e.g. for futureproofing) or can check, clippy and fix files be condensed?

Hopes-to-fix: #53896

Cc @alexcrichton, @zackmdavis

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 7, 2018

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 8, 2018

☔️ The latest upstream changes (presumably #56258) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 10, 2018

bors added a commit that referenced this pull request Dec 11, 2018

Auto merge of #56600 - ljedrz:fix_edition, r=Mark-Simulacrum
bootstrap: fix edition

A byproduct of work on #56595; done with `cargo fix --edition`.

kennytm added a commit to kennytm/rust that referenced this pull request Dec 12, 2018

Rollup merge of rust-lang#56600 - ljedrz:fix_edition, r=Mark-Simulacrum
bootstrap: fix edition

A byproduct of work on rust-lang#56595; done with `cargo fix --edition`.

kennytm added a commit to kennytm/rust that referenced this pull request Dec 13, 2018

Rollup merge of rust-lang#56600 - ljedrz:fix_edition, r=Mark-Simulacrum
bootstrap: fix edition

A byproduct of work on rust-lang#56595; done with `cargo fix --edition`.

kennytm added a commit to kennytm/rust that referenced this pull request Dec 14, 2018

Rollup merge of rust-lang#56600 - ljedrz:fix_edition, r=Mark-Simulacrum
bootstrap: fix edition

A byproduct of work on rust-lang#56595; done with `cargo fix --edition`.

kennytm added a commit to kennytm/rust that referenced this pull request Dec 14, 2018

Rollup merge of rust-lang#56600 - ljedrz:fix_edition, r=Mark-Simulacrum
bootstrap: fix edition

A byproduct of work on rust-lang#56595; done with `cargo fix --edition`.
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 14, 2018

Thanks for the PR! I'm somewhat hesitant to merge this in though because it seems like it's presumably duplicating a lot of code already there for other aspects of rustbuild. Rustbuild already suffers from a good deal of duplication, so I think I'd personally prefer if we could try to explore strategies of deduplication before merging these in

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Dec 14, 2018

I would kind of like to merge this actually. It would be a big step towards moving the compiler to edition 2018.

@ljedrz

This comment has been minimized.

Copy link
Contributor

ljedrz commented Dec 15, 2018

@alexcrichton there is definitely room for improvement in terms of deduplication, that's one of the reasons for the WIP; I just wanted to make sure the general approach was fine before squeezing the code a bit more.

I'll rebase and deduplicate the code shortly.

@ljedrz ljedrz force-pushed the ljedrz:x_py_clippy_fix branch from 9135b3e to 0e43db1 Dec 15, 2018

@ljedrz ljedrz changed the title [WIP] Add clippy and fix commands to x.py Add clippy and fix commands to x.py Dec 15, 2018

@ljedrz

This comment has been minimized.

Copy link
Contributor

ljedrz commented Dec 15, 2018

@alexcrichton I updated the commit - the new commands are now integrated into check.rs.

bors added a commit that referenced this pull request Dec 15, 2018

Auto merge of #56600 - ljedrz:fix_edition, r=Mark-Simulacrum
bootstrap: fix edition

A byproduct of work on #56595; done with `cargo fix --edition`.

bors added a commit that referenced this pull request Dec 15, 2018

Auto merge of #56600 - ljedrz:fix_edition, r=Mark-Simulacrum
bootstrap: fix edition

A byproduct of work on #56595; done with `cargo fix --edition`.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 16, 2018

☔️ The latest upstream changes (presumably #56600) made this pull request unmergeable. Please resolve the merge conflicts.

@ljedrz ljedrz force-pushed the ljedrz:x_py_clippy_fix branch from 0e43db1 to aee9a7a Dec 16, 2018

@ljedrz

This comment has been minimized.

Copy link
Contributor

ljedrz commented Dec 16, 2018

Rebased.

@@ -23,6 +23,22 @@ pub struct Std {
pub target: Interned<String>,
}

fn args(kind: Kind) -> Vec<String> {
match kind {
Kind::Clippy => vec!["--".to_owned(), "-W".to_owned(), "clippy::all".to_owned()],

This comment has been minimized.

@sinkuu

sinkuu Dec 16, 2018

Contributor

-W clippy::all breaks on any error. Is there a way to produce errors but not have them break the progress?

--cap-lints warn should work.

This comment has been minimized.

@ljedrz

ljedrz Dec 17, 2018

Contributor

I managed to pass it into clippy arguments, but it doesn't seem to be enough 🤔.

@sinkuu

This comment has been minimized.

Copy link
Contributor

sinkuu commented Dec 16, 2018

unlike check, in order to be able to traverse all the crates, some of them need to be marked with the #![feature(rustc_private)] attribute

You can make Builder::cargo to add RUSTFLAGS="-Zforce-unstable-if-unmarked" instead of RUSTC_FORCE_UNSTABLE=1 when cmd == "clippy". Bootstrap's rustc shim adds the flag when RUSTC_FORCE_UNSTABLE is found, but cargo clippy invokes its own rustc driver.

I'm not sure how to redirect the errors in a way that would show colors; for now I was able to de-jsonize and print them (something not needed for check)

This is also due to rustc shim. It removes --message-format json from args passed to rustc, so rustc normally prints colored errors to stderr.

@ljedrz

This comment has been minimized.

Copy link
Contributor

ljedrz commented Dec 16, 2018

Thanks for the suggestions @sinkuu! I'll apply them soon.

Edit: I added RUSTFLAGS="-Zforce-unstable-if-unmarked" for clippy and fix, but I don't know how I can override --message-format json; any ideas?

@ljedrz ljedrz force-pushed the ljedrz:x_py_clippy_fix branch from aee9a7a to 5176bb1 Dec 17, 2018

@ljedrz ljedrz force-pushed the ljedrz:x_py_clippy_fix branch from 5176bb1 to 0820b48 Dec 17, 2018

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 24, 2018

☔️ The latest upstream changes (presumably #57094) made this pull request unmergeable. Please resolve the merge conflicts.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 26, 2018

☔️ The latest upstream changes (presumably #57108) made this pull request unmergeable. Please resolve the merge conflicts.

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jan 11, 2019

Ping-- what's the status of this PR? I see that @ljedrz asked about how to override --message-format json; above, but it doesn't seem like there's been any comment since?

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Jan 15, 2019

We could add an unstable flag that prints terminal color codes to the formatted diagnostic message in the json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment