Skip to content

feat(add): suggest similarly named features #15438

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 1, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 31 additions & 9 deletions src/cargo/ops/cargo_add/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ use crate::core::Summary;
use crate::core::Workspace;
use crate::sources::source::QueryKind;
use crate::util::cache_lock::CacheLockMode;
use crate::util::edit_distance;
use crate::util::style;
use crate::util::toml::lookup_path_base;
use crate::util::toml_mut::dependency::Dependency;
Expand Down Expand Up @@ -159,19 +160,32 @@ pub fn add(workspace: &Workspace<'_>, options: &AddOptions<'_>) -> CargoResult<(
activated.retain(|f| !unknown_features.contains(f));

let mut message = format!(
"unrecognized feature{} for crate {}: {}\n",
"unrecognized feature{} for crate {}: {}",
if unknown_features.len() == 1 { "" } else { "s" },
dep.name,
unknown_features.iter().format(", "),
);
if activated.is_empty() && deactivated.is_empty() {
write!(message, "no features available for crate {}", dep.name)?;
write!(message, "\n\nno features available for crate {}", dep.name)?;
} else {
if !deactivated.is_empty() {
let mut suggested = false;
for unknown_feature in &unknown_features {
let suggestion = edit_distance::closest_msg(
unknown_feature,
deactivated.iter().chain(activated.iter()),
|dep| *dep,
"feature",
);
if !suggestion.is_empty() {
write!(message, "{suggestion}")?;
suggested = true;
}
}
if !deactivated.is_empty() && !suggested {
if deactivated.len() <= MAX_FEATURE_PRINTS {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we still display all of the features if there is a closest match for every unknown feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say yes, although this might be pretty lengthy

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking around at how we do this elsewhere

  • Most places do not list all options, even without a suggestion
  • One place always lists out how to see all options, independent of whether there was a suggestion
  • 2 places will conditionally show all options if a suggestion is not present

So there is only a small precedence but it tends to be towards not listing everything if there is a suggestion.

Thinking particularly for this case, we most likely want to focus the users attention on the suggestion. Also, since there was a concern over the number of suggestions, what there can be a lot more of is number of features. While we use columns, it can be quite large.

I lean towards only showing the suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking around at how we do this elsewhere

* Most places do not list all options, even without a suggestion

* One place always lists out how to see all options, independent of whether there was a suggestion

* 2 places will conditionally show all options if a suggestion is not present

So there is only a small precedence but it tends to be towards not listing everything if there is a suggestion.

Thinking particularly for this case, we most likely want to focus the users attention on the suggestion. Also, since there was a concern over the number of suggestions, what there can be a lot more of is number of features. While we use columns, it can be quite large.

I lean towards only showing the suggestion.

So, how should the output look like?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not too sure what you are looking for with this question. As I said, I lean towards only showing the suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not too sure what you are looking for with this question. As I said, I lean towards only showing the suggestion.

Sorry for the confusion, I mean: how should the suggestion look like? Just say that there N similarly spelled features, or perhaps something else?

Also, closest_msg appends two line breaks into the middle of the message, so maybe I should use different function?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the confusion, I mean: how should the suggestion look like? Just say that there N similarly spelled features, or perhaps something else?

Unless we have a good reason, we should be use closest_msg. If we do have a good reason, we should mirror it. So that means we don't say there are "N similarly spelled features".

Also, closest_msg appends two line breaks into the middle of the message, so maybe I should use different function?

I commented on that at
#15438 (comment) . Other messages don't show two blank lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

To double check, earlier in this thread, I asked about not showing deactivated or activated features. Unless I'm misunderstanding something, it looks like that isn't done yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To double check, earlier in this thread, I asked about not showing deactivated or activated features. Unless I'm misunderstanding something, it looks like that isn't done yet.

Looks like I missed it. (De)activated features shouldn't be printed only if there are feature suggestions, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that would align with other places that only exhaustively list things when there are no suggestions.

writeln!(
write!(
message,
"disabled features:\n {}",
"\n\ndisabled features:\n {}",
deactivated
.iter()
.map(|s| s.to_string())
Expand All @@ -184,14 +198,18 @@ pub fn add(workspace: &Workspace<'_>, options: &AddOptions<'_>) -> CargoResult<(
.format("\n ")
)?;
} else {
writeln!(message, "{} disabled features available", deactivated.len())?;
write!(
message,
"\n\n{} disabled features available",
deactivated.len()
)?;
}
}
if !activated.is_empty() {
if !activated.is_empty() && !suggested {
if deactivated.len() + activated.len() <= MAX_FEATURE_PRINTS {
writeln!(
message,
"enabled features:\n {}",
"\n\nenabled features:\n {}",
activated
.iter()
.map(|s| s.to_string())
Expand All @@ -204,7 +222,11 @@ pub fn add(workspace: &Workspace<'_>, options: &AddOptions<'_>) -> CargoResult<(
.format("\n ")
)?;
} else {
writeln!(message, "{} enabled features available", activated.len())?;
writeln!(
message,
"\n\n{} enabled features available",
activated.len()
)?;
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[workspace]

[package]
name = "cargo-list-test-fixture"
version = "0.0.0"
edition = "2024"

Empty file.
24 changes: 24 additions & 0 deletions tests/testsuite/cargo_add/feature_suggestion_multiple/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
use cargo_test_support::current_dir;
use cargo_test_support::file;
use cargo_test_support::prelude::*;
use cargo_test_support::Project;

#[cargo_test]
fn case() {
let project = Project::from_template(current_dir!().join("in"));
let project_root = project.root();
let cwd = &project_root;

cargo_test_support::registry::Package::new("my-package", "0.1.0+my-package")
.feature("bar", &[])
.feature("foo", &[])
.publish();

snapbox::cmd::Command::cargo_ui()
.arg("add")
.arg_line("my-package --features baz --features feo")
.current_dir(cwd)
.assert()
.failure()
.stderr_eq(file!["stderr.term.svg"]);
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[workspace]

[package]
name = "cargo-list-test-fixture"
version = "0.0.0"
edition = "2024"

Empty file.
24 changes: 24 additions & 0 deletions tests/testsuite/cargo_add/feature_suggestion_none/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
use cargo_test_support::current_dir;
use cargo_test_support::file;
use cargo_test_support::prelude::*;
use cargo_test_support::Project;

#[cargo_test]
fn case() {
let project = Project::from_template(current_dir!().join("in"));
let project_root = project.root();
let cwd = &project_root;

cargo_test_support::registry::Package::new("my-package", "0.1.0+my-package")
.feature("bar", &[])
.feature("foo", &[])
.publish();

snapbox::cmd::Command::cargo_ui()
.arg("add")
.arg_line("my-package --features none_existent")
.current_dir(cwd)
.assert()
.failure()
.stderr_eq(file!["stderr.term.svg"]);
}
38 changes: 38 additions & 0 deletions tests/testsuite/cargo_add/feature_suggestion_none/stderr.term.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[workspace]

[package]
name = "cargo-list-test-fixture"
version = "0.0.0"
edition = "2024"

Empty file.
23 changes: 23 additions & 0 deletions tests/testsuite/cargo_add/feature_suggestion_single/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
use cargo_test_support::current_dir;
use cargo_test_support::file;
use cargo_test_support::prelude::*;
use cargo_test_support::Project;

#[cargo_test]
fn case() {
let project = Project::from_template(current_dir!().join("in"));
let project_root = project.root();
let cwd = &project_root;

cargo_test_support::registry::Package::new("my-package", "0.1.0+my-package")
.feature("bar", &[])
.publish();

snapbox::cmd::Command::cargo_ui()
.arg("add")
.arg_line("my-package --features baz")
.current_dir(cwd)
.assert()
.failure()
.stderr_eq(file!["stderr.term.svg"]);
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 2 additions & 2 deletions tests/testsuite/cargo_add/features_unknown/stderr.term.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions tests/testsuite/cargo_add/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ mod dev_existing_path_base;
mod dev_prefer_existing_version;
mod dry_run;
mod empty_dep_name;
mod feature_suggestion_multiple;
mod feature_suggestion_none;
mod feature_suggestion_single;
mod features;
mod features_activated_over_limit;
mod features_deactivated_over_limit;
Expand Down
Loading