cargo-fmt: remove command line validation which did not properly handle the windows environment

[scampi]

@jakoschiko @chris-morgan Could you give this branch a try ?

Maybe the problem in windows comes from our handling of env::args(), where the first item is skipped. That is usually the executable name, but not always:

The first element is traditionally the path of the executable, but it can be set to arbitrary text, and may not even exist. This means this property should not be relied upon for security purposes.

Close #2694

[chris-morgan]

Haven’t tested it yet, but have skimmed the diff, and I’m >98% sure this won’t fix the problem, as it looks to only affect which arguments are slurped, which is not where the problem is (e.g. “package `member` is not a member of the workspace” indicates that it successfully slurped the package name, but that its workspace member list was awry).

[scampi]

@chris-morgan on windows I suspect it's because we call skip(1), I've opened #3310 to track this.
That fix should avoid getting the Invalid argument: member. error when passing --package member or -p member.

[scampi]

Actually could you try removing skip(1) and see what happens on your machine ?

[jakoschiko]

I'm using the setup from #2694 on Windows 10.

On branch scampi:issue2694:

$ echo $PWD
/d/code/rust/testfmt/member
$ cargo-fmt --version
rustfmt 1.0.1-nightly (2a76d955 2019-02-01)
$ cargo-fmt
Failed to find targets
usage: cargo fmt [options]

On branch scampi:issue2694 and with skip(1) removed:

$ cargo-fmt
Invalid argument: `C:\Users\anonymous\.cargo\bin\cargo-fmt.exe`.
usage: cargo fmt [options]

[jakoschiko]

I added some dbgs to get_targets_root_only:

fn get_targets_root_only(targets: &mut HashSet<Target>) -> Result<(), io::Error> {
    let metadata = get_cargo_metadata(None)?;
    let current_dir = dbg!(dbg!(env::current_dir()?).canonicalize()?);
    let current_dir_manifest = dbg!(current_dir.join("Cargo.toml"));
    let workspace_root_path = dbg!(PathBuf::from(&metadata.workspace_root).canonicalize()?);
    let in_workspace_root = dbg!(workspace_root_path == current_dir);

    for package in metadata.packages {
        if in_workspace_root || dbg!(dbg!(PathBuf::from(&package.manifest_path)) == current_dir_manifest) {
            for target in package.targets {
                targets.insert(Target::from_target(&target));
            }
        }
    }

    Ok(())
}

Here the result:

$ cargo-fmt
[src/cargo-fmt/main.rs:239] env::current_dir()? = "D:\\Code\\rust\\testfmt\\member"
[src/cargo-fmt/main.rs:239] dbg!(env :: current_dir (  ) ?).canonicalize()? = "\\\\?\\D:\\Code\\rust\\testfmt\\member"
[src/cargo-fmt/main.rs:240] current_dir.join("Cargo.toml") = "\\\\?\\D:\\Code\\rust\\testfmt\\member\\Cargo.toml"
[src/cargo-fmt/main.rs:241] PathBuf::from(&metadata.workspace_root).canonicalize()? = "\\\\?\\D:\\Code\\rust\\testfmt"
[src/cargo-fmt/main.rs:242] workspace_root_path == current_dir = false
[src/cargo-fmt/main.rs:245] PathBuf::from(&package.manifest_path) = "D:\\Code\\rust\\testfmt\\member\\Cargo.toml"
[src/cargo-fmt/main.rs:245] dbg!(PathBuf :: from ( & package . manifest_path )) == current_dir_manifest = false
Failed to find targets
usage: cargo fmt [options]

The result of canonicalize looks strange.

Edit: Changing PathBuf::from(&package.manifest_path) to PathBuf::from(&package.manifest_path).canonicalize()? fixes the problem.

[scampi]

Thanks @jakoschiko for the help.

You're right, canonicalize on Windows returns an extended-length path which adds the leading "\\?\" we see in your debug. I'll apply your fix for this.

