Workspace inheritance reads from wrong workspace when one is nested inside of another

[heaths]

Problem

We have a monorepo where we want to manage some lints at the workspace level. We also have some CI scripts that set RUSTFLAGS=-Dwarnings. For crates that set lints.workspace = true, lint configuration is ignored. I've even tried setting the priority as discussed in #12208 et. al. to no avail.

If I simply set e.g., lints.clippy.useless_format = "allow" in the specific crates' Cargo.toml, no warnings-as-errors are emitted.

Steps

1. Create a workspace Cargo.toml with the content:

   [workspace.lints.clippy]
   useless_format = "allow" # or even { level = "allow", priority = -1 }

2. Create a crate Cargo.toml with content:

   [lints]
   workspace = true

3. Add some code to trigger the example lint. In our case, it's generated code where almost all strings will have format specifiers and determining otherwise for a few instances didn't seem worth the cost.

   fn example() -> String {
      format!("/some/path")
   }

4. Run the following command. Notice that clippy::useless_format warnings-as-errors are still emitted and the command fails.

   # Also tried on 1.74.0, 1.76.0, and nightly to same effect.
   # Stable for the repo is 1.74, but system-wide is 1.81 and still repros.
   RUSTFLAGS=-Dwarnings cargo +stable clippy --workspace

5. Change step 2 content to:

   [lints.clippy]
   useless_format = "allow"

6. Rust step 4 command again. Notice no errors are emitted.

Possible Solution(s)

No response

Notes

No response

Version

No response

[ehuss]

Hm, unfortunately I'm not able to reproduce that. Can you include a .zip file of a workspace the demonstrates the issue?

[heaths]

FWIW, I opened it here because my assumption was that cargo was not "flatting" the package manifest (if it even does that) before lint tables were parsed. Certainly possible this is an issue in clippy itself. In the course of trying to troubleshoot the issue, it seems plenty of people were successfully using workspace.lints.rust.

[heaths]

Hm, unfortunately I'm not able to reproduce that. Can you include a .zip file of a workspace the demonstrates the issue?

@ehuss certainly, but it'll take me a bit. Our main branch as it exists right (commit b16e8790a1d0ebe608584db675b4042796b35caf) now at https://github.com/Azure/azure-sdk-for-rust exhibits all this if you were to run RUSTFLAGS=-Dwarnings cargo +stable clippy --package azure_storage_blobs but I'm happy to try to provide a minimal repro.

To note, I went ahead and fixed the bug that was unnecessarily using format!() in some cases so main won't have this problem next week sometime. Initially it didn't seem worth the time fixing it, but I ended up spending more time troubleshooting this issue. 🤦‍♂️

[weihanglo]

I cannot reproduce it either. Guess there were something cached between the first and the second invocation. You might want to run cargo clean in between.

The implementation of [lints] table is as simple as passing corresponding -A clippy::useless_format rustc flag to rustc invocation. You could run cargo clippy with --verbose and check if -Dwarnings is passed to rustc after -A clippy::useless_format. If I am not wrong, -Dwarnings should be the last flag.

[heaths]

It took me a bit - I couldn't repro it either at first - but then I figured out what my minimal repro was missing: an empty [workspace.lints] section in the root Cargo.toml. That was there to satisfy cargo since we've been injecting lints.workspace = true in all our newer crates.

See https://github.com/heaths/rust-lang-cargo-14645, specifically heaths/rust-lang-cargo-14645@19378e5. Note that the root crate imports a dependency by path, but that dependency is part of a separate workspace.

In https://github.com/azure/azure-sdk-for-rust we are generating dozens of crates that we put into a separate workspace to avoid rust-analyzer perf degredation with hand-coded crates. One of those hand-coded crates is using the generated crate in its underlying implementation. That hand-coded crate has lints.workspace = true in a workspace with an empty [workspace.lints]. The generated crate also has lints.workspace = true but its workspace had a non-empty [workspace.lints.clippy] as documented above.

So somehow that empty workspace.lints section is affecting the path-imported dependency. If I put the useless_format = "allow" in the root Cargo.toml and remove it from the deps/Cargo.toml workspace, it still works. Seems the empty [workspace.lints] is overwriting the [workspace.lints.clippy] of the separate workspace. See heaths/rust-lang-cargo-14645#1 for an example of how the separate root workspace is impacting the crate from another workspace. Is that expected?

[epage]

https://github.com/heaths/rust-lang-cargo-14645,

What am I supposed to do with this to reproduce the issue?

[heaths]

