Helpful error for invalid `cargo add` #15688

kornelski · 2025-06-21T09:59:36Z

When users try to use paths and URLs directly in cargo add, like cargo add ../path or cargo add ssh://git/dep.git, cargo treats it as a package name syntax error and complains that "characters must be Unicode XID characters", which isn't addressing users' misconception.

I've added "note: ..." with recovery suggestions to these errors.

I've also noticed that cargo add --path dir/Cargo.toml errs with "failed to read dir/Cargo.toml/Cargo.toml". Explaining that failure case felt silly, so I've made --path automatically strip Cargo.toml from the path instead.

rustbot · 2025-06-21T09:59:40Z

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

epage

Thanks for making the errors better!

epage · 2025-06-21T11:31:42Z

src/cargo/ops/cargo_add/mod.rs

-        let path = paths::normalize_path(&std::env::current_dir()?.join(raw_path));
+        let mut relative_path = Path::new(raw_path);
+        if relative_path.ends_with("Cargo.toml") {
+            relative_path = relative_path.parent().unwrap();


If you'd like to speed up this PR, I'd recommend removing this as it is a new feature that adds to our compatibility surface that mirrors other features,

I've just changed it to an error. I feel that if I make it a separate PR, you'll ask to make it consistent with install, and install uses SourceId here, and that would require a whole refactoring of both commands, and that's too much for me.

epage · 2025-06-21T11:33:07Z

src/cargo/ops/cargo_add/mod.rs

@@ -104,6 +105,14 @@ pub fn add(workspace: &Workspace<'_>, options: &AddOptions<'_>) -> CargoResult<(
                    options.gctx,
                    &mut registry,
                )
+                .map_err(|err| {


Please add a test for this change. Ideally, add it in a commit before, with it passing so it shows the current behavior and this commit would then show how the behavior changed.

In principle I agree this should be tested, but the test framework has such high overhead that I think it's unreasonable to use it for such a tiny feature: #15691

While there might be overhead, we'd still want to see tests for changes, even ones like this. While there can be improvements, how we currently test is the reality we are currently in and doesn't affect the need for tests.

For example, a test will catch if the error you are matching on changes.

I've restructured the code to require an exact error type (so that the integration point is checked by the type system, not a test). Then I've added a localized unit test for the suggestion. I think this way it can test more cases, quicker, and isn't as sensitive to irrelevant UI changes as the end-to-end tests are.

src/cargo/ops/cargo_add/mod.rs

epage · 2025-06-21T11:36:05Z

src/cargo/ops/cargo_add/mod.rs

+    {
+        Some("git URLs must be specified with --git")
+    } else if arg.registry.is_none()
+        && (spec.starts_with("registry+") || spec.starts_with("sparse+"))


--registry values do not start with those but are names to look up urls in config

Updated the message

epage · 2025-06-21T11:37:36Z

src/cargo/ops/cargo_add/mod.rs

+fn spec_fix_suggestion(arg: &DepOp) -> Option<&'static str> {
+    let spec = arg.crate_spec.as_deref()?;
+
+    if arg.git.is_none()


For each of these, you are checking if a related flag is used. How come?

I want to avoid suggesting a flag that has already been specified by the user

epage · 2025-06-21T11:38:31Z

src/cargo/ops/cargo_add/mod.rs

+    {
+        Some("registy can be specified with --registry")
+    } else if spec.contains("://") {
+        Some("`cargo add` expects crates specified as 'name' or 'name@version', not URLs")


Can't this be a git url as well?

Yes, that's why the text is phrased to fit use along --git too (e.g. in case they swapped url with crate names)

epage · 2025-06-21T11:55:53Z

Should we extend this to cargo install which has a similar cli?

epage · 2025-06-21T13:29:17Z

src/cargo/ops/cargo_add/mod.rs

@@ -378,7 +408,11 @@ fn resolve_dependency(
        };
        selected
    } else if let Some(raw_path) = &arg.path {
-        let path = paths::normalize_path(&std::env::current_dir()?.join(raw_path));
+        let relative_path = Path::new(raw_path);
+        if relative_path.ends_with("Cargo.toml") {


What if there is a directory named Cargo.toml or we change what we allow for paths?

This is why I like the other approach of augmenting an existing error rather than assuming its an error.

I get your concern about edge cases, and I usually prefer to implement a robust solution. However, these edge cases are extremely unlikely to cause any real problems, and I don't think any other solution here is practical. From the perspective of what I can hope to achieve in Cargo, it's either this or nothing.

The problem is that the codebase doesn't already have a proper place to do this where this would count as a small-enough non-objectionable change.

The conversion from path CLI arguments to path sources is already done differently in multiple places. Duplicating the same hack in multiple places wouldn't be appropriate, but not duplicating the hack would require refactoring the code to remove the duplication first. I would actually like to do that! I enjoy doing such code cleanups!

However, I know you don't approve of unsolicited refactors like that, and you don't trust my judgement on program's architecture or coding style. The process for proposing, justifying, and getting approval for a cross-cutting refactoring in Cargo is prohibitively time-consuming. For an outside contributor like me it adds non-coding overhead that is totally out of proportion of the problem being solved here.

So I'm leaving the crappy error:

error: failed to read …/Cargo.toml/Cargo.toml

epage · 2025-06-21T13:32:20Z

src/cargo/ops/cargo_add/mod.rs

+    } else if arg.registry.is_none()
+        && (spec.starts_with("registry+") || spec.starts_with("sparse+"))
+    {
+        Some("registy can be specified with --registry name")


The name is a bit ambiguous. What about making its association and being a placeholder more explicit

registry can be specified with `--registry <name>`

Should we do similar for the others?

Ok, I've used convention from the --help, with <NAME>.

rustbot assigned ehuss Jun 21, 2025

rustbot added Command-add S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 21, 2025

epage reviewed Jun 21, 2025

View reviewed changes

kornelski force-pushed the add-err branch from 4d11371 to 58a5080 Compare June 21, 2025 12:36

epage reviewed Jun 21, 2025

View reviewed changes

Helpful error for confused cargo add arguments

ccbff6f

kornelski force-pushed the add-err branch from 58a5080 to ccbff6f Compare June 21, 2025 22:22

Helpful error for invalid cargo add #15688

Are you sure you want to change the base?

Helpful error for invalid cargo add #15688

Conversation

kornelski commented Jun 21, 2025

Uh oh!

rustbot commented Jun 21, 2025

Uh oh!

epage left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epage commented Jun 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Helpful error for invalid `cargo add` #15688

Helpful error for invalid `cargo add` #15688