Skip to content

Commit 6a160a2

Browse files
authored
fix(add): Report a missing source error for workspace dependencies (#16063)
### What does this PR try to resolve? 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 ### How to test and review this PR?
2 parents 5629df7 + e512c7b commit 6a160a2

File tree

13 files changed

+153
-74
lines changed

13 files changed

+153
-74
lines changed

src/cargo/ops/cargo_add/mod.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -647,7 +647,11 @@ fn query_dependency(
647647
let query = dep.query(gctx)?;
648648
match query {
649649
MaybeWorkspace::Workspace(_) => {
650-
unreachable!("This should have been caught when parsing a workspace root")
650+
anyhow::bail!(
651+
"dependency ({}) specified without \
652+
providing a local path, Git repository, or version",
653+
dependency.toml_key()
654+
);
651655
}
652656
MaybeWorkspace::Other(query) => query,
653657
}

src/cargo/util/toml_mut/dependency.rs

Lines changed: 69 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -245,91 +245,87 @@ impl Dependency {
245245
(key.to_owned(), None)
246246
};
247247

248-
let source: Source = if let Some(git) = table.get("git") {
249-
let mut src = GitSource::new(
250-
git.as_str()
251-
.ok_or_else(|| invalid_type(key, "git", git.type_name(), "string"))?,
252-
);
253-
if let Some(value) = table.get("branch") {
254-
src =
255-
src.set_branch(value.as_str().ok_or_else(|| {
248+
let source: Source =
249+
if let Some(git) = table.get("git") {
250+
let mut src = GitSource::new(
251+
git.as_str()
252+
.ok_or_else(|| invalid_type(key, "git", git.type_name(), "string"))?,
253+
);
254+
if let Some(value) = table.get("branch") {
255+
src = src.set_branch(value.as_str().ok_or_else(|| {
256256
invalid_type(key, "branch", value.type_name(), "string")
257257
})?);
258-
}
259-
if let Some(value) = table.get("tag") {
260-
src =
261-
src.set_tag(value.as_str().ok_or_else(|| {
258+
}
259+
if let Some(value) = table.get("tag") {
260+
src = src.set_tag(value.as_str().ok_or_else(|| {
262261
invalid_type(key, "tag", value.type_name(), "string")
263262
})?);
264-
}
265-
if let Some(value) = table.get("rev") {
266-
src =
267-
src.set_rev(value.as_str().ok_or_else(|| {
263+
}
264+
if let Some(value) = table.get("rev") {
265+
src = src.set_rev(value.as_str().ok_or_else(|| {
268266
invalid_type(key, "rev", value.type_name(), "string")
269267
})?);
270-
}
271-
if let Some(value) = table.get("version") {
272-
src = src.set_version(value.as_str().ok_or_else(|| {
273-
invalid_type(key, "version", value.type_name(), "string")
274-
})?);
275-
}
276-
src.into()
277-
} else if let Some(path) = table.get("path") {
278-
let base = table
279-
.get("base")
280-
.map(|base| {
281-
base.as_str()
282-
.ok_or_else(|| invalid_type(key, "base", base.type_name(), "string"))
283-
.map(|s| s.to_owned())
284-
})
285-
.transpose()?;
286-
let relative_to = if let Some(base) = &base {
287-
Cow::Owned(lookup_path_base(
288-
&PathBaseName::new(base.clone())?,
289-
gctx,
290-
&|| Ok(workspace_root),
291-
unstable_features,
292-
)?)
293-
} else {
294-
Cow::Borrowed(crate_root)
295-
};
296-
let path =
297-
relative_to
268+
}
269+
if let Some(value) = table.get("version") {
270+
src = src.set_version(value.as_str().ok_or_else(|| {
271+
invalid_type(key, "version", value.type_name(), "string")
272+
})?);
273+
}
274+
src.into()
275+
} else if let Some(path) = table.get("path") {
276+
let base = table
277+
.get("base")
278+
.map(|base| {
279+
base.as_str()
280+
.ok_or_else(|| {
281+
invalid_type(key, "base", base.type_name(), "string")
282+
})
283+
.map(|s| s.to_owned())
284+
})
285+
.transpose()?;
286+
let relative_to = if let Some(base) = &base {
287+
Cow::Owned(lookup_path_base(
288+
&PathBaseName::new(base.clone())?,
289+
gctx,
290+
&|| Ok(workspace_root),
291+
unstable_features,
292+
)?)
293+
} else {
294+
Cow::Borrowed(crate_root)
295+
};
296+
let path = relative_to
298297
.join(path.as_str().ok_or_else(|| {
299298
invalid_type(key, "path", path.type_name(), "string")
300299
})?);
301-
let mut src = PathSource::new(path);
302-
src.base = base;
303-
if let Some(value) = table.get("version") {
304-
src = src.set_version(value.as_str().ok_or_else(|| {
305-
invalid_type(key, "version", value.type_name(), "string")
306-
})?);
307-
}
308-
src.into()
309-
} else if let Some(version) = table.get("version") {
310-
let src =
311-
RegistrySource::new(version.as_str().ok_or_else(|| {
300+
let mut src = PathSource::new(path);
301+
src.base = base;
302+
if let Some(value) = table.get("version") {
303+
src = src.set_version(value.as_str().ok_or_else(|| {
304+
invalid_type(key, "version", value.type_name(), "string")
305+
})?);
306+
}
307+
src.into()
308+
} else if let Some(version) = table.get("version") {
309+
let src = RegistrySource::new(version.as_str().ok_or_else(|| {
312310
invalid_type(key, "version", version.type_name(), "string")
313311
})?);
314-
src.into()
315-
} else if let Some(workspace) = table.get("workspace") {
316-
let workspace_bool = workspace
317-
.as_bool()
318-
.ok_or_else(|| invalid_type(key, "workspace", workspace.type_name(), "bool"))?;
319-
if !workspace_bool {
320-
anyhow::bail!("`{key}.workspace = false` is unsupported")
321-
}
322-
let src = WorkspaceSource::new();
323-
src.into()
324-
} else {
325-
let mut msg = format!("unrecognized dependency source for `{key}`");
326-
if table.is_empty() {
327-
msg.push_str(
328-
", expected a local path, Git repository, version, or workspace dependency to be specified",
312+
src.into()
313+
} else if let Some(workspace) = table.get("workspace") {
314+
let workspace_bool = workspace.as_bool().ok_or_else(|| {
315+
invalid_type(key, "workspace", workspace.type_name(), "bool")
316+
})?;
317+
if !workspace_bool {
318+
anyhow::bail!("`{key}.workspace = false` is unsupported")
319+
}
320+
let src = WorkspaceSource::new();
321+
src.into()
322+
} else {
323+
anyhow::bail!(
324+
"dependency ({key}) specified without \
325+
providing a local path, Git repository, version, or \
326+
workspace dependency to use"
329327
);
330-
}
331-
anyhow::bail!(msg);
332-
};
328+
};
333329
let registry = if let Some(value) = table.get("registry") {
334330
Some(
335331
value
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
[workspace]
2+
members = ["primary", "dependency"]
3+
4+
[workspace.dependencies]
5+
foo.workspace = true
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
[package]
2+
name = "foo"
3+
version = "0.0.0"
4+
edition = "2015"

tests/testsuite/cargo_add/invalid_inherited_dependency/in/dependency/src/lib.rs

Whitespace-only changes.
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
[package]
2+
name = "bar"
3+
version = "0.0.0"
4+
edition = "2015"

tests/testsuite/cargo_add/invalid_inherited_dependency/in/primary/src/lib.rs

Whitespace-only changes.
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
use crate::prelude::*;
2+
use cargo_test_support::Project;
3+
use cargo_test_support::compare::assert_ui;
4+
use cargo_test_support::current_dir;
5+
use cargo_test_support::file;
6+
use cargo_test_support::str;
7+
8+
#[cargo_test]
9+
fn case() {
10+
cargo_test_support::registry::init();
11+
let project = Project::from_template(current_dir!().join("in"));
12+
let project_root = project.root();
13+
let cwd = &project_root;
14+
15+
snapbox::cmd::Command::cargo_ui()
16+
.arg("add")
17+
.args(["foo", "-p", "bar"])
18+
.current_dir(cwd)
19+
.assert()
20+
.failure()
21+
.stdout_eq(str![""])
22+
.stderr_eq(file!["stderr.term.svg"]);
23+
24+
assert_ui().subset_matches(current_dir!().join("out"), &project_root);
25+
}
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
[workspace]
2+
members = ["primary", "dependency"]
3+
4+
[workspace.dependencies]
5+
foo.workspace = true
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
[package]
2+
name = "foo"
3+
version = "0.0.0"
4+
edition = "2015"

0 commit comments

Comments
 (0)