Make component removal best-effort and order-independent

[Inconnu08]

Previously, rustup component remove parsed and removed components in a single pass,
which meant the command stopped on the first error after potentially removing earlier
valid components. As a result, behavior depended on argument order.

This change makes component removal best-effort by:

• first collecting all parseable requested components
• then attempting to remove each of them
• deferring error reporting until the end

This makes the behavior consistent regardless of whether an invalid component appears
before or after valid ones as discussed here: #4794 .

Also adds a regression test covering both orderings.

Closes #4794

[rami3l]

Many thanks for working on this :)

View changes since this review

[djc]

These commits are really not independent -- would prefer to have them all squashed.

View changes since this review

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[rami3l]

Hmmm my apologies, but I think I might need some extra time for a final look at this before merging 🙏

[rami3l]

I'm feel okay about it globally but I have a few concerns:

• I would like to confirm with @FranciscoTGouveia whether the original intention of #4794 is to aggregate certain errors (which is what I believe) rather than just showing the first error but with the bail-out delayed. IMHO the behavior as implemented in this patch will be very confusing to the user if multiple errors do happen. In this case I suggest that we either aggregate UnknownComponent* separately and early bail out for the rest, or actually aggregate all errors (which might be more difficult in terms of display).

• The current distinction of UnknownComponent and UnknownComponents doesn't seem super necessary since it doesn't prevent the API user from creating the latter with only one Vec member.

• The tuples in UnknownComponents might be better expressed via a separate struct, but that's a minor implementation detail.

View changes since this review

[Inconnu08]

Thanks, that makes sense.

My intent with the current implementation was to:

• preserve the existing UnknownComponent behavior for the single-component case
• aggregate unknown component names only when multiple invalid components are present
• keep the rest of the behavior as close as possible to the existing flow

One of the motivations for introducing UnknownComponents was also to avoid repeating the full error message multiple times when several invalid components are provided, which felt a bit noisy UX-wise. The grouped format seemed more concise and easier to read in that case.

That said, I agree the mixed behavior could be confusing if multiple different error types occur. I’m happy to adjust the approach once the intended UX for #4794 is confirmed with @FranciscoTGouveia whether that means aggregating only UnknownComponent* and bailing early for others, or reshaping the error handling more uniformly.

Also, point on UnknownComponent vs UnknownComponents: I introduced the second mainly to avoid regressing the existing single-component behavior, but I’m open to simplifying that if we settle on a cleaner model.

And agreed on the tuple, a small struct would likely make that clearer if we keep the grouped variant.

[FranciscoTGouveia]

The original intention of 4794 was indeed to install all possible components and report back every invalid one.

For example, running rustup component add rust-analyzere rustc-dev llvm-toolse, should install rustc-dev successfully, while returning two separate errors, one for each misspelled component.

On another note, I briefly tested the patch and believe it would be worth adding a test case covering the example above, as this is the current output:

$ my-rustup component add rust-analyzere rustc-dev llvm-toolse
error: toolchain '1.90-x86_64-unknown-linux-gnu' does not contain component 'rust-analyzere' for target 'x86_64-unknown-linux-gnu'; did you mean 'rust-analyzer'?

[rami3l]

The original intention of 4794 was indeed to install all possible components and report back every invalid one.

@FranciscoTGouveia Thanks for your reply! Given that, my suggestions would be:

• It'd be easier to just aggregate all UnknownComponent* errors in the installation process and bail out immediately like before if other kinds of errors do occur.

• On the distinction of UnknownComponent vs UnknownComponents, I'd say it's more idiomatic to special-case the display format on a unified UnknownComponents, rather than doing two enum variants. In other words: if the Vec is of length 1 we print like the old UnknownComponent, otherwise we use the new printing format as introduced here.

• It'd be better if the helper struct could be introduced.

@Inconnu08 What do you think?

[rami3l]

(Assigned @FranciscoTGouveia for review upon his request.)

[FranciscoTGouveia]

View changes since the review

What if we insert a newline before calling suggest_message?
Otherwise, we could easily exceed 80 chars :/

[FranciscoTGouveia]

View changes since the review

There should also be a third case in which a valid component appears between invalid ones, ensuring that errors are correctly reported for both invalid components.