cargo icon indicating copy to clipboard operation
cargo copied to clipboard

Implicit Inheritance for Workspapce Inheritance

Open epage opened this issue 2 years ago • 6 comments

Problem

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

Use cases

  • Enable example packages to inherit optional fields while allowing the package to stand on its own (comment)

Proposed Solution

Possibilities:

  • Workspace directives could be implicitly and automatically inherited to members. In the future, however, Cargo will want to support nested workspaces, and it's unclear how these features will interact. In order to strik a reasonable middle-ground for now a simple solution which should address many use cases is proposed and we can continue to refine this over time as necessary.
  • Directives could be flagged to be explicitly inherited to workspace members as an optional way of specifying this. For now though to keep this proposal simple this is left out as a possible future extension of Cargo.

Notes

No response

epage avatar May 31 '23 15:05 epage

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.

epage avatar May 31 '23 15:05 epage

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.

Jasper-Bekkers avatar Nov 16 '23 16:11 Jasper-Bekkers

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.

epage avatar Nov 16 '23 17:11 epage

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.

Nemo157 avatar Nov 16 '23 17:11 Nemo157

@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.

  • Dependencies are a weird case. We don't want to automatically add them. I'm also slightly uncomfortable with dependencies without any source set.
  • I dislike that the naming of inherits is decoupled from .workspace = true but I don't have a better idea.
  • I did inherits instead of package.inherits to make it obvious it applies to everything, like cargo-features. However, this doesn't work well in the presence of workspaces

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"
  • Maybe it can be group.internal.package without workspace.
  • We could possibly support packages that inherit from (workspace member) packages, like profiles, but that might encourage unnecessary coupling and gets weird with dependencies.

epage avatar Nov 30 '23 22:11 epage

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 avatar Dec 05 '23 10:12 jplatte

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.

jplatte avatar Jan 03 '24 21:01 jplatte

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.

epage avatar Jan 03 '24 21:01 epage