WIP: wrote 3 tests about improving the error message in the path dependency by ibilalkayy · Pull Request #16138 · rust-lang/cargo

ibilalkayy · 2025-10-22T14:11:37Z

What does this PR try to resolve?

This PR is trying to improve the error message when the wrong package is found in the path dependency. Currently the PR started from the tests.

How to test and review this PR?

There are 3 tests added that cover each case that is brought up in this issue.

invalid_package_name_in_path()
- It reads the main manifest file to find that the definitely_not_bar exists in the crates/bar and it goes to the crates/bar to find that it is not present there and it gives the error.
invalid_package_in_subdirectory()
- It reads the main manifest file to find that the definitely_not_bar exists in the crates/bar and after going there, it does not find it. Then it goes more deeper in the definitely_not_bar to find that the manifest file exists there and give use the helpful message.
invalid_manifest_in_path()
- It reads the main manifest file to find that definitely_not_bar exists in the crates/bar but after going there, it finds that there are other two packages but definitely_not_bar is not present and it gives the message.

rustbot · 2025-10-22T14:11:42Z

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

tests/testsuite/path.rs

ibilalkayy · 2025-10-29T16:38:30Z

@epage I noticed that there were 7 spaces missing on both lines. So I added them and the test got passed.

I have squashed and pushed the code, but these spaces are not showing. Maybe it's a GitHub thing.

There is a conflict that came after your changes. Should I resolve the error by keeping this function or remove it?

function get_codegen_section_x(sections) {...}

weihanglo · 2025-11-26T15:22:44Z

src/cargo/core/resolver/errors.rs

 use super::context::ResolverContext;
 use super::types::{ConflictMap, ConflictReason};

