fix(toml): Provide a way to show unused manifest keys for dependencies

[Muscraft]

Dependencies have not been able to show unused manifest keys for some time, this problem partially resulted in #11329.

This problem is caused by having an enum when deserializing. To get around this you can use:

#[serde(flatten)]
other: BTreeMap<String, toml::Value>,

This collects any unused keys into other that can later be used to show warnings. This idea was suggested in a thread I cannot find but is mentioned in serde#941.

[rustbot]

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

[ehuss]

I'm a bit confused as to the full details of the situation. Some of these ignored messages work on stable or nightly, and I'd like to dig into the details of what is going on.

Starting with #11565, none of the ignored messages seem to be working. Can you explain why serde_ignored isn't handling them starting with that PR, but was working before? Which situations worked and which ones didn't? Is there something about untagged enums that doesn't work? Would it be possible to have a custom deserialize impl that avoids needing special handling here?

[Muscraft]

Before #11565, the two tests added in this PR pass with only minor changes (location of a few messages). I believe this is due to IntermediateDependency. IntermediateDependency made it so anything that wasn't defined within it would throw an unused manifest key warning. This is expected behavior, but it wouldn't throw those warnings if what it would be turned into didn't have one of those fields. This is ultimately the root cause of #11329.

Test showing it misses `git` when using workspace inheritance

#[cargo_test]
fn warn_inherit_unused_manifest() {
    Package::new("dep", "0.1.0").publish();

    let p = project()
        .file(
            "Cargo.toml",
            r#"
            [workspace]
            members = []
            [workspace.dependencies]
            dep = { version = "0.1", wxz = "wxz" }
            [package]
            name = "bar"
            version = "0.2.0"
            authors = []
            [dependencies]
            dep = { workspace = true, git = "wxz" }
        "#,
        )
        .file("src/main.rs", "fn main() {}")
        .build();

    p.cargo("check")
        .with_stderr(
            "\
[WARNING] [CWD]/Cargo.toml: unused manifest key: workspace.dependencies.dep.wxz
[WARNING] [CWD]/Cargo.toml: unused manifest key: dependencies.dep.git
[UPDATING] `[..]` index
[DOWNLOADING] crates ...
[DOWNLOADED] dep v0.1.0 ([..])
[CHECKING] [..]
[CHECKING] bar v0.2.0 ([CWD])
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
",
        )
        .run();
}

#11565 mostly addresses #11523 by making it so TomlDependency is just a normal TomlDependency and TomlWorkspaceDependency is opt-in. The other thing #11565 does is make things easier to read and understand, making things more unified and easier to split out in the future. The end goal with work similar to #11565 is to make toml/mod.rs easier to understand and make changes to.

As for why #11565 caused this regression/why IntermediateDependency partially worked, it is due to how serde_ignored works. Fields inside of an untagged enum are known to have problems with serde_ignored since serde must try to deserialize it into all possible formats. Since it must try all formats, things that should throw an unused manifest key warning don't.

This PR tries to fix that by using #[serde(other)], which can collect everything that would be "unused". We can then output those later. I am unsure if a custom deserialize impl would solve this issue. I would have to dig much deeper into that area to know if it is a viable solution compared to what I have already come up with.

[ehuss]

Since this is duplicated code with DepTable::kind_table, could that instead be moved to a method of DepKind and reuse it?

[ehuss]

Note: This doesn't handle quoting TOML keys. But it looks like stringify doesn't do that, either. Probably not too important, just wanted to note it.

[ehuss]

Hm, that's unfortunate. It seems like this affects everything that uses MaybeWorkspace. Would it be possible to fix that, too?

I'd be reluctant to add a bunch of manual checks for ignored fields. Would it be possible to add custom deserializers for the different MaybeWorkspace kinds? For example, the string kinds can be something like:

type MaybeString = MaybeWorkspace<String, TomlWorkspaceField>;

impl<'de> de::Deserialize<'de> for MaybeString {
    fn deserialize<D>(d: D) -> Result<Self, D::Error>
    where
        D: de::Deserializer<'de>,
    {
        struct Visitor;

        impl<'de> de::Visitor<'de> for Visitor {
            type Value = MaybeString;

            fn expecting(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
                f.write_str("a string or workspace")
            }

            fn visit_string<E>(self, value: String) -> Result<Self::Value, E>
            where
                E: de::Error,
            {
                Ok(MaybeString::Defined(value))
            }

            fn visit_map<V>(self, map: V) -> Result<Self::Value, V::Error>
            where
                V: de::MapAccess<'de>,
            {
                let mvd = de::value::MapAccessDeserializer::new(map);
                TomlWorkspaceField::deserialize(mvd).map(MaybeWorkspace::Workspace)
            }
        }

        d.deserialize_any(Visitor)
    }
}

Then serde_ignored should work for those kinds of fields.

(I just hacked that together, it may need some work.)

The only things that will have problems are those that are maps. From what I can tell, that is just dependencies and badges. Badges might be a little complicated since they will likely need an other field, too, so they couldn't reuse TomlWorkspaceField.

That may result in more total lines of code, so I'm not 100% sure it is worth it, but it might help ensure that any fields added in the future don't forget to manually check for unused keys.

[ehuss]

@Muscraft Just checking if you had any questions here. I'm a little concerned about the regression in diagnostics in 1.69. If a more general solution for the inherited definitions looks to be a lot of work, I'd be happy to move forward with a fix for the primary issue of ignored fields in dependency declarations, and address other cases in future PRs.

[Muscraft]

It looks like I need to rebase onto master. The changes between my first commit and the one I force pushed are here

[ehuss]

This looks great, thanks!

We discussed the unfortunate increase in lines of code here with all the serde impls. @Muscraft indicated interest in organizing things into more submodules, which sounds great to me, but they will follow up on that later.

@bors r+

[bors]

📌 Commit a32af2f has been approved by ehuss

It is now in the queue for this repository.

[bors]

⌛ Testing commit a32af2f with merge 0942fc7...

[bors]

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 0942fc7 to master...

[weihanglo]

Nominated beta backport to 1.69.