Improve cargo add -F crate/invalid_feat error discovery

[Emilgardis]

Problem

When using cargo add -F crate/feat, if the feat feature does not exist, the error is not presented until the end.

This can be easy to miss, as most cargo warnings are presented with more context/text around them. cargo add however spits out a features table as if everything is ok and then presents an error.

Proposed Solution

Attach more context in the output to clearly signal that "hey, this is not correct, check the error message"

Notes

this has been implemented in #11098, creating this per request.

[epage]

This can be easy to miss, as most cargo warnings are presented with more context/text around them.

I feel like this is the key to the problem that we should focus on.

cargo add however spits out a features table as if everything is ok and then presents an error.

Good call, we should probably not show the table like this. We should probably split up print_msg to separate out the "Adding" from the table and put the table in after everything has been processed for that dependency.

The error should probably follow our usual pattern of providing a suggestion if one exists and showing everything if one doesn't exist.

[Emilgardis]

One nice thing with providing the table, even on error, is that you can quickly see if you accidentally typoed or misremembered the feature flags name.

The error should probably follow our usual pattern of providing a suggestion if one exists

Perhaps for the attached example:

add tokio --features sync,net,rt,rr     

the error could be

error: unrecognized feature `rr`
help: a feature with a similar name exists: `rt`

and showing everything if one doesn't exist.

what does this mean?

[epage]

One nice thing with providing the table, even on error, is that you can quickly see if you accidentally typoed or misremembered the feature flags name.

I think that is also what the alternative I brought up does.

the error could be
...
what does this mean?

This is an example of an existing "not found" error in cargo: https://github.com/rust-lang/cargo/pull/10999/files

[Emilgardis]

so something like this?

crate has one feature existing_feat

$ cargo add -F crate/invalid_feat, crate/invalid_feat2 crate/existing_feat1
Adding crate v0.0.1 to dependencies
error: no feature `invalid_feat`, `invalid_feat2` or `existing_feat1` available for `crate`
Available features:
    existing_feat

I don't see how to do multiple errors from cargo add, is it possible?

[epage]

so something like this?

There are 3 scenarios

• A close match is found, suggest that
• No close matches found but features exist, so list them all
• No features exist

I don't see how to do multiple errors from cargo add, is it possible?

In what way are you wanting to do multiple errors?

[Emilgardis]

In what way are you wanting to do multiple errors?

in the cargo add -F crate/invalid_feat crate/invalid_feat2 crate/existing_feat1 example, we have two features with "no close match", and one with a close match. I think it would make sense to present the close match, but also list all.

$ cargo add -F crate/invalid_feat crate/invalid_feat2 crate/existing_feat1
Adding crate v0.0.1 to dependencies
error: no feature "existing_feat1" exists for "crate", did you mean "existing_feat"
error: features "invalid_feat" and "invalid_feat2" does not exist
    Available features: "existing_feat"

error: unrecognized features ["invalid_feat", "invalid_feat2", "existing_feat1"]

I was thinking of something like https://docs.rs/color-eyre/latest/color_eyre/section/trait.Section.html#tymethod.with_error but already built-in, i.e emitting errors, not just one error.

[epage]

Hmm, I'm thinking we should start with a simpler error message and move on from there. We should just do Available features for now instead of doing any typo suggestions.

[Emilgardis]

something like this? or maybe

[weihanglo]

I like the latter! Looks better and less noisy.

[Emilgardis]

How about this? Showing available (currently not enabled features) and enabled features

[epage]

While the long list is less cluttered, I feel like it undoes the goal of this Issue which is to draw more attention to the error itself. I would prefer the comma-separated list when things are long like this.

[Emilgardis]

maybe

?

max length 84

edit: an alternative

[weihanglo]

…when things are long like this.

You made me recall this auto-generated feature list. 1505 features in a Cargo.toml

https://github.com/rustwasm/wasm-bindgen/blob/564ce74168904e95a7905a828488ec3029bcaad4/crates/web-sys/Cargo.toml#L31-L1535

This may deserve a new issue if we want to address, along with other places with similar issues.

[epage]

@Emilgardis I think I prefer the first as the second is a bit more repetitive

"available features" isn't actually available because its missing enabled features (its legal to specify already enabled features)

[Emilgardis]

should the naming be changed for available features? I think it makes sense, even if it doesn't include all "available" features.

[epage]

We should either merge the lists of rename it to "disabled features".

The thing I like about separate lists is that the disabled list is most likely what people will want to search through so this provides a way to shrink it down.

[Emilgardis]

awesome, I've implemented this in the original pr

[Emilgardis]

I think theres one more thing to do here, should probably clearly signal that the command failed, and that nothing was changed for the crate in the manifest

Originally posted by @Emilgardis in #11098 (comment)

There's also the consideration of providing typo helping, i.e if feature name is within levenshtein distance y of (an)other feature(s), provide a "help: did you mean feature_x?" or similar

[epage]

For me, I like to keep issues well scoped so they have a clear end. The goal here was to improve visibility of the error.

clearly signal that the command failed

What isn't clear at the moment at least compared to any other cargo error?

that nothing was changed for the crate in the manifest

Is this an actual point of confusion you had or theoretical? If its theoretical, I'd like confirmation on people actually being confused so we better understand what problem we are trying to solve and to understand how wide scoping it is across cargo commands.

I would also personally see this in a distinct issue with its own discussion thread.

There's also the consideration of providing typo helping, i.e if feature name is within levenshtein distance y of (an)other feature(s), provide a "help: did you mean feature_x?" or similar

I see typo hints as a distinct issue. In addition, a dedicated issue would be useful as there is a lot of UX there'll probably be a lot of back and forth on how to make this look and having a dedicated thread for that would help keep everything straight.

[Emilgardis]

One way to interpret and interact with cargo add after #11098 is

$ cargo add tokio --features sync,net,rt,iostd
    Updating crates.io index
      Adding tokio v1.21.1 to dependencies.
error: unrecognized features for crate tokio: iostd
disabled features:
    bytes, fs, full, io-std, io-util, macros, memchr, mio, num_cpus, parking_lot
    process, rt-multi-thread, signal, signal-hook-registry, stats, test-util, time
    tokio-macros, tracing, winapi
enabled features:
    libc, net, once_cell, rt, socket2, sync

if the disabled and enabled features are long, it could be easy to miss that sync, net and rt were not enabled/written to the manifest. so if you don't "reuse" the previous command to fix the typos, you'd do

$ cargo add tokio --features io-std 
    Updating crates.io index
      Adding tokio v1.21.1 to dependencies.
             Features:
             + io-std
             + mio
             - bytes
             - fs
             - full
             - io-util
             - libc
             - macros
             - memchr
             - net
             - num_cpus
             - once_cell
             - parking_lot
             - process
             - rt
             - rt-multi-thread
             - signal
             - signal-hook-registry
             - socket2
             - stats
             - sync
             - test-util
             - time
             - tokio-macros
             - tracing
             - winapi

Is this an actual point of confusion you had or theoretical? If its theoretical, I'd like confirmation on people actually being confused so we better understand what problem we are trying to solve and to understand how wide scoping it is across cargo commands.

I've definitely had and have seen others have this problem of missing the fact that nothing was written with the previous error reporting, for me it's solved with the new way (and possibly because I now know the actual implementation) but I can see how it can be a bit confusing still.