rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.61k stars 2.4k forks source link

Implicit Inheritance for Workspapce Inheritance #12208

Open epage opened 1 year ago

epage commented 1 year ago

Problem

The workspace inheritance RFC deferred inheriting-by-default to not get in the way of merging a minimal solution

Use cases

Proposed Solution

Possibilities:

Notes

No response

epage commented 1 year ago

I feel like we need to gather use cases to decide if this is even worth it, so I've created this as a place to gather them.

Jasper-Bekkers commented 10 months ago

We'd like to have this feature because that the moment having to opt-in to [lints] is a no-go for us. We have medium sized workspace with about ~150 crates in there and we're frequently adding new ones. Even though cargo new automatically adds the [lints] workspace = true section, it still requires us to catch in code reviews that this has been done properly (maybe not everybody uses cargo new etc).

Ideally we can just force the clippy lints for the entire workspace & project so that we don't have to keep this in mind and we can just set it once. It's how we're currently using / abusing the [target.'cfg(all())'] rustflags hack and it's serving us quite well.

epage commented 10 months ago

Not great but a workaround would be to have a tool that errors if a workspace member does not have it inherited.

With #12235, that could be integrated as a cargo lint that is on by default.

If we had general implicit inheritance, we'd need a way to override it. Take workspace.package.rust-version and you want one package to not have it set at all. There is no "opt-out" value for that field. TOML doesn't have a None value. We could make it support false but that only works for that field and we'd need to handle it piece-meal. We could support package.rust-version.workspace = false to mean "don't inherut bit don't set it to something"

Say we don't want to go down that route, we'd then need to decide if we're ok with implicit inheritance being section by section. I feel like that could lead to its own confusion.

We also need to consider what routes (and their likelihood) we might want to take workspace inheritance. At least for [lints], I could see users wanting support for named groups of lints to inherit if there are very disparate use cases in a workspace, like firmware and application code. If we do implicit inheritance, we'd need to evaluate if we want to take into account features like this and look into their ramifications.

Nemo157 commented 10 months ago

For lints at least I feel like having explicit pushing from the workspace would work fine in most cases, it doesn't need to be implicit if you only have to configure it in one spot.

epage commented 9 months ago

@Muscraft and I did some brainstorming on this.

Inspired by the inherits field for Custom Profiles,

inherits = true

[package]
name = "foo"

...

inherits = true would be the same as if all package fields and lints had workspace = true.

With a new edition, we could have a rust-2027-compatibility lint that encourages people to set inherits = false when it is unset and make inherits = true the default. While the related Pre-RFC and RFCs shouldn't be procrastinated too long, I don't think 2027 is too bad of a time frame because the presence / absence of inherits = true is more likely to be obvious than lints.workspace = true. The biggest concern is [lints] but forgetting inherits will also make you lose out on everything else (inheriting package.version, package.rust-version, package.edition, etc).

One future possibility to keep in mind is the idea of workspace inheritance of sets/groups. These are sets of inheritable fields that you can opt-in on a named basis

[workspace.group.public.package]
rust-version = "1.60.0"
edition = "2018"

[workspace.group.internal.package]
inherits = "some-other-group"
rust-version = "1.73.0"
edition = "2021"

[package]
name = "foo"
rust-version.workspace = "internal"
edition.workspace = "internal"
inherits = "public"

[package]
name = "bar"
jplatte commented 9 months ago

Maybe inherits = "lints" could be a thing too? … and a more conservative default for the upcoming edition, if other fields inheriting by default is controversial.

jplatte commented 8 months ago

After re-reading, I had an idea for how use cases that include example crates could benefit without an edition bump: Treat lints.workspace = true outside a workspace as a warning, not an error (and act as if the lints table was absent). I feel like this shouldn't be hard to generalize to any field that can be inherited from the workspace, and from my POV combines well with .workspace = false acting as a way of explicitly resetting a field to its default (possibly null) value.

epage commented 8 months ago

For myself, I dislike downgrading lints.workspace = true from an error to a warning as we are then running counter to what the user said. I don't feel like the example use case is large enough to justify that type of sloppiness.