Differentiate the warning when an alias (built-in or user-defined) shadows an external subcommand

[basile-henry]

Fixes #11149

[rust-highfive]

r? @epage

(rust-highfive has picked a reviewer for you, use r? to override)

[weihanglo]

Looks great!

Could you add a test case under tests/testsuite/cargo_alias_config.rs to exercise this behaviour? Thank you!

[ehuss]

My intent with the concern at #11099 (comment) is to just not warn at all for a built-in alias. Cargo doesn't warn about built-in commands shadowing a user-installed command. Why would a built-in alias be different? There isn't much the user can do to squelch the warning other than uninstall the command, which would prevent usage in older toolchains.

[weihanglo]

Oops. That's my fault. I understood it wrong. I agree with ehuss, and we might follow what ehuss suggests.

[epage]

My intent with the concern at #11099 (comment) is to just not warn at all for a built-in alias. Cargo doesn't warn about built-in commands shadowing a user-installed command. Why would a built-in alias be different? There isn't much the user can do to squelch the warning other than uninstall the command, which would prevent usage in older toolchains.

I thought we do warn when shadowing aliases

            (Some(_), Ok(Some(_))) => {
                // User alias conflicts with a built-in subcommand
                config.shell().warn(format!(
                    "user-defined alias `{}` is ignored, because it is shadowed by a built-in command",
                    cmd,
                ))?;
            }

[ehuss]

The situation that doesn't warn today is when an installed third-party subcommand is being shadowed by a built-in command. The expected scenario where this happens is when cargo subsumes an existing command, and is more of a transition (like cargo-tree moving from external to internal).

That's a warning for a user alias (something configured in config.toml) being shadowed by a built-in command. The expected scenario there is that the user is trying to change the default behavior of an existing command, something we generally don't want them to do.

The scenario for this PR is a third-party subcommand being shadowed by a built-in alias. That's more like the former scenario, where cargo is internalizing that third-party command. There is nothing the user can do to remove that built-in alias, and from their perspective there isn't much difference from a "built-in command" and a "built-in alias" as they are mostly indistinguishable.

[epage]

@basile-henry do you expect to have time to apply @ehuss feedback soon or would you be ok with someone else taking over? #11149 was deferred out of #11099 and I think we'd like to get all changes for #11099 committed before the beta branch that happens in November.

[basile-henry]

Sorry @epage, I had forgotten about this PR. I have pushed a commit following @ehuss' suggestion.

There is nothing the user can do to remove that built-in alias

I think if you're the author of that third-party command that is being shadowed by a built-in alias, you might consider naming your third-party command something else. But I see your point, they would likely notice the issue fairly quickly anyway. 👍

[basile-henry]

Should I squash/rebase the commits? What's the usual cargo policy?

[epage]

@basile-henry I don't think we are too strict on commits, otherwise we'd have bors squash or we'd have a tool to help enforce commits were somewhat in shape.

For me personally, I clean up my commits so that they are atomic and clearly communicate the intended change. Let us know if you'd like to do that and I'll hold off on merging

[basile-henry]

I'm not sure how to fix the failing tests. The two tests that are failing have nothing to do with this PR. It looks like the difference is around which type of quotes are emitted in an error message. And these tests pass on my machine 😅

[epage]

#11235 fixed the failing test. A new release of clap came out and we have tests that are sensitive to text output but don't have a lock file to hold us to a specific version, so the tests failed.

[basile-henry]

I can rebase and remove my workaround once #11235 goes in 👍

[epage]

Its merged

[epage]

@bors r+

[bors]

📌 Commit 3ff6d9e has been approved by epage

It is now in the queue for this repository.

[bors]

⌛ Testing commit 3ff6d9e with merge b332991...

[bors]

☀️ Test successful - checks-actions
Approved by: epage
Pushing b332991 to master...