Clone the repo and run cargo clippy from the root. You'll get a clippy::useless_format for deps/foo warning despite that crate's workspace (deps/Cargo.toml) allowing it. If you add the allow to the root workspace's Cargo.toml the issue goes away. So it seems the empty root workspace - of which the dependent root crate belongs - is effectively clearing out dependencies' workspace-configured lints. Conversely, "parent" workspaces like this can override "chlid" workspaces' lint configurations. Seems those two should remain separate. Granted, the dependency is also included by path (monorepo example), or should that be expected?

[epage]

This also reproduces with rustc which is a more minimal case as an implementation detail of Cargo's development. I switched this out for unused_variables and added a let unused = ""; in the function with the useless_format

I put in some debug statements in Cargo

[src/cargo/util/toml/mod.rs:318:9] &package_name = PackageName(
    "rust-lang-cargo-14645",
)
[src/cargo/util/toml/mod.rs:831:5] normalized_path = "/home/epage/src/personal/dump/rust-lang-cargo-14645/Cargo.toml"
[src/cargo/util/toml/mod.rs:504:9] &normalized_lints = Some(
    {},
)
[src/cargo/util/toml/mod.rs:318:9] &package_name = PackageName(
    "foo",
)
[src/cargo/util/toml/mod.rs:831:5] normalized_path = "/home/epage/src/personal/dump/rust-lang-cargo-14645/deps/foo/Car
go.toml"
[src/cargo/core/workspace.rs:1991:13] current = "/home/epage/src/personal/dump/rust-lang-cargo-14645/deps/foo"
[src/cargo/core/workspace.rs:1991:13] current = "/home/epage/src/personal/dump/rust-lang-cargo-14645/deps"
[src/cargo/core/workspace.rs:1991:13] current = "/home/epage/src/personal/dump/rust-lang-cargo-14645"
[src/cargo/util/toml/mod.rs:859:5] &workspace_path = "/home/epage/src/personal/dump/rust-lang-cargo-14645/Cargo.toml"
[src/cargo/util/toml/mod.rs:504:9] &normalized_lints = Some(
    {},
)
[src/cargo/core/workspace.rs:1991:13] current = "/home/epage/src/personal/dump/rust-lang-cargo-14645/deps/foo"
[src/cargo/core/workspace.rs:1991:13] current = "/home/epage/src/personal/dump/rust-lang-cargo-14645/deps"
[src/cargo/core/workspace.rs:1991:13] current = "/home/epage/src/personal/dump/rust-lang-cargo-14645"

The bug appears to be that our caching of parsed workspaces isn't taking into account all of the workspace load rules. This is a problem for all of workspace inheritance. I'm assuming this hasn't been found before as workspaces aren't usually nested but parallel to each other.

[SamB]

The bug appears to be that our caching of parsed workspaces isn't taking into account all of the workspace load rules. This is a problem for all of workspace inheritance. I'm assuming this hasn't been found before as workspaces aren't usually nested but parallel to each other.

Yeah, given what I've seen in the cargo docs and the Cargo.toml files I've looked at, I personally assumed that nested workspaces weren't really a thing, though I guess the workspace.exclude key should have been a clue that it was more complicated than that? Though The workspace field does explicitly state that you can't have both package.workspace and [workspaces] in the same manifest:

That is, a crate cannot both be a root crate in a workspace (contain [workspace]) and also be a member crate of another workspace (contain package.workspace).

(Fortunately, in the scenario described in this issue, nobody actually wants the two workspaces to be related.)

reference/workspaces.md also says:

When inside a subdirectory within the workspace, Cargo will automatically
search the parent directories for a Cargo.toml file with a [workspace]
definition to determine which workspace to use.

and two things jump out at me:

1. The "when" clause ("When inside a subdirectory within the workspace") doesn't make sense, because there is no way for Cargo to know "this crate is not part of a workspace" without searching.

2. It doesn't actually say which Cargo.toml wins if more than one parent has a [workspace] table:

   • Is it the outermost?
   • Is it the innermost?
   • Is this just against the rules?
   • Should Cargo skip manifests where workspace.exclude would exclude the present directory's crate?
   • Should Cargo skip manifests where the present directory's crate isn't referenced from workspace.members¹, either directly or by some sequence of path dependencies?
     (This is a rhetorical question. I suspect it would be bad for both performance and layering, assuming that it is even possible to compute this.)

Footnotes

1. Wait, not workspace.includes? ↩

[heaths]

• Is it the outermost?
• Is it the innermost?
• Is this just against the rules?

If we're discussing possible design, I'd prefer the innermost since it follows quite a bit (most?) of similar concepts in our industry such as .gitignore, .editorconfig, etc. Most features tend to look up to find the innermost "thing" and, in some cases, might continue on either implicitly e.g., .gitignore or explicitly if imported e.g., Directory.Build.props (MSBuild). This would provide a consistent experience, IMO.