From fe18c7f5b0439a35c6930b659f4a499304f144b1 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 7 Oct 2025 10:53:23 -0500 Subject: [PATCH 1/3] test(add): Show panic --- .../in/Cargo.toml | 5 +++ .../in/dependency/Cargo.toml | 4 +++ .../in/dependency/src/lib.rs | 0 .../in/primary/Cargo.toml | 4 +++ .../in/primary/src/lib.rs | 0 .../invalid_inherited_dependency/mod.rs | 25 +++++++++++++++ .../out/Cargo.toml | 5 +++ .../out/dependency/Cargo.toml | 4 +++ .../out/primary/Cargo.toml | 4 +++ .../stderr.term.svg | 31 +++++++++++++++++++ tests/testsuite/cargo_add/mod.rs | 1 + 11 files changed, 83 insertions(+) create mode 100644 tests/testsuite/cargo_add/invalid_inherited_dependency/in/Cargo.toml create mode 100644 tests/testsuite/cargo_add/invalid_inherited_dependency/in/dependency/Cargo.toml create mode 100644 tests/testsuite/cargo_add/invalid_inherited_dependency/in/dependency/src/lib.rs create mode 100644 tests/testsuite/cargo_add/invalid_inherited_dependency/in/primary/Cargo.toml create mode 100644 tests/testsuite/cargo_add/invalid_inherited_dependency/in/primary/src/lib.rs create mode 100644 tests/testsuite/cargo_add/invalid_inherited_dependency/mod.rs create mode 100644 tests/testsuite/cargo_add/invalid_inherited_dependency/out/Cargo.toml create mode 100644 tests/testsuite/cargo_add/invalid_inherited_dependency/out/dependency/Cargo.toml create mode 100644 tests/testsuite/cargo_add/invalid_inherited_dependency/out/primary/Cargo.toml create mode 100644 tests/testsuite/cargo_add/invalid_inherited_dependency/stderr.term.svg diff --git a/tests/testsuite/cargo_add/invalid_inherited_dependency/in/Cargo.toml b/tests/testsuite/cargo_add/invalid_inherited_dependency/in/Cargo.toml new file mode 100644 index 00000000000..f8aaaaba98a --- /dev/null +++ b/tests/testsuite/cargo_add/invalid_inherited_dependency/in/Cargo.toml @@ -0,0 +1,5 @@ +[workspace] +members = ["primary", "dependency"] + +[workspace.dependencies] +foo.workspace = true diff --git a/tests/testsuite/cargo_add/invalid_inherited_dependency/in/dependency/Cargo.toml b/tests/testsuite/cargo_add/invalid_inherited_dependency/in/dependency/Cargo.toml new file mode 100644 index 00000000000..0de6f9b15a8 --- /dev/null +++ b/tests/testsuite/cargo_add/invalid_inherited_dependency/in/dependency/Cargo.toml @@ -0,0 +1,4 @@ +[package] +name = "foo" +version = "0.0.0" +edition = "2015" diff --git a/tests/testsuite/cargo_add/invalid_inherited_dependency/in/dependency/src/lib.rs b/tests/testsuite/cargo_add/invalid_inherited_dependency/in/dependency/src/lib.rs new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/testsuite/cargo_add/invalid_inherited_dependency/in/primary/Cargo.toml b/tests/testsuite/cargo_add/invalid_inherited_dependency/in/primary/Cargo.toml new file mode 100644 index 00000000000..fb520246281 --- /dev/null +++ b/tests/testsuite/cargo_add/invalid_inherited_dependency/in/primary/Cargo.toml @@ -0,0 +1,4 @@ +[package] +name = "bar" +version = "0.0.0" +edition = "2015" diff --git a/tests/testsuite/cargo_add/invalid_inherited_dependency/in/primary/src/lib.rs b/tests/testsuite/cargo_add/invalid_inherited_dependency/in/primary/src/lib.rs new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/testsuite/cargo_add/invalid_inherited_dependency/mod.rs b/tests/testsuite/cargo_add/invalid_inherited_dependency/mod.rs new file mode 100644 index 00000000000..5aab4e21f9d --- /dev/null +++ b/tests/testsuite/cargo_add/invalid_inherited_dependency/mod.rs @@ -0,0 +1,25 @@ +use crate::prelude::*; +use cargo_test_support::Project; +use cargo_test_support::compare::assert_ui; +use cargo_test_support::current_dir; +use cargo_test_support::file; +use cargo_test_support::str; + +#[cargo_test] +fn case() { + cargo_test_support::registry::init(); + let project = Project::from_template(current_dir!().join("in")); + let project_root = project.root(); + let cwd = &project_root; + + snapbox::cmd::Command::cargo_ui() + .arg("add") + .args(["foo", "-p", "bar"]) + .current_dir(cwd) + .assert() + .failure() + .stdout_eq(str![""]) + .stderr_eq(file!["stderr.term.svg"]); + + assert_ui().subset_matches(current_dir!().join("out"), &project_root); +} diff --git a/tests/testsuite/cargo_add/invalid_inherited_dependency/out/Cargo.toml b/tests/testsuite/cargo_add/invalid_inherited_dependency/out/Cargo.toml new file mode 100644 index 00000000000..f8aaaaba98a --- /dev/null +++ b/tests/testsuite/cargo_add/invalid_inherited_dependency/out/Cargo.toml @@ -0,0 +1,5 @@ +[workspace] +members = ["primary", "dependency"] + +[workspace.dependencies] +foo.workspace = true diff --git a/tests/testsuite/cargo_add/invalid_inherited_dependency/out/dependency/Cargo.toml b/tests/testsuite/cargo_add/invalid_inherited_dependency/out/dependency/Cargo.toml new file mode 100644 index 00000000000..0de6f9b15a8 --- /dev/null +++ b/tests/testsuite/cargo_add/invalid_inherited_dependency/out/dependency/Cargo.toml @@ -0,0 +1,4 @@ +[package] +name = "foo" +version = "0.0.0" +edition = "2015" diff --git a/tests/testsuite/cargo_add/invalid_inherited_dependency/out/primary/Cargo.toml b/tests/testsuite/cargo_add/invalid_inherited_dependency/out/primary/Cargo.toml new file mode 100644 index 00000000000..fb520246281 --- /dev/null +++ b/tests/testsuite/cargo_add/invalid_inherited_dependency/out/primary/Cargo.toml @@ -0,0 +1,4 @@ +[package] +name = "bar" +version = "0.0.0" +edition = "2015" diff --git a/tests/testsuite/cargo_add/invalid_inherited_dependency/stderr.term.svg b/tests/testsuite/cargo_add/invalid_inherited_dependency/stderr.term.svg new file mode 100644 index 00000000000..ddd2051e40a --- /dev/null +++ b/tests/testsuite/cargo_add/invalid_inherited_dependency/stderr.term.svg @@ -0,0 +1,31 @@ + + + + + + + + + thread 'main' ([..]) panicked at src/cargo/ops/cargo_add/mod.rs:519:21: + + internal error: entered unreachable code: This should have been caught when parsing a workspace root + + note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace + + + + + + diff --git a/tests/testsuite/cargo_add/mod.rs b/tests/testsuite/cargo_add/mod.rs index 11e9f5fd2a4..5a8f814ad11 100644 --- a/tests/testsuite/cargo_add/mod.rs +++ b/tests/testsuite/cargo_add/mod.rs @@ -50,6 +50,7 @@ mod help; mod infer_prerelease; mod invalid_arg; mod invalid_git_name; +mod invalid_inherited_dependency; mod invalid_key_inherit_dependency; mod invalid_key_overwrite_inherit_dependency; mod invalid_key_rename_inherit_dependency; From ac120d9a16699a3360db6ac79178f0798c100ce0 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 7 Oct 2025 14:06:35 -0500 Subject: [PATCH 2/3] fix(add): Be consistent in parse errors between toml/toml_mut --- src/cargo/util/toml_mut/dependency.rs | 142 +++++++++++++------------- 1 file changed, 69 insertions(+), 73 deletions(-) diff --git a/src/cargo/util/toml_mut/dependency.rs b/src/cargo/util/toml_mut/dependency.rs index c28e96f2c68..ca63947097e 100644 --- a/src/cargo/util/toml_mut/dependency.rs +++ b/src/cargo/util/toml_mut/dependency.rs @@ -245,91 +245,87 @@ impl Dependency { (key.to_owned(), None) }; - let source: Source = if let Some(git) = table.get("git") { - let mut src = GitSource::new( - git.as_str() - .ok_or_else(|| invalid_type(key, "git", git.type_name(), "string"))?, - ); - if let Some(value) = table.get("branch") { - src = - src.set_branch(value.as_str().ok_or_else(|| { + let source: Source = + if let Some(git) = table.get("git") { + let mut src = GitSource::new( + git.as_str() + .ok_or_else(|| invalid_type(key, "git", git.type_name(), "string"))?, + ); + if let Some(value) = table.get("branch") { + src = src.set_branch(value.as_str().ok_or_else(|| { invalid_type(key, "branch", value.type_name(), "string") })?); - } - if let Some(value) = table.get("tag") { - src = - src.set_tag(value.as_str().ok_or_else(|| { + } + if let Some(value) = table.get("tag") { + src = src.set_tag(value.as_str().ok_or_else(|| { invalid_type(key, "tag", value.type_name(), "string") })?); - } - if let Some(value) = table.get("rev") { - src = - src.set_rev(value.as_str().ok_or_else(|| { + } + if let Some(value) = table.get("rev") { + src = src.set_rev(value.as_str().ok_or_else(|| { invalid_type(key, "rev", value.type_name(), "string") })?); - } - if let Some(value) = table.get("version") { - src = src.set_version(value.as_str().ok_or_else(|| { - invalid_type(key, "version", value.type_name(), "string") - })?); - } - src.into() - } else if let Some(path) = table.get("path") { - let base = table - .get("base") - .map(|base| { - base.as_str() - .ok_or_else(|| invalid_type(key, "base", base.type_name(), "string")) - .map(|s| s.to_owned()) - }) - .transpose()?; - let relative_to = if let Some(base) = &base { - Cow::Owned(lookup_path_base( - &PathBaseName::new(base.clone())?, - gctx, - &|| Ok(workspace_root), - unstable_features, - )?) - } else { - Cow::Borrowed(crate_root) - }; - let path = - relative_to + } + if let Some(value) = table.get("version") { + src = src.set_version(value.as_str().ok_or_else(|| { + invalid_type(key, "version", value.type_name(), "string") + })?); + } + src.into() + } else if let Some(path) = table.get("path") { + let base = table + .get("base") + .map(|base| { + base.as_str() + .ok_or_else(|| { + invalid_type(key, "base", base.type_name(), "string") + }) + .map(|s| s.to_owned()) + }) + .transpose()?; + let relative_to = if let Some(base) = &base { + Cow::Owned(lookup_path_base( + &PathBaseName::new(base.clone())?, + gctx, + &|| Ok(workspace_root), + unstable_features, + )?) + } else { + Cow::Borrowed(crate_root) + }; + let path = relative_to .join(path.as_str().ok_or_else(|| { invalid_type(key, "path", path.type_name(), "string") })?); - let mut src = PathSource::new(path); - src.base = base; - if let Some(value) = table.get("version") { - src = src.set_version(value.as_str().ok_or_else(|| { - invalid_type(key, "version", value.type_name(), "string") - })?); - } - src.into() - } else if let Some(version) = table.get("version") { - let src = - RegistrySource::new(version.as_str().ok_or_else(|| { + let mut src = PathSource::new(path); + src.base = base; + if let Some(value) = table.get("version") { + src = src.set_version(value.as_str().ok_or_else(|| { + invalid_type(key, "version", value.type_name(), "string") + })?); + } + src.into() + } else if let Some(version) = table.get("version") { + let src = RegistrySource::new(version.as_str().ok_or_else(|| { invalid_type(key, "version", version.type_name(), "string") })?); - src.into() - } else if let Some(workspace) = table.get("workspace") { - let workspace_bool = workspace - .as_bool() - .ok_or_else(|| invalid_type(key, "workspace", workspace.type_name(), "bool"))?; - if !workspace_bool { - anyhow::bail!("`{key}.workspace = false` is unsupported") - } - let src = WorkspaceSource::new(); - src.into() - } else { - let mut msg = format!("unrecognized dependency source for `{key}`"); - if table.is_empty() { - msg.push_str( - ", expected a local path, Git repository, version, or workspace dependency to be specified", + src.into() + } else if let Some(workspace) = table.get("workspace") { + let workspace_bool = workspace.as_bool().ok_or_else(|| { + invalid_type(key, "workspace", workspace.type_name(), "bool") + })?; + if !workspace_bool { + anyhow::bail!("`{key}.workspace = false` is unsupported") + } + let src = WorkspaceSource::new(); + src.into() + } else { + anyhow::bail!( + "dependency ({key}) specified without \ + providing a local path, Git repository, version, or \ + workspace dependency to use" ); - } - anyhow::bail!(msg); - }; + }; let registry = if let Some(value) = table.get("registry") { Some( value From e512c7be78fa29ba809d0ab6dfb6869340c5fb84 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 7 Oct 2025 14:42:17 -0500 Subject: [PATCH 3/3] fix(add): Report a missing source error for workspace dependencies When you have ```toml [workspace.dependencies] apply.workspace = true ``` You have a source-less workspace dependency with an unused `workspace` field. In #13775, we removed support for source-less dependencies but missed this case. This mirrors the error message from `toml/mod.rs` that #13775 added. Other options I considered: - Making the presence of `workspace` an error in `cargo-util-schemas` - Validating `workspace.dependencies` in `toml/mod.rs`, even when its not inherited In both cases, they are behavior changes that and I wanted to keep the focus of this change small rather than get tied up in what behavior changes are safe or should be done with an Edition. The second option could still lead to this panic if we move some of that error reporting into later stages of Cargo. Fixes #1398 --- src/cargo/ops/cargo_add/mod.rs | 6 +++++- .../invalid_inherited_dependency/stderr.term.svg | 14 +++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/cargo/ops/cargo_add/mod.rs b/src/cargo/ops/cargo_add/mod.rs index ed8506d0915..251d4c539a1 100644 --- a/src/cargo/ops/cargo_add/mod.rs +++ b/src/cargo/ops/cargo_add/mod.rs @@ -516,7 +516,11 @@ fn resolve_dependency( let query = dep.query(gctx)?; match query { MaybeWorkspace::Workspace(_) => { - unreachable!("This should have been caught when parsing a workspace root") + anyhow::bail!( + "dependency ({}) specified without \ + providing a local path, Git repository, or version", + dependency.toml_key() + ); } MaybeWorkspace::Other(query) => query, } diff --git a/tests/testsuite/cargo_add/invalid_inherited_dependency/stderr.term.svg b/tests/testsuite/cargo_add/invalid_inherited_dependency/stderr.term.svg index ddd2051e40a..59207de00a8 100644 --- a/tests/testsuite/cargo_add/invalid_inherited_dependency/stderr.term.svg +++ b/tests/testsuite/cargo_add/invalid_inherited_dependency/stderr.term.svg @@ -1,11 +1,13 @@ - +