Avoid panic when using `partition_packages` on Rust < 1.71

[clechasseur]

#59 added support for using the workspace default members when no parameter is specified. However, this only works on Rust 1.71+, so using clap-cargo on Rust < 1.71 results in a panic when calling partition_packages.

I suggest a simple change: avoid using workspace_default_members unless the flags actually do ask for default packages. This way, it would for example be possible to call a clap-cargo-powered tool with --workspace on Rust < 1.71 and it would work.

(A better change might be to check if workspace_default_members are available before using them, but alas this doesn't seem to be available in cargo_metadata currently.)

If this change is acceptable, I volunteer to work on a PR for this.

[epage]

Could you clarify what panic you are seeing?

I can see us cleaning up the code to only calculate things we need but I don't see how that would be causing a panic.

[clechasseur]

Minimal repro (available here):

Cargo.toml

[package]
name = "cargo-clap-cargo-test"
version = "0.1.0"
edition = "2021"

[dependencies]
cargo_metadata = "=0.18.1"
clap = { version = "=4.5.2", features = ["cargo", "derive"] }
clap-cargo = { version = "=0.14.0", features = ["clap", "cargo_metadata"] }

src/main.rs

use clap::{Args, Parser};
use clap_cargo::{Manifest, Workspace};

#[derive(Debug, Parser)]
#[command(name = "cargo", bin_name = "cargo")]
enum Cli {
    ClapCargoTest(ClapCargoTestArgs),
}

#[derive(Debug, Args)]
struct ClapCargoTestArgs {
    #[command(flatten)]
    manifest: Manifest,
    #[command(flatten)]
    workspace: Workspace,
}

fn main() {
    let Cli::ClapCargoTest(args) = Cli::parse();

    let metadata = args.manifest.metadata().exec().unwrap();
    let (selected, unselected) = args.workspace.partition_packages(&metadata);

    println!(
        "Selected packages: {:?}\nUnselected packages: {:?}",
        selected.iter().map(|&p| &p.name).collect::<Vec<_>>(),
        unselected.iter().map(|&p| &p.name).collect::<Vec<_>>(),
    );
}

Result (some parts snipped for brevity):

PS ...\clap-cargo-test> cargo install --path .
  Installing cargo-clap-cargo-test v0.1.0 (...)
  ...
   Installed package `cargo-clap-cargo-test v0.1.0 (...)

PS ...\clap-cargo-test> cargo +1.71.0 clap-cargo-test
Selected packages: ["cargo-clap-cargo-test"]
Unselected packages: ["anstream", "anstyle", ...]

PS ...\clap-cargo-test> cargo +1.70.0 clap-cargo-test
thread 'main' panicked at ...\.cargo\registry\src\index.crates.io-6f17d22bba15001f\cargo_metadata-0.18.1\src\lib.rs:246:14:
WorkspaceDefaultMembers should only be dereferenced on Cargo versions >= 1.71
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The panic is in the cargo_metadata crate here: https://github.com/oli-obk/cargo_metadata/blob/0.18.1/src/lib.rs#L246

pub struct WorkspaceDefaultMembers(Option<Vec<PackageId>>);

impl core::ops::Deref for WorkspaceDefaultMembers {
    type Target = [PackageId];

    fn deref(&self) -> &Self::Target {
        self.0
            .as_ref()
            .expect("WorkspaceDefaultMembers should only be dereferenced on Cargo versions >= 1.71")
            // ^^ THIS RIGHT HERE ^^
    }
}

It is caused by defererencing the workspace_default_members here: https://github.com/crate-ci/clap-cargo/blob/v0.14.0/src/workspace.rs#L42-L43

pub fn partition_packages<'m>(
        &self,
        meta: &'m cargo_metadata::Metadata,
    ) -> (
        Vec<&'m cargo_metadata::Package>,
        Vec<&'m cargo_metadata::Package>,
    ) {
        let selection =
            Packages::from_flags(self.workspace || self.all, &self.exclude, &self.package);
        let workspace_members: std::collections::HashSet<_> =
            meta.workspace_members.iter().collect();
        let workspace_default_members: std::collections::HashSet<_> =
            meta.workspace_default_members.iter().collect();
                                       // ^ DEFERENCING HAPPENS HERE

To avoid it, my suggestion would be to move the dereferencing of workspace_default_members to the Packages::Default match arm. This way, it would still panic when used with default flags, but specifying a custom flag like --workspace would allow proper use on Rust < 1.71.

[epage]

If we delay accessing that field, we will just make it so we sometimes panic but o always. That would make it harder to detect this problem.

It looks like cargo-metadata does not provide a sufficient API for us to detect the user is running an older cargo.

Choices for moving forward

• Do nothing. I generally recommend people run the latest version of rustc for development and worry about their deployment / MSRV version for production builds / CI.
• Revert #59 until some indeterminate period of time. We are already 5 versions away. Its hard to say what a sufficient window further away would be
• See if cargo-metadata would be willing to add support for detecting the presence of that field.

[clechasseur]

I guess there's another choice: determine the running Cargo's version to see if it's safe to access the field. However, that doesn't seem to be an ideal solution. (That could be implemented in the tool using clap-cargo too.)

FWIW, I've also volunteered to add something to detect this in cargo_metadata: oli-obk/cargo_metadata#254

Do nothing. I generally recommend people run the latest version of rustc for development and worry about their deployment / MSRV version for production builds / CI.

I indeed detected the issue in a CI workflow that validates a crate's MSRV.