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.

Sign up for GitHub

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

Jump to bottom

Rename --prepare-for to --edition, drop arg #5845

Merged
merged 1 commit into from
Aug 2, 2018
Merged

Rename --prepare-for to --edition, drop arg #5845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 15 additions & 11 deletions src/bin/cargo/commands/fix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,19 @@ pub fn cli() -> App {
)
.arg(
Arg::with_name("edition")
.long("edition")
.help("Fix in preparation for the next edition"),
)
.arg(
// This is a deprecated argument, we'll want to phase it out
// eventually.
Arg::with_name("prepare-for")
.long("prepare-for")
.help("Fix warnings in preparation of an edition upgrade")
.takes_value(true)
.possible_values(&["2018"]),
.possible_values(&["2018"])
.conflicts_with("edition")
.hidden(true),
)
.arg(
Arg::with_name("allow-no-vcs")
Expand All @@ -67,22 +76,16 @@ remaining warnings will be displayed when the check process is finished. For
example if you'd like to prepare for the 2018 edition, you can do so by
executing:

cargo fix --prepare-for 2018

Note that this is not guaranteed to fix all your code as it only fixes code that
`cargo check` would otherwise compile. For example unit tests are left out
from this command, but they can be checked with:

cargo fix --prepare-for 2018 --all-targets
cargo fix --edition

which behaves the same as `cargo check --all-targets`. Similarly if you'd like
to fix code for different platforms you can do:

cargo fix --prepare-for 2018 --target x86_64-pc-windows-gnu
cargo fix --edition --target x86_64-pc-windows-gnu

or if your crate has optional features:

cargo fix --prepare-for 2018 --no-default-features --features foo
cargo fix --edition --no-default-features --features foo

If you encounter any problems with `cargo fix` or otherwise have any questions
or feature requests please don't hesitate to file an issue at
Expand Down Expand Up @@ -121,7 +124,8 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
}
}
ops::fix(&ws, &mut ops::FixOptions {
edition: args.value_of("edition"),
edition: args.is_present("edition"),
prepare_for: args.value_of("prepare-for"),
compile_opts: opts,
allow_dirty: args.is_present("allow-dirty"),
allow_no_vcs: args.is_present("allow-no-vcs"),
Expand Down
64 changes: 52 additions & 12 deletions src/cargo/ops/fix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,12 @@ use util::paths;

const FIX_ENV: &str = "__CARGO_FIX_PLZ";
const BROKEN_CODE_ENV: &str = "__CARGO_FIX_BROKEN_CODE";
const PREPARE_FOR_ENV: &str = "__CARGO_FIX_PREPARE_FOR";
const EDITION_ENV: &str = "__CARGO_FIX_EDITION";

pub struct FixOptions<'a> {
pub edition: Option<&'a str>,
pub edition: bool,
pub prepare_for: Option<&'a str>,
pub compile_opts: CompileOptions<'a>,
pub allow_dirty: bool,
pub allow_no_vcs: bool,
Expand All @@ -48,9 +50,12 @@ pub fn fix(ws: &Workspace, opts: &mut FixOptions) -> CargoResult<()> {
opts.compile_opts.build_config.extra_rustc_env.push((key, "1".to_string()));
}

if let Some(edition) = opts.edition {
if opts.edition {
let key = EDITION_ENV.to_string();
opts.compile_opts.build_config.extra_rustc_env.push((key, "1".to_string()));
} else if let Some(edition) = opts.prepare_for {
opts.compile_opts.build_config.extra_rustc_env.push((
EDITION_ENV.to_string(),
PREPARE_FOR_ENV.to_string(),
edition.to_string(),
));
}
Expand Down Expand Up @@ -465,11 +470,23 @@ fn log_failed_fix(stderr: &[u8]) -> Result<(), Error> {
#[derive(Default)]
struct FixArgs {
file: Option<PathBuf>,
prepare_for_edition: Option<String>,
prepare_for_edition: PrepareFor,
enabled_edition: Option<String>,
other: Vec<OsString>,
}

enum PrepareFor {
Next,
Edition(String),
None,
}

impl Default for PrepareFor {
fn default() -> PrepareFor {
PrepareFor::None
}
}

impl FixArgs {
fn get() -> FixArgs {
let mut ret = FixArgs::default();
Expand All @@ -490,8 +507,10 @@ impl FixArgs {
}
ret.other.push(path.into());
}
if let Ok(s) = env::var(EDITION_ENV) {
ret.prepare_for_edition = Some(s);
if let Ok(s) = env::var(PREPARE_FOR_ENV) {
ret.prepare_for_edition = PrepareFor::Edition(s);
} else if env::var(EDITION_ENV).is_ok() {
ret.prepare_for_edition = PrepareFor::Next;
}
return ret
}
Expand All @@ -505,8 +524,15 @@ impl FixArgs {
if let Some(edition) = &self.enabled_edition {
cmd.arg("--edition").arg(edition);
}
if let Some(prepare_for) = &self.prepare_for_edition {
cmd.arg("-W").arg(format!("rust-{}-compatibility", prepare_for));
match &self.prepare_for_edition {
PrepareFor::Edition(edition) => {
cmd.arg("-W").arg(format!("rust-{}-compatibility", edition));
}
PrepareFor::Next => {
let edition = self.next_edition();
cmd.arg("-W").arg(format!("rust-{}-compatibility", edition));
}
PrepareFor::None => {}
}
}

Expand All @@ -519,8 +545,9 @@ impl FixArgs {
/// then yield an error to the user, indicating that this is happening.
fn verify_not_preparing_for_enabled_edition(&self) -> CargoResult<()> {
let edition = match &self.prepare_for_edition {
Some(s) => s,
None => return Ok(()),
PrepareFor::Edition(s) => s,
PrepareFor::Next => self.next_edition(),
PrepareFor::None => return Ok(()),
};
let enabled = match &self.enabled_edition {
Some(s) => s,
Expand All @@ -544,8 +571,9 @@ impl FixArgs {

fn warn_if_preparing_probably_inert(&self) -> CargoResult<()> {
let edition = match &self.prepare_for_edition {
Some(s) => s,
None => return Ok(()),
PrepareFor::Edition(s) => s,
PrepareFor::Next => self.next_edition(),
PrepareFor::None => return Ok(()),
};
let path = match &self.file {
Some(s) => s,
Expand All @@ -567,4 +595,16 @@ impl FixArgs {

Ok(())
}

fn next_edition(&self) -> &str {
match self.enabled_edition.as_ref().map(|s| &**s) {
// 2015 -> 2018,
None | Some("2015") => "2018",

// This'll probably be wrong in 2020, but that's future Cargo's
// problem. Eventually though we'll just add more editions here as
// necessary.
_ => "2018",
Copy link
Member

Choose a reason for hiding this comment

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

Hmmmm. Is it actually only future cargo's problem? What if I run cargo fix --edition on an 2018-edition project right now? I'd naively assume this method to deal with that. As it stands right now it sometimes returns a string we can't really work with, and instead assumes we call a function like verify_not_preparing_for_enabled_edition.

Maybe it's overkill, but I'd have this return a Result<&str, CantUpgradeEditionError>, with enum CantUpgradeEditionError { UnknownEdition, LatestKnownEdition }.

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 I was wondering this as well, and it's true that the natural interperetation of this flag on the 2018 edition is to avoid doing anything. Currently though we have a good property where if you mix up the transition steps you'll get warnings and errors to nudge you in the right direction. If we take this approach, though, if you enable the edition to soon you won't get a helpful error but rather a deluge of resolution errors (due to crate::).

For that reason I figured it'd be best to stick to a policy of "--edition can't be used when you're already on the latest edition" and that way we should hopefully not surprise too many people and otherwise continue to help nudge towards the new edition along the prescribed path.

}
}
}
37 changes: 30 additions & 7 deletions tests/testsuite/fix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ fn prepare_for_2018() {
[FINISHED] [..]
";
assert_that(
p.cargo("fix --prepare-for 2018 --allow-no-vcs"),
p.cargo("fix --edition --allow-no-vcs"),
execs().with_status(0).with_stderr(stderr).with_stdout(""),
);

Expand Down Expand Up @@ -324,7 +324,7 @@ fn local_paths() {
";

assert_that(
p.cargo("fix --prepare-for 2018 --allow-no-vcs"),
p.cargo("fix --edition --allow-no-vcs"),
execs().with_status(0).with_stderr(stderr).with_stdout(""),
);

Expand Down Expand Up @@ -362,7 +362,7 @@ issues in preparation for the 2018 edition
[FINISHED] [..]
";
assert_that(
p.cargo("fix --prepare-for 2018 --allow-no-vcs"),
p.cargo("fix --edition --allow-no-vcs"),
execs().with_status(0).with_stderr(stderr).with_stdout(""),
);
}
Expand Down Expand Up @@ -452,7 +452,7 @@ fn specify_rustflags() {
[FINISHED] [..]
";
assert_that(
p.cargo("fix --prepare-for 2018 --allow-no-vcs")
p.cargo("fix --edition --allow-no-vcs")
.env("RUSTFLAGS", "-C target-cpu=native"),
execs().with_status(0).with_stderr(stderr).with_stdout(""),
);
Expand Down Expand Up @@ -882,7 +882,6 @@ fn prepare_for_and_enable() {
.build();

let stderr = "\
[CHECKING] foo v0.1.0 ([..])
error: cannot prepare for the 2018 edition when it is enabled, so cargo cannot
automatically fix errors in `src[/]lib.rs`

Expand All @@ -895,7 +894,7 @@ information about transitioning to the 2018 edition see:

";
assert_that(
p.cargo("fix --prepare-for 2018 --allow-no-vcs")
p.cargo("fix --edition --allow-no-vcs")
.masquerade_as_nightly_cargo(),
execs()
.with_stderr_contains(stderr)
Expand All @@ -920,7 +919,7 @@ issues in preparation for the 2018 edition
[FINISHED] [..]
";
assert_that(
p.cargo("fix --prepare-for 2018 --allow-no-vcs")
p.cargo("fix --edition --allow-no-vcs")
.masquerade_as_nightly_cargo(),
execs()
.with_stderr(stderr)
Expand Down Expand Up @@ -966,3 +965,27 @@ fn fix_overlapping() {
println!("{}", contents);
assert!(contents.contains("crate::foo::<crate::A>()"));
}

#[test]
fn both_edition_migrate_flags() {
if !is_nightly() {
return
}
let p = project()
.file("src/lib.rs", "")
.build();

let stderr = "\
error: The argument '--edition' cannot be used with '--prepare-for <prepare-for>'

USAGE:
cargo[..] fix --edition --message-format <FMT>

For more information try --help
";

assert_that(
p.cargo("fix --prepare-for 2018 --edition"),
execs().with_status(1).with_stderr(stderr),
);
}