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.

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

cargo fix doesn’t fix path dependencies #13022

Closed
SimonSapin opened this issue Nov 1, 2018 · 11 comments
Closed

cargo fix doesn’t fix path dependencies #13022

SimonSapin opened this issue Nov 1, 2018 · 11 comments
Labels
Command-fix S-propose-close Status: A team member has nominated this for closing, pending further input from the team

Comments

@SimonSapin
Copy link
Contributor

In an empty directory, run:

cargo new --lib a
cargo new --lib b
cargo new --lib a/c
echo 'b = {path = "../b"}' >> a/Cargo.toml
echo 'c = {path = "./c"}' >> a/Cargo.toml
cat > a/src/lib.rs <<END
pub fn foo() {}

pub mod bar {
    use foo;
    pub fn baz() { foo() }
}
END
cp a/src/lib.rs b/src/lib.rs
cp a/src/lib.rs a/c/src/lib.rs
(cd a && cargo +nightly fix --edition -j1)
find -name lib.rs | xargs grep use
rm -r a b

This creates three crates with identical code, with one depending on the other two through path Cargo dependencies. cargo fix is able to silently fix the "root" crate, for the other two it only prints warnings.

Output with nightly-2018-11-01 which contains cargo 1.31.0-nightly (2d0863f65 2018-10-20) which depends on rustfix = "0.4.2":

     Created library `a` project
     Created library `b` project
     Created library `a/c` project
    Checking b v0.1.0 (/tmp/foo/b)                                                                                     
warning: absolute paths must start with `self`, `super`, `crate`, or an external crate name in the 2018 edition        
 --> /tmp/foo/b/src/lib.rs:4:9                                                                                         
  |                                                                                                                    
4 |     use foo;                                                                                                       
  |         ^^^ help: use `crate`: `crate::foo`                                                                        
  |                                                                                                                    
  = note: `-W absolute-paths-not-starting-with-crate` implied by `-W rust-2018-compatibility`                          
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in the 2018 edition!
  = note: for more information, see issue #53130 <https://github.com/rust-lang/rust/issues/53130>                      
                                                                                                                       
    Checking c v0.1.0 (/tmp/foo/a/c)                                                                                   
warning: absolute paths must start with `self`, `super`, `crate`, or an external crate name in the 2018 edition        
 --> c/src/lib.rs:4:9                                                                                                  
  |                                                                                                                    
4 |     use foo;                                                                                                       
  |         ^^^ help: use `crate`: `crate::foo`                                                                        
  |                                                                                                                    
  = note: `-W absolute-paths-not-starting-with-crate` implied by `-W rust-2018-compatibility`                          
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in the 2018 edition!
  = note: for more information, see issue #53130 <https://github.com/rust-lang/rust/issues/53130>                      
                                                                                                                       
    Checking a v0.1.0 (/tmp/foo/a)                                                                                     
      Fixing src/lib.rs (1 fix)                                                                                        
    Finished dev [unoptimized + debuginfo] target(s) in 0.44s                                                          
./b/src/lib.rs:    use foo;
./a/c/src/lib.rs:    use foo;
./a/src/lib.rs:    use crate::foo;
@Nemo157
Copy link
Member

Nemo157 commented Nov 1, 2018

Alternatively

cargo +nightly new --edition 2015 --lib a
cargo +nightly new --edition 2015 --lib b
cargo +nightly new --edition 2015 --lib a/c
cat >> a/Cargo.toml <<END
b = {path = "../b"}
c = {path = "./c"}
[workspace]
members = [".", "./c"]
END
cat > a/src/lib.rs <<END
pub fn foo() {}

pub mod bar {
    use foo;
    pub fn baz() { foo() }
}
END
cp a/src/lib.rs b/src/lib.rs
cp a/src/lib.rs a/c/src/lib.rs
(cd a && cargo +nightly fix --edition -j1 --all)
find -name lib.rs | xargs grep use
rm -r a b

Fixes both a and a/c, it seems to me like the bug is that it's showing the edition warnings for dependencies that it's not migrating. I wouldn't expect it to migrate path dependencies outside the current workspace automatically.

@steveklabnik
Copy link
Member

This should probably be filed against Cargo, given that's where cargo fix lives these days.

@alexcrichton
Copy link
Member

Yes this is largely a Cargo issue rather than a rustfix one, and agreed that warnings should only be presented for crates which are actually considered candidates for fixing!

@alexcrichton
Copy link
Member

This should be fixed in #6247

@SimonSapin
Copy link
Contributor Author

Silencing migration warnings for crates that are not being fixed is a nice improvement, but it doesn’t fix the issue that path dependencies are not fixed.

@alexcrichton
Copy link
Member

You can migrate multiple crates manually with -p flags I believe, but otherwise crates aren't migrated wholesale and just one at a time

@SimonSapin
Copy link
Contributor Author

Yes they aren’t, and I think that’s a bug.

bors referenced this issue Nov 1, 2018
Don't turn on edition lints for unfixed crates

Currently Cargo runs the risk of turning on the edition lints for crates
which `cargo fix` isn't actually fixing, which means you'll get a huge
deluge of lints that would otherwise be automatically fixable! Fix this
situation by only enabling lints in the same cases that we're actually
applying fixes.

Closes rust-lang-nursery/rustfix#150
alexcrichton referenced this issue in alexcrichton/cargo Nov 2, 2018
Currently Cargo runs the risk of turning on the edition lints for crates
which `cargo fix` isn't actually fixing, which means you'll get a huge
deluge of lints that would otherwise be automatically fixable! Fix this
situation by only enabling lints in the same cases that we're actually
applying fixes.

Closes rust-lang-nursery/rustfix#150
@killercup
Copy link
Member

@steveklabnik did you mean to close that from your fork of cargo?

@steveklabnik steveklabnik reopened this Mar 25, 2019
@steveklabnik
Copy link
Member

uh, no. Sorry about that. All I did was the usual "update my fork of cargo and push master". It shouldn't be closing random things...

@ehuss ehuss transferred this issue from rust-lang/rustfix Nov 22, 2023
@ehuss ehuss added Command-fix S-propose-close Status: A team member has nominated this for closing, pending further input from the team labels Nov 22, 2023
@ehuss
Copy link
Contributor

ehuss commented Nov 22, 2023

I'm uncertain, but I think I'm going to propose to close this. I think there are some complications with implicitly affecting path dependencies (like with patches and such), and also that would deviate quite a bit from how other cargo commands work. I'm not sure if it is really a good idea to implicitly affects path dependencies. Ideally in that scenario, using a workspace would be better.

@weihanglo
Copy link
Member

using a workspace would be better.

Agree on this approach. Path dependencies can also be shared across workspaces and not belong to any of them. Fixing them sounds risky and not always intented.

Closing as proposed.

@weihanglo weihanglo closed this as not planned Won't fix, can't repro, duplicate, stale Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Command-fix S-propose-close Status: A team member has nominated this for closing, pending further input from the team
Projects
None yet
Development

No branches or pull requests

7 participants