+fn debug_source_path() {


If these are for local debugging purpose, please remove.

This was showing where they are at in experimenting while they then work to change this to fit in

weihanglo · 2025-11-26T15:25:03Z

tests/testsuite/path.rs

+            edition = "2015"
+            authors = []
+
+            [workspace]


Is [workspace] here essential? If not maybe we should remove.

epage · 2025-12-03T14:57:05Z

src/cargo/core/resolver/errors.rs

-    let gctx = GlobalContext::default().unwrap();
-    let sid = SourceId::for_path(path).unwrap();
-
+fn debug_source_path(msg: &mut String, path: &Path, gctx: &GlobalContext, sid: SourceId) {


These should go below where they are used so that the first item in the file is the starting point for reading it and going further down just adds more context

epage · 2026-01-29T21:44:53Z

src/cargo/core/resolver/errors.rs

-            &mut msg,
-            "no matching package named `{}` found",
-            dep.package_name()
-        );


This was moved from out of the else block, into it.

ibilalkayy · 2026-03-05T23:22:26Z

Hey @epage, I have now written my doubts and confusion in the comment that you can review it.

Here is the code where I wrote comment: https://github.com/rust-lang/cargo/pull/16138/changes

There is one functionality about the invalid_manifest_in_path remaining that needs to be written and called out

Plus I am not sure why the tests are passing locally but failing here with these two functions.

…r the path dependencies

…for the path dependencies

epage · 2026-03-06T19:01:42Z

tests/testsuite/path.rs

+        .with_status(101)
+        .with_stderr_data(str![[r#"
+[ERROR] no matching package named `bar` found at `definitely_not_bar`
+[NOTE] required by package `foo v0.5.0 ([ROOT]/foo)`


I'm assuming this test is failing. Tests should be atomic which includes tests passing. This should be showing the existing behavior so that the following commit then changes that behavior so we can see the new behavior.

epage · 2026-03-06T19:02:26Z

src/cargo/core/resolver/errors.rs

+
+            let global_context = match gctx {
+                Some(context) => context,
+                None => &GlobalContext::new(Shell::new(), PathBuf::new(), PathBuf::new()),


It seems suspicious for us to create global contexts on the fly

epage · 2026-03-06T19:04:50Z

src/cargo/core/resolver/errors.rs

+                    &mut msg,
+                    "no matching package named `{}` found at `{}`",
+                    name.name,
+                    dep.package_name()


Grammatically, this reads in an odd way.

"at" in a situation like this implies a location which would likely to be assumed to be a path but to say "package foo not found in package bar" is odd because they are both packages and a package cannot have packages inside of it.

I would instead recommend having a note naming the package that does exist at a specific path

epage · 2026-03-06T19:05:57Z

src/cargo/core/resolver/errors.rs

+            let root_package =
+                inspect_root_package(&mut msg, Path::new(&path), global_context, sid);
+
+            if let Some(name) = root_package {


what happens if a root package is not found?

epage · 2026-03-06T19:06:45Z

src/cargo/core/resolver/errors.rs

+) -> Option<RootPackageInfo> {
+    let mut ps = PathSource::new(path, sid, gctx);
+
+    ps.root_package().expect("failed to get the root");


expects are an assert and should name the condition that is upholding this ("I expect foo to be true")

epage · 2026-03-06T19:06:49Z

src/cargo/core/resolver/errors.rs

+                .source_id()
+                .url()
+                .to_file_path()
+                .expect("[ERROR]: failed to get the path");


expects are an assert and should name the condition that is upholding this ("I expect foo to be true")

epage · 2026-03-06T19:08:06Z

src/cargo/core/resolver/errors.rs

+
+    let pkg = ps
+        .read_package()
+        .expect("failed to read the package in root");


Why are we getting the root package, throwing it away, then calling read_package?

epage · 2026-03-06T19:08:32Z

src/cargo/core/resolver/errors.rs

+        .read_package()
+        .expect("failed to read the package in root");
+
+    let package_info = RootPackageInfo {


Why do we need a struct for this?

epage · 2026-03-06T19:09:38Z

src/cargo/core/resolver/errors.rs

+                let _ = writeln!(
+                    &mut msg,
+                    "help: package `{}` exists at `{}`",
+                    name.name, name.name


The at is giving the package name

epage · 2026-03-06T19:09:50Z

src/cargo/core/resolver/errors.rs

+                let _ = writeln!(
+                    &mut msg,
+                    "help: package `{}` exists at `{}`",
+                    name.name, name.name


epage · 2026-03-06T19:10:26Z

src/cargo/core/resolver/errors.rs

+    _path: PathBuf,
+}
+
+fn inspect_recursive_packages(


this name doesn't say what it is doing

epage · 2026-03-06T19:10:45Z

src/cargo/core/resolver/errors.rs

+    msg: &mut String,
+    path: &Path,
+    gctx: &GlobalContext,
+    sid: SourceId,
+) -> Option<RootPackageInfo> {


A bit unusual to have a function like this also modify msg

epage · 2026-03-06T19:10:49Z

src/cargo/core/resolver/errors.rs

+}
+
+fn inspect_recursive_packages(
+    msg: &mut String,


A bit unusual to have a function like this also modify msg

epage · 2026-03-06T19:11:20Z

src/cargo/core/resolver/errors.rs

+            } else if let Some(name) = recursive_packages {
+                let _ = writeln!(
+                    &mut msg,
+                    "no matching package named `{}` found at `{}`",


We sure are repeating this a lot

epage · 2026-03-06T19:12:20Z

src/cargo/core/resolver/errors.rs

+            let manifest = pkg.manifest_path();
+            let pkg_dir = manifest
+                .parent()
+                .expect("failed to take the parent path")


ditto about expect

epage · 2026-03-06T19:12:32Z

src/cargo/core/resolver/errors.rs

+            // } else {
+            //     let list = rps.list_files(&pkg).unwrap();
+            //     println!("{:?}", list);


epage · 2026-03-06T19:12:40Z

src/cargo/core/resolver/errors.rs

+            let package_info = RecursivePackageInfo {
+                name: pkg.name().to_string(),
+                _path: pkg_dir,
+            };


Why a struct?

rustbot assigned ehuss Oct 22, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 22, 2025

epage marked this pull request as draft October 22, 2025 14:20

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 22, 2025

epage reviewed Oct 22, 2025

View reviewed changes

tests/testsuite/path.rs Outdated Show resolved Hide resolved

epage reviewed Oct 22, 2025

View reviewed changes

tests/testsuite/path.rs Outdated Show resolved Hide resolved

epage reviewed Oct 22, 2025

View reviewed changes

tests/testsuite/path.rs Outdated Show resolved Hide resolved

epage changed the title ~~wrote 3 tests about improving the error message in the path dependency~~ WIP: wrote 3 tests about improving the error message in the path dependency Oct 22, 2025

ibilalkayy force-pushed the path_dependency_msg_improvement branch from 2a4dc4d to 2bf90ff Compare October 29, 2025 14:15

epage reviewed Oct 29, 2025

View reviewed changes

tests/testsuite/path.rs Outdated Show resolved Hide resolved

ibilalkayy force-pushed the path_dependency_msg_improvement branch from 2bf90ff to 4d041ea Compare October 29, 2025 16:29

rustbot added A-cli-help Area: built-in command-line help A-completions Area: shell completions A-documenting-cargo-itself Area: Cargo's documentation A-timings Area: timings Command-test labels Oct 29, 2025

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Oct 29, 2025

ibilalkayy force-pushed the path_dependency_msg_improvement branch 3 times, most recently from a12c57e to 9c538bb Compare November 5, 2025 17:00

rustbot added the A-dependency-resolution Area: dependency resolution and the resolver label Nov 26, 2025

weihanglo reviewed Nov 26, 2025

View reviewed changes

epage reviewed Dec 3, 2025

View reviewed changes

ibilalkayy force-pushed the path_dependency_msg_improvement branch from a9a8dc1 to 9695b25 Compare January 13, 2026 07:23

epage reviewed Jan 29, 2026

View reviewed changes

ibilalkayy force-pushed the path_dependency_msg_improvement branch 2 times, most recently from 196d02a to a025762 Compare February 18, 2026 17:23

ibilalkayy force-pushed the path_dependency_msg_improvement branch 2 times, most recently from 7e5a250 to 5ccd130 Compare March 5, 2026 22:12

ibilalkayy requested a review from epage March 5, 2026 23:22

ibilalkayy force-pushed the path_dependency_msg_improvement branch from 5ccd130 to 754b8ad Compare March 6, 2026 12:09

ibilalkayy added 4 commits March 6, 2026 22:48

test(root package): test the invalid package name for path dependencies

3eb76f7

fix(root package): inspect the root package name for path dependencies

c60d65a

test(recursive packages): test the inavlid package in subdirectory fo…

bc48c21

…r the path dependencies

fix(recursive packages): inspect the inavlid package in subdirectory …

94cd203

…for the path dependencies

ibilalkayy force-pushed the path_dependency_msg_improvement branch from 754b8ad to 94cd203 Compare March 6, 2026 18:54

epage reviewed Mar 6, 2026

View reviewed changes

src/cargo/core/resolver/errors.rs

_path: PathBuf,

}

fn inspect_recursive_packages(

Copy link

Contributor

epage Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this name doesn't say what it is doing

epage reviewed Mar 6, 2026

View reviewed changes

Conversation

ibilalkayy commented Oct 22, 2025 • edited by epage Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

How to test and review this PR?

Uh oh!

rustbot commented Oct 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

ibilalkayy commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ibilalkayy commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ibilalkayy commented Oct 22, 2025 •

edited by epage

Loading

ibilalkayy commented Oct 29, 2025 •

edited

Loading

ibilalkayy commented Mar 5, 2026 •

edited

Loading