Warning on conflicting keys

[hi-rustin]

Signed-off-by: hi-rustin rustin.liu@gmail.com

What does this PR try to resolve?

close #10299 and close #10317

How should we test and review this PR?

• Warning on conflicting proc_macro and crate_types keys.
• Warning on conflicting dev-dependencies, build-dependencies, and default-features keys.

[rust-highfive]

r? @ehuss

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

[weihanglo]

I feel like crate-types got the same historic problem. Would you like to address it as well in this PR?

cargo/src/cargo/util/toml/mod.rs

Lines 1961 to 1966 in 58790d3

	// The intention was to only accept `crate-type` here but historical
	// versions of Cargo also accepted `crate_type`, so look for both.
	#[serde(rename = "crate-type")]
	crate_type: Option<Vec<String>>,
	#[serde(rename = "crate_type")]
	crate_type2: Option<Vec<String>>,

Edit: a few more fields in src/cargo/util/toml/mod.rs have alternative names. Just search 2: and you'll see 😆

[hi-rustin]

Edit: a few of fields in src/cargo/util/toml/mod.rs have alternative name. Just search 2: and you'll see 😆

Sent an issue for this. #10317

I feel like crate-types got the same historic problem. Would you like to address it as well in this PR?

Yes, I also found it. I will address it later. Thanks for your review.

[alexcrichton]

AFAIK this also affects dev-dependencies, build-dependencies, and default-features.

[hi-rustin]

AFAIK this also affects dev-dependencies, build-dependencies, and default-features.

@alexcrichton I'm not quite sure what this effect means? I've added dev-deps to my tests but it doesn't seem to have any effect?

[alexcrichton]

If we want to go so far as to warn on duplicate manifest keys like this (which I'm not 100% sold on but I won't block the effort) then there are other keys which can be duplicated, such as dev_dependencies and dev-dependencies. I don't think that this logic should only apply to one of the keys that's duplicate-able.

[hi-rustin]

@alexcrichton I have created an issue to track other keys. #10317

[alexcrichton]

r? @alexcrichton

Sorry I'm kind of confused, this PR is addressing some conflicting keys but not others? Is there a reason to not address the known ones here all using the same infrastructure?

[hi-rustin]

Is there a reason to not address the known ones here all using the same infrastructure?

My original thought was to warn only about the fields mentioned in the issue in this PR. So let me change them all in this PR.

[hi-rustin]

@rustbot ready

[hi-rustin]

@alexcrichton Friendly Ping~

[alexcrichton]

Thanks for the PR, but I'm going to be stepping down from the Cargo team so I'm going to un-assign myself from this. The Cargo team will help review this when they get a chance.

[weihanglo]

I would love to reuse a function to remove code duplication (though it helps little 🥲). And the warning message should point out that the old field is ignored. Something like

fn warn_on_deprecated(new_path: &str, name: &str, kind: &str, warnings: &mut Vec<String>) {
    let old_path = new_path.replace("-", "_");
    warnings.push(format!(
        "conflicting between `{new_path}` and `{old_path}` in the `{name}` {kind}.\n
        `{old_path}` is ignored and not recommended for use in the future"
    ))
}

[hi-rustin]

I would love to reuse a function to remove code duplication

Added. Thanks for your review! 💚 💙 💜 💛 ❤️

[weihanglo]

@bors r+

Thanks!

[bors]

📌 Commit 8b895cc has been approved by weihanglo

[bors]

⌛ Testing commit 8b895cc with merge 56ccd1f...

[bors]

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 56ccd1f to master...