Improve error handling when PathSource is relative

[Morganamilo]

When editing dependencies with cargo, if a relative PathSource is supplied cargo panics with "both paths are absolute". This is the opposite of what's actually wrong leading to confusion.

Instead, use the same error formatting we use in other diff_paths calls and return that error instead of panicking.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[weihanglo]

Thanks for the pull request. This change looks reasonable, although it is not immediately clear to when this would happen. Could you provide a reproducible example of it?

[Morganamilo]

Yeah here's the code I was using to test this. It's a newly initialised cargo project with cargo and anyhow as a dependencies. (Also attached as tarball if useful cargo-path-source-error.tar.gz)

use std::{env::current_dir, path::Path};

use cargo::{
    core::{Features, Workspace},
    util::toml_mut::{
        dependency::{PathSource, Source},
        manifest::LocalManifest,
    },
    GlobalContext,
};

fn main() -> anyhow::Result<()> {
    let cd = current_dir()?;
    let gctx = GlobalContext::default().unwrap();
    let feat = Features::default();

    // Assume we're running via `cargo run` and current_dir is a cargo project just for the sake of
    // example
    let ws = Workspace::new(&cd.join("Cargo.toml"), &gctx)?;
    let mut m = LocalManifest::try_new(&cd.join("Cargo.toml"))?;

    let d = m
        .get_dependency_versions("cargo", &ws, &feat)
        .collect::<Vec<_>>();

    for (_, dep) in d {
        let dep = dep?;

        // Errors because "../cargo is relative".
        // Path should be absolute, cargo then does its own
        // processing to make it relative to the Workspace.
        // Error doesn't actually happen until call to insert_into_table().
        let s = Source::Path(PathSource::new(Path::new("../cargo")));

        let dep = dep.clone().set_source(s);
        m.insert_into_table(&["dependencies".to_string()], &dep, &gctx, &cd, &feat)?;
    }
    Ok(())
}

Against cargo master

morganamilo@songbird ~git/foo % c r -q             
thread 'main' panicked at /Users/morganamilo/git/cargo/src/cargo/util/toml_mut/dependency.rs:751:67:
both paths are absolute
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Against this PR

morganamilo@songbird ~git/foo % c r -q
Error: path comparison requires two absolute paths; path_source: `../cargo`, workspace_path: `/Users/morganamilo/git/foo`

I still don't think the error message is the best but I thought it would be best to follow the error messages used elsewhere to keep the PR simple.

[weihanglo]

Thanks for the reproduction! Actually, I was expecting a bug in cargo the executable, but it turns out from the use of cargo as a dependency. Just remind that depending on cargo as a library has drawbacks, and it serves mainly for the cargo official executable.

If it were a bug in cargo binary itself, we should better provide an actionable message and tell people how to fix it. In this case, the PathSource::path is expected to be an absolute path. An expect call is a good balance between code complexity and traceable message for maintainers, hence no extra context here.

Instead of adding extra context, I slightly lean towards caller passing absolute path to PathSource::new, as there is no action users could take. Granted, the doc comment of PathSource::new didn't mention this requirement.

[weihanglo]

To be honest, some other usages of diff_paths may not need extra anyhow contexts either IMO.

[Morganamilo]

Right, if that's the philosophy of libcargo I'll change it back to except and just fix the error message to not be the opposite.

[weihanglo]

Thanks! Merging.