fix(toml): Add default-features to TomlWorkspaceDependency

[Muscraft]

In #11329 it was noted that default-features is ignored when used in a dependency that inherits from a workspace i.e.

[workspace]
members = []
[workspace.dependencies]
dep = "0.1"

[package]
name = "bar"
version = "0.2.0"
authors = []
[dependencies]
dep = { workspace = true, default-features = false }

This problem is caused by problems with deserializing a TomlDependency and not emitting an unused manifest key correctly. When discussed in a recent Cargo team meeting we felt the best course of action was to allow default-features = false when inheriting a dependency, but it does not change actually set default-features. It will be used to warn when there is a difference between the definition in [workspace.dependencies] and [dependencies] i.e.

[package]
name = "bar"
version = "0.2.0"
[dependencies]
dep = { workspace = true, default-features = false }

[workspace]
members = []
[workspace.dependencies]
dep = { version = "0.1", default-features = true }

This does not entirely resolve the problem with unused manifest keys. A follow up PR and issue will be created to address those concerns.

close #11329

[rustbot]

r? @ehuss

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

[ehuss]

I'm overall a bit uncertain about the behaviors here, so I'm going to write out what my expectations would be:

workspace: foo = { version="1.0", default-features = false }
member: foo = { workspace = true }
expectation: The foo dependency should have default-features disabled. There should not be any warning, as this was the expected syntax that was decided in the RFC, and is generally the natural way to write it. That is, the user wanted whatever was defined in the workspace.

workspace: foo = { version="1.0", default-features = false }
member: foo = { workspace = true, default-features = true }
expectation: The foo dependency should have default-features enabled. There should not be any warning. This allows different members to decide whether or not they have the default-features enabled. (If this is a warning/error, users could circumvent it with features=["default"] which seems unnecessary.)

workspace: foo = { version="1.0", default-features = false }
member: foo = { workspace = true, default-features = false }
expectation: The foo default-features should be disabled. I think it should be allowed without warning, it is just re-iterating the workspace value.

---

workspace: foo = { version="1.0", default-features = true } or foo = "1.0"
member: foo = { workspace = true }
expectation: The foo dependency should have default-features enabled. No warning/error.

workspace: foo = { version = "1.0", default-features = true } or foo = "1.0"
member: foo = { workspace = true, default-features = false }
expectation: This should have been an error, but since it was erroneously accepted in the past, it should be a warning that the default-features is ignored, and may be an error in the future.

workspace: foo = { version = "1.0", default-features = true } or foo = "1.0"
member: foo = { workspace = true, default-features = true }
expectation: This should have been an error, but since it was erroneously accepted in the past, I think we should just accept it without a warning/error. It is just re-iterating the default. It is also I think a bit unlikely (I don't see default-features = true done very often).

WDYT?

[ehuss]

Since this was previously accepted, I'm not sure this should be initially an error. What do you think about making it a warning that it may become an error in the future?

Also, I'm a bit confused why true would be an error, but false is a warning. Since default-features=true is the default, why would there be an error if someone specifies it?

[smmalis37]

I personally would agree with @ehuss on expectations, however it's worth noting that the RFC specifies different behavior currently:

For now if a workspace = true dependency is specified then also specifying the default-features value is disallowed. The default-features value for a directive is inherited from the [workspace.dependencies] declaration, which defaults to true if nothing else is specified."

I would very much like to have the ability to use the second case in @ehuss' list however, despite it being against the RFC as currently written.

[Muscraft]

I agree with what was talked about above. It is the best way forward with minimal changes to the expected behavior. I have implemented tests for the changed behavior. Let me know if you want me to add tests for the unchanged behaviors.

[ehuss]

Thanks, just a few style nits.

[ehuss]

I suggest using implicit arguments in format strings when possible, as it can make it a little clearer to read instead of needing to match the {} with the arguments.

Suggested change

	"`default-features` is ignored for {}, since `default-features` was {} for \
	`workspace.dependencies.{}`, this could become a hard error in the future",
	label, ws_def_feat, label
	"`default-features` is ignored for {label}, since `default-features` was \
	{ws_def_feat} for `workspace.dependencies.{label}`, \
	this could become a hard error in the future"

[ehuss]

I think this might be a little simpler or clearer:

Suggested change

	dep.default_features = default_features.or(default_features2);
	dep.default_features = Some(true);

[ehuss]

Can you switch to "check" in tests like this? build is a little slower and uses more disk space.

[ehuss]

BTW, when you are ready for a review, you can use @rustbot ready, and it will update the PR labels.

[ehuss]

@rust-lang/cargo This makes some changes to the way default-features=… is handled with workspace inheritance. Since this is a small change in behavior, and a small deviation from the RFC, I'm going to go ahead and do an FCP to run it by everyone. The table below should summarize the behavior.

Workspace	Member	Old Default	New Default	Reasoning
nothing	df=false	Enabled	Enabled, warning that it is ignored.	Should have been an error in the past since the field is ignored. May change to an error in the future.
df=true	df=false	Enabled	Enabled, warning	^Same
nothing	df=true	Enabled	Enabled	There is no conflict about default being enabled.
df=true	df=true	Enabled	Enabled	^Same
nothing	nothing	Enabled	Enabled	No ambiguity.
df=true	nothing	Enabled	Enabled	^Same
df=false	df=false	Disabled	Disabled	No ambiguity.
df=false	df=true	Disabled	💥Enabled	This allows members to decide whether or not they want default features. The workaround of features=["default"] seems unnecessary.
df=false	nothing	Disabled	Disabled	Natural way to write "give me whatever was written in workspace".

@rfcbot fcp merge

[rfcbot]

Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Eh2406
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[epage]

Wanted to make clear some points

• The deviation from the RFC is we allow the package to have default-features in the dependency
• We are maintaining the additive nature of the package dependency, it can enable default features but it can't disable them

With those, I think this makes sense and I'm fine with it.

As for the warning, at worse, it can be an error in the next edition. If we feel the impact is minimal, we can change it sooner.

[bors]

⌛ Testing commit 49d722c with merge 7e2a3c2...

[bors]

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 7e2a3c2 to master...