Add `--prepare-for` flag #98

killercup · 2018-05-09T20:04:33Z

This requires

#![feature(crate_in_paths)]
#![deny(absolute_path_starting_with_module)]

on rustc 1.27.0-nightly (8ff4b4206 2018-05-08), so we're not really there yet.

This requires #![feature(crate_in_paths)] #![deny(absolute_path_starting_with_module)] on `rustc 1.27.0-nightly (8ff4b4206 2018-05-08)`, so we're not really there yet.

alexcrichton

👍

alexcrichton · 2018-05-09T20:40:09Z

cargo-fix/src/cli.rs

+                .arg(
+                    Arg::with_name("edition")
+                        .long("prepare-for")
+                        .help("Fix warnigns in preparation of an edition upgrade")


alexcrichton · 2018-05-09T20:40:38Z

cargo-fix/tests/all/edition_upgrade.rs

+            r#"
+                #![allow(unused)]
+                #![feature(crate_in_paths)]
+                #![warn(absolute_path_starting_with_module)]


This shouldn't be necessary, right?

alexcrichton · 2018-05-09T20:40:57Z

cargo-fix/src/cli.rs

+        info!("edition upgrade!");
+        let mut rustc_flags = env::var_os("RUSTC_FLAGS").unwrap_or_else(|| "".into());
+        rustc_flags.push("-A warnings -W rust_2018_breakage");
+        cmd.env("RUSTC_FLAGS", &rustc_flags);


I think this should be RUSTFLAGS?

Fix env var name, list explicit allow lints 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.

alexcrichton · 2018-05-09T21:14:34Z

cargo-fix/src/cli.rs

+    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");


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

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

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 :(

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.

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.

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

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

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

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.

alexcrichton · 2018-05-09T21:15:06Z

cargo-fix/tests/all/edition_upgrade.rs

+            "src/lib.rs",
+            r#"
+                #![allow(unused)]
+                #![feature(crate_in_paths)]


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...

Yeah…

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

alexcrichton · 2018-05-10T15:47:46Z

Ok after some conversation on gitter it looks like we're adding another step to the cheat sheet which is that you need to add #![feature(rust_2018_preview)] to your crate to do anything.

@killercup given that can we, in the meantime, read the crate root source file and just grep for the string "rust_2018_preview"? If that string is present we can turn on the breakage lint but otherwise I don't think we should automatically enable it.

Otherwise we're gonna get a lot of bug reports I think because rustfix says "please file a bug if a fix was applied and it then failed"

Manishearth · 2018-05-10T15:51:31Z

Otherwise we're gonna get a lot of bug reports I think because rustfix says "please file a bug if a fix was applied and it then failed"

To be clear, this can't happen due to lack of a feature flag, the lints are designed to only trigger if the features required for their replacement are enabled. The failure case here is that not having the feature means you don't get the warning, not that not having the feature gives you a broken warning.

alexcrichton · 2018-05-10T15:57:36Z

Ok cool, I seem to have been confused then. I remembered running cargo fix and getting spurious 'please report this bug' warnings because fixes were applied for features that were not activated. That doesn't seems to be happening now though so it sounds like we don't have to search the crate root for a #![feature] annotation, but we will still require users to enable the feature before running cargo fix for it to do anything.

alexcrichton · 2018-05-10T17:08:19Z

👍

WIP: Add --prepare-for flag

cea8fd1

This requires #![feature(crate_in_paths)] #![deny(absolute_path_starting_with_module)] on `rustc 1.27.0-nightly (8ff4b4206 2018-05-08)`, so we're not really there yet.

killercup requested a review from alexcrichton May 9, 2018 20:04

alexcrichton reviewed May 9, 2018

View reviewed changes

killercup added 2 commits May 9, 2018 23:05

Fix typo

bb18250

alexcrichton reviewed May 9, 2018

View reviewed changes

No need for the allow(unused) anymore

43d5f18

killercup mentioned this pull request May 9, 2018

Document how to transition between editions rust-lang/edition-guide#4

Merged

Use rust_2018_preview feature

cd641cd

killercup changed the title ~~WIP: Add --prepare-for flag~~ Add --prepare-for flag May 10, 2018

Let's not disable lints

a956e9e

killercup force-pushed the prepare-for-flag branch from 133b2be to a956e9e Compare May 10, 2018 16:50

alexcrichton merged commit 5649ad4 into master May 10, 2018

alexcrichton deleted the prepare-for-flag branch May 10, 2018 17:08

killercup mentioned this pull request Jul 17, 2018

Cargo fix --prepare-for should only apply edition fixes rust-lang/cargo#5738

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--prepare-for` flag #98

Add `--prepare-for` flag #98

killercup commented May 9, 2018

alexcrichton left a comment

alexcrichton May 9, 2018

alexcrichton May 9, 2018

alexcrichton May 9, 2018

alexcrichton May 9, 2018

killercup May 9, 2018

alexcrichton May 9, 2018

killercup May 10, 2018

oli-obk May 10, 2018

alexcrichton May 10, 2018

oli-obk May 10, 2018

oli-obk May 10, 2018

alexcrichton May 10, 2018

alexcrichton May 9, 2018

killercup May 9, 2018

alexcrichton commented May 10, 2018

Manishearth commented May 10, 2018

alexcrichton commented May 10, 2018

alexcrichton commented May 10, 2018

Add --prepare-for flag #98

Add --prepare-for flag #98

Conversation

killercup commented May 9, 2018

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented May 10, 2018

Manishearth commented May 10, 2018

alexcrichton commented May 10, 2018

alexcrichton commented May 10, 2018

Add `--prepare-for` flag #98

Add `--prepare-for` flag #98