Regarding skip(1) it would be kinda OK if the following were removed; I was just trying to figure out what could explain the various errors reported in the issue, and anyway that skipping will be further worked on in a separate PR (issue #3310).

rustfmt/src/cargo-fmt/main.rs

Lines 55 to 66 in 2a76d95

	// If there is any invalid argument passed to `cargo fmt`, return without formatting.
	let mut is_package_arg = false;
	for arg in env::args().skip(1).take_while(|a| a != "--") {
	if arg.starts_with('-') {
	is_package_arg = arg.starts_with("--package") | arg.starts_with("-p");
	} else if !is_package_arg {
	print_usage_to_stderr(&opts, &format!("Invalid argument: `{}`.", arg));
	return FAILURE;
	} else {
	is_package_arg = false;
	}
	}

[scampi]

The following error was also reported

C:\workspace\member>cargo fmt --package member
package `member` is not a member of the workspace
[usage]

@jakoschiko Could you also provide a debug of the following line ? thanks

rustfmt/src/cargo-fmt/main.rs

Line 296 in 2a76d95

if workspace_hitlist.remove(&package.name) {

[jakoschiko]

Using stable:

$ echo $PWD
/d/code/rust/testfmt
$ cargo fmt --version
rustfmt 1.0.0-stable (43206f41 2018-11-30)
$ cargo fmt --package member
$ cargo fmt -p member
Invalid argument: `member`.
usage: cargo fmt [options]
...
$ cargo fmt -pmember
$ cd member
$ cargo fmt --package member
$ cargo fmt -p member
Invalid argument: `member`.
usage: cargo fmt [options]
...
$ cargo fmt -pmember

Using your current branch with the dbgs:

$ echo $PWD
/d/code/rust/testfmt
$ cargo fmt --version
Invalid argument: `fmt`.
usage: cargo fmt [options]
...
$ cargo-fmt --version
rustfmt 1.0.1-nightly (06d8feb8 2019-02-02)
$ cargo-fmt --package member
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cargo-fmt -p member
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cargo-fmt -pmember
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cd member
$ cargo-fmt --package member
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cargo-fmt -p member
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cargo-fmt -pmember
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true

-p member seems to work. But because of skip(1) I can't use rustfmt as cargo subcommand. With skip(2):

$ echo $PWD
/d/code/rust/testfmt
$ cargo fmt --version
rustfmt 1.0.1-nightly (06d8feb8 2019-02-02)
$ cargo fmt --package member
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cargo fmt -p member
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cargo fmt -pmember
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cd member
$ cargo fmt --package member
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cargo fmt -p member
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cargo fmt -pmember
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true

[scampi]

@jakoschiko I reverted the change to skip, as I didn't test this properly before proposing the PR.
I've checked the branch as of ec8ec05 and it seems to work fine to me.

I tried the changes locally and used the following debugs in the gif below. I didn't manage to reproduce @chris-morgan 's error:

C:\workspace\member>cargo fmt --package member
package `member` is not a member of the workspace
[usage]

I think this PR is fine now.

---

@@ -54,7 +54,7 @@ fn execute() -> i32 {
 
     // If there is any invalid argument passed to `cargo fmt`, return without formatting.
     let mut is_package_arg = false;
-    for arg in env::args().skip(1).take_while(|a| a != "--") {
+    for arg in dbg!(env::args()).skip(2).take_while(|a| a != "--") {
         if arg.starts_with('-') {
             is_package_arg = arg.starts_with("--package") | arg.starts_with("-p");
         } else if !is_package_arg {
@@ -224,7 +224,7 @@ fn get_targets(strategy: &CargoFmtStrategy) -> Result<HashSet<Target>, io::Error
         CargoFmtStrategy::Some(ref hitlist) => get_targets_with_hitlist(hitlist, &mut targets)?,
     }
 
-    if targets.is_empty() {
+    if dbg!(&targets).is_empty() {
         Err(io::Error::new(
             io::ErrorKind::Other,
             "Failed to find targets".to_owned(),
@@ -292,10 +292,10 @@ fn get_targets_with_hitlist(
 ) -> Result<(), io::Error> {
     let metadata = get_cargo_metadata(None)?;
 
-    let mut workspace_hitlist: HashSet<&String> = HashSet::from_iter(hitlist);
+    let mut workspace_hitlist: HashSet<&String> = HashSet::from_iter(dbg!(hitlist));
 
     for package in metadata.packages {
-        if workspace_hitlist.remove(&package.name) {
+        if workspace_hitlist.remove(dbg!(&package.name)) {
             for target in package.targets {
                 targets.insert(Target::from_target(&target));
             }

[topecongiro]

I am closing this in favor of #3652. @scampi Thank you for your work, and sorry that the review got delayed. For better or worse, I now have a Windows machine at my hand, so testing this kind of error should be more straightforward.