Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

Add --prepare-for flag #98

Merged
merged 6 commits into from
May 10, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions cargo-fix/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,13 @@ pub fn run() -> Result<(), Error> {
Arg::with_name("broken-code")
.long("broken-code")
.help("Fix code even if it already has compiler errors"),
)
.arg(
Arg::with_name("edition")
.long("prepare-for")
.help("Fix warnings in preparation of an edition upgrade")
.takes_value(true)
.possible_values(&["2018"]),
),
)
.get_matches();
Expand Down Expand Up @@ -77,6 +84,15 @@ pub fn run() -> Result<(), Error> {
cmd.env("RUSTC_ORIGINAL", rustc);
}

// Trigger edition-upgrade mode. Currently only supports the 2018 edition.
info!("edition upgrade? {:?}", matches.value_of("edition"));
if let Some("2018") = matches.value_of("edition") {
info!("edition upgrade!");
let mut rustc_flags = env::var_os("RUSTFLAGS").unwrap_or_else(|| "".into());
rustc_flags.push("-W rust-2018-breakage -A unused -A nonstandard-style -A bad-style");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come these specific -A flags were required? I'd actually sort of expect that we don't pass any extra arguments other than the -W for the lint

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want just the breakage links here -- but other lints might trigger. And I don't know how to disabled everything but one lint group in a clever way :(

I'd like to have a way to disable all lints and than individually enable
the breakage lint group, but sadly -A warnings -D rust-2018-breakage
doesn't seem to work. So, let's list the default lint groups for now.

– commit message of fe2526e

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm is it bad though if other warnings are emitted? Or is it bad if we fix some otherwise fixable warnings along the way?

I'm worried about this being an ever-growing list :(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm all for fixing as many things as possible, but it might send the wrong message in this specific context. E.g., if I run cargo fix --prepare-for 2018 and it removes a bunch of (unused) muts I might end up thinking that this is required for the new edition.

I'll try and play around with filtering by the lint group that triggered a lint. If that's possible we can just add the -W rust-2018-breakage and skip fixing other lints.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can run without edition lints, and if any diagnostics appear, ask the user to fix them first. If none appear, do what you do now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True yeah we could filter, but I'd imagine that for most maintained codebases they're already warning-free, so I wouldn't expect something like this to come up much in practice

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't even mean filtering, just abort if there are any (and tell the user the reason for the abort)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have seen many new rustaceans who have crates that are full of warnings, because they're used to that from other languages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sure yeah that's a possibility, although I'm still not sure why we would go out of our way to worry about other lints. Everything should work A-OK if we purely pass -W rust-2018-breakage. The user will be informed, as they always are, of lints that couldn't be fixed. Anything that comes up which is fixable as part of the breakage lint is already fixed.

cmd.env("RUSTFLAGS", &rustc_flags);
}

// An now execute all of Cargo! This'll fix everything along the way.
//
// TODO: we probably want to do something fancy here like collect results
Expand Down
45 changes: 45 additions & 0 deletions cargo-fix/tests/all/edition_upgrade.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
//! Test that we can use cargo-fix to upgrade our code to work with the 2018
//! edition.
//!
//! We'll trigger the `absolute_path_starting_with_module` lint which should
//! transform a `use ::foo;` where `foo` is local to `use crate::foo;`.

use super::project;

#[test]
fn prepare_for_2018() {
let p = project()
.file(
"src/lib.rs",
r#"
#![feature(crate_in_paths)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh gee this is gonna make for a not-so-great user experience :(

If users don't have this enabled then rustfix will accidentally deduce that its fixes broke your crate, so they'll all be backed out. That's a bummer...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah…

Without the feature flag, we don't 'fix' anything, though, so I don't see how we could ever revert fixes?


mod foo {
pub const FOO: &str = "fooo";
}

mod bar {
use ::foo::FOO;
}

fn main() {
let x = ::foo::FOO;
}
"#,
)
.build();

let stderr = "\
[CHECKING] foo v0.1.0 (CWD)
[FIXING] src/lib.rs (2 fixes)
[FINISHED] dev [unoptimized + debuginfo]
";
p.expect_cmd("cargo-fix fix --prepare-for 2018")
.stdout("")
.stderr(stderr)
.run();

println!("{}", p.read("src/lib.rs"));
assert!(p.read("src/lib.rs").contains("use crate::foo::FOO;"));
assert!(p.read("src/lib.rs").contains("let x = crate::foo::FOO;"));
}
1 change: 1 addition & 0 deletions cargo-fix/tests/all/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,7 @@ fn diff(expected: &str, actual: &str) {
mod broken_build;
mod broken_lints;
mod dependencies;
mod edition_upgrade;
mod smoke;
mod subtargets;
mod warnings;