fix(toolchain): forbid toolchain names starting with +

[cachebag]

Users accidentally use +stable with rustup run; in this PR we now forbid toolchain names starting with + globally.

closes #4571

[djc]

This makes sense to me, thanks!

[rami3l]

@djc Just confirming, currently it's far from common but we do allow toolchain names with the + prefixes right? I don't see anything about that in rustup toolchain link --help so I assume it's still valid.

If that is the case, then it's possible that we need to raise this error later in the pipeline, only when a toolchain with the said name is not found?

As a user used to +toolchain, it can be easy to accidentally do that with cargo run but the error you get doesn't communicate that problem:

$ rustup run +stable cargo check
error: toolchain '+stable' is not installed
#4571

Premature error throwing

[djc]

From what I'm seeing + prefixes are only specifically supported at the top level (in both rustup and proxy mode). I definitely don't recall any details but I think I'm comfortable with the risk of these changes.

[rami3l]

From what I'm seeing + prefixes are only specifically supported at the top level (in both rustup and proxy mode). I definitely don't recall any details but I think I'm comfortable with the risk of these changes.

@djc Sorry for not being clear enough previously, but what I wanted to say is about the edge case of a toolchain name starting with +, say +foo, so that the calls would be written as cargo ++foo ....

I know this sounds crazy and should be banned in #4059, but IIRC for now such names are still valid 😓

> rustup run +foo cargo
error: toolchain '+foo' is not installed

> rustup ++foo show
Default host: aarch64-apple-darwin
[...]
error: toolchain '+foo' is not installed

... totally coherent 😱

[djc]

Ahh. Let's not worry about it and/or ban it now?

[rami3l]

@djc Yeah, why not? I'd expect this breakage will be relatively small and nearly no one will actually notice it.

@cachebag How about just banning all toolchain names starting with + right now?

Instead of the current changes, put the general part in the fn validate(candidate: &str) -> Result<&str, InvalidName>; function (that's where common rules are defined), and put the hint specific to #4571 in rustup run and just suggest using the toolchain name without that prefix, without being specific to rustup run.

[rami3l]

Many thanks! I'm okay with this as long as the name of the commit and the PR is changed to reflect the latest changes, say forbid toolchain names starting with `+`.

@djc What do you think?

[cachebag]

forbid toolchain names starting with +

Does this also steer from the original issue at hand?

[rami3l]

Hmmm the original error message was confusing but made sense exactly because + has been permitted at the beginning of the toolchain names... So I won't call this a deviation.

Also, given the current error message, it should be easy to deduce that a toolchain name is expected as the positional argument here, rather than the +-prefixed form.

Besides, I think this is also an opportunity for us to test out what toolchain names are actually used so at least we'd have some expectation of what can be broken.

Finally, this could allow smarter guesses to happen, since we are sure that the + prefix here is not conveying any particular intention.

• For example, when stable is installed we can eventually issue something like: "+stable is not a valid toolchain name, did you mean stable?" which IMHO would be a great follow-up to do.

[rami3l]

As I already said, this looks good to me as long as @djc feels the same. However we could potentially use more prudence in merging yet another breaking change between beta and stable... I'd currently prefer playing the safe card and postpone this to the upcoming 2.0 👀

[djc]

However we could potentially use more prudence in merging yet another breaking change between beta and stable... I'd currently prefer playing the safe card and postpone this to the upcoming 2.0 👀

I mean, do you have a concrete plan for 2.0? When will that be released? I still think the version number is not going to make much difference in how much testing/caution people exercise before/when upgrading.

[rami3l]

I mean, do you have a concrete plan for 2.0? When will that be released? I still think the version number is not going to make much difference in how much testing/caution people exercise before/when upgrading.

@djc Thank you for this constructive question.

I'm sorry but I'm not sure if we are on the same page here; I think my stance with breaking changes will not change significantly whether the next non-patch release will be 1.30 or 2.0, as it is in theory more dangerous after a beta release has been cut.

Of course, in terms of actual impact this should be considered a minor breakage, so I'm also okay with merging this patch immediately, as long as enough attention has been drawn. Your call!

[djc]

Given the amount of response on internals I have no reason to feel the beta has resulted in a substantial amount of testing. Maybe we should do a post to the Inside Rust blog?

[rami3l]

Given the amount of response on internals I have no reason to feel the beta has resulted in a substantial amount of testing. Maybe we should do a post to the Inside Rust blog?

@djc Yes, that's a good idea! I'll take care of it.

Besides, after giving this PR another look, I think I'm probably worrying too much again over a small change that can be easily reverted in the next patch release. My apologies!