Skip to content

Commit d34d0a1

Browse files
borsbors
committedMay 1, 2024
Auto merge of #13839 - epage:df_2024, r=Muscraft
fix(toml): On 2024 Edition, disallow ignored `default-features` when inheriting ### What does this PR try to resolve? This is part of rust-lang/rust#123754 This is a follow up to #11409 which tweaked how we do inheritance of default-features, including warning when `default-features = false` is ignored. This turns those warnings into an error. ### How should we test and review this PR? ### Additional information
·
home-0.5.110.81.0
2 parents 57d3248 + 627b1d1 commit d34d0a1

File tree

5 files changed

+401
-66
lines changed

5 files changed

+401
-66
lines changed
 

‎crates/cargo-util-schemas/src/manifest/mod.rs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -683,6 +683,13 @@ impl TomlDependency {
683683
}
684684
}
685685

686+
pub fn default_features(&self) -> Option<bool> {
687+
match self {
688+
TomlDependency::Detailed(d) => d.default_features(),
689+
TomlDependency::Simple(..) => None,
690+
}
691+
}
692+
686693
pub fn unused_keys(&self) -> Vec<String> {
687694
match self {
688695
TomlDependency::Simple(_) => vec![],

‎src/cargo/ops/fix.rs‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ use std::{env, fs, str};
4545

4646
use anyhow::{bail, Context as _};
4747
use cargo_util::{exit_status_to_string, is_simple_exit_code, paths, ProcessBuilder};
48+
use cargo_util_schemas::manifest::TomlManifest;
4849
use rustfix::diagnostics::Diagnostic;
4950
use rustfix::CodeFix;
5051
use semver::Version;
@@ -265,6 +266,10 @@ fn migrate_manifests(ws: &Workspace<'_>, pkgs: &[&Package]) -> CargoResult<()> {
265266
format!("{file} from {existing_edition} edition to {prepare_for_edition}"),
266267
)?;
267268

269+
let ws_original_toml = match ws.root_maybe() {
270+
MaybePackage::Package(package) => package.manifest().original_toml(),
271+
MaybePackage::Virtual(manifest) => manifest.original_toml(),
272+
};
268273
if Edition::Edition2024 <= prepare_for_edition {
269274
let mut document = pkg.manifest().document().clone().into_mut();
270275
let mut fixes = 0;
@@ -290,10 +295,15 @@ fn migrate_manifests(ws: &Workspace<'_>, pkgs: &[&Package]) -> CargoResult<()> {
290295
fixes += rename_array_of_target_fields_2024(root, "test");
291296
fixes += rename_array_of_target_fields_2024(root, "bench");
292297
fixes += rename_dep_fields_2024(root, "dependencies");
298+
fixes += remove_ignored_default_features_2024(root, "dependencies", ws_original_toml);
293299
fixes += rename_table(root, "dev_dependencies", "dev-dependencies");
294300
fixes += rename_dep_fields_2024(root, "dev-dependencies");
301+
fixes +=
302+
remove_ignored_default_features_2024(root, "dev-dependencies", ws_original_toml);
295303
fixes += rename_table(root, "build_dependencies", "build-dependencies");
296304
fixes += rename_dep_fields_2024(root, "build-dependencies");
305+
fixes +=
306+
remove_ignored_default_features_2024(root, "build-dependencies", ws_original_toml);
297307
for target in root
298308
.get_mut("target")
299309
.and_then(|t| t.as_table_like_mut())
@@ -302,10 +312,22 @@ fn migrate_manifests(ws: &Workspace<'_>, pkgs: &[&Package]) -> CargoResult<()> {
302312
.filter_map(|(_k, t)| t.as_table_like_mut())
303313
{
304314
fixes += rename_dep_fields_2024(target, "dependencies");
315+
fixes +=
316+
remove_ignored_default_features_2024(target, "dependencies", ws_original_toml);
305317
fixes += rename_table(target, "dev_dependencies", "dev-dependencies");
306318
fixes += rename_dep_fields_2024(target, "dev-dependencies");
319+
fixes += remove_ignored_default_features_2024(
320+
target,
321+
"dev-dependencies",
322+
ws_original_toml,
323+
);
307324
fixes += rename_table(target, "build_dependencies", "build-dependencies");
308325
fixes += rename_dep_fields_2024(target, "build-dependencies");
326+
fixes += remove_ignored_default_features_2024(
327+
target,
328+
"build-dependencies",
329+
ws_original_toml,
330+
);
309331
}
310332

311333
if 0 < fixes {
@@ -337,6 +359,47 @@ fn rename_dep_fields_2024(parent: &mut dyn toml_edit::TableLike, dep_kind: &str)
337359
fixes
338360
}
339361

362+
fn remove_ignored_default_features_2024(
363+
parent: &mut dyn toml_edit::TableLike,
364+
dep_kind: &str,
365+
ws_original_toml: &TomlManifest,
366+
) -> usize {
367+
let mut fixes = 0;
368+
for (name_in_toml, target) in parent
369+
.get_mut(dep_kind)
370+
.and_then(|t| t.as_table_like_mut())
371+
.iter_mut()
372+
.flat_map(|t| t.iter_mut())
373+
.filter_map(|(k, t)| t.as_table_like_mut().map(|t| (k, t)))
374+
{
375+
let name_in_toml: &str = &name_in_toml;
376+
let ws_deps = ws_original_toml
377+
.workspace
378+
.as_ref()
379+
.and_then(|ws| ws.dependencies.as_ref());
380+
if let Some(ws_dep) = ws_deps.and_then(|ws_deps| ws_deps.get(name_in_toml)) {
381+
if ws_dep.default_features() == Some(false) {
382+
continue;
383+
}
384+
}
385+
if target
386+
.get("workspace")
387+
.and_then(|i| i.as_value())
388+
.and_then(|i| i.as_bool())
389+
== Some(true)
390+
&& target
391+
.get("default-features")
392+
.and_then(|i| i.as_value())
393+
.and_then(|i| i.as_bool())
394+
== Some(false)
395+
{
396+
target.remove("default-features");
397+
fixes += 1;
398+
}
399+
}
400+
fixes
401+
}
402+
340403
fn rename_array_of_target_fields_2024(root: &mut dyn toml_edit::TableLike, kind: &str) -> usize {
341404
let mut fixes = 0;
342405
for target in root

‎src/cargo/util/toml/mod.rs‎

Lines changed: 84 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -692,8 +692,14 @@ fn resolve_dependencies<'a>(
692692

693693
let mut deps = BTreeMap::new();
694694
for (name_in_toml, v) in dependencies.iter() {
695-
let mut resolved =
696-
dependency_inherit_with(v.clone(), name_in_toml, inherit, package_root, warnings)?;
695+
let mut resolved = dependency_inherit_with(
696+
v.clone(),
697+
name_in_toml,
698+
inherit,
699+
package_root,
700+
edition,
701+
warnings,
702+
)?;
697703
if let manifest::TomlDependency::Detailed(ref mut d) = resolved {
698704
deprecated_underscore(
699705
&d.default_features2,
@@ -949,12 +955,13 @@ fn dependency_inherit_with<'a>(
949955
name: &str,
950956
inherit: &dyn Fn() -> CargoResult<&'a InheritableFields>,
951957
package_root: &Path,
958+
edition: Edition,
952959
warnings: &mut Vec<String>,
953960
) -> CargoResult<manifest::TomlDependency> {
954961
match dependency {
955962
manifest::InheritableDependency::Value(value) => Ok(value),
956963
manifest::InheritableDependency::Inherit(w) => {
957-
inner_dependency_inherit_with(w, name, inherit, package_root, warnings).with_context(|| {
964+
inner_dependency_inherit_with(w, name, inherit, package_root, edition, warnings).with_context(|| {
958965
format!(
959966
"error inheriting `{name}` from workspace root manifest's `workspace.dependencies.{name}`",
960967
)
@@ -968,76 +975,87 @@ fn inner_dependency_inherit_with<'a>(
968975
name: &str,
969976
inherit: &dyn Fn() -> CargoResult<&'a InheritableFields>,
970977
package_root: &Path,
978+
edition: Edition,
971979
warnings: &mut Vec<String>,
972980
) -> CargoResult<manifest::TomlDependency> {
973-
fn default_features_msg(label: &str, ws_def_feat: Option<bool>, warnings: &mut Vec<String>) {
974-
let ws_def_feat = match ws_def_feat {
975-
Some(true) => "true",
976-
Some(false) => "false",
977-
None => "not specified",
978-
};
981+
let ws_dep = inherit()?.get_dependency(name, package_root)?;
982+
let mut merged_dep = match ws_dep {
983+
manifest::TomlDependency::Simple(ws_version) => manifest::TomlDetailedDependency {
984+
version: Some(ws_version),
985+
..Default::default()
986+
},
987+
manifest::TomlDependency::Detailed(ws_dep) => ws_dep.clone(),
988+
};
989+
let manifest::TomlInheritedDependency {
990+
workspace: _,
991+
992+
features,
993+
optional,
994+
default_features,
995+
default_features2,
996+
public,
997+
998+
_unused_keys: _,
999+
} = &pkg_dep;
1000+
let default_features = default_features.or(*default_features2);
1001+
1002+
match (default_features, merged_dep.default_features()) {
1003+
// member: default-features = true and
1004+
// workspace: default-features = false should turn on
1005+
// default-features
1006+
(Some(true), Some(false)) => {
1007+
merged_dep.default_features = Some(true);
1008+
}
1009+
// member: default-features = false and
1010+
// workspace: default-features = true should ignore member
1011+
// default-features
1012+
(Some(false), Some(true)) => {
1013+
deprecated_ws_default_features(name, Some(true), edition, warnings)?;
1014+
}
1015+
// member: default-features = false and
1016+
// workspace: dep = "1.0" should ignore member default-features
1017+
(Some(false), None) => {
1018+
deprecated_ws_default_features(name, None, edition, warnings)?;
1019+
}
1020+
_ => {}
1021+
}
1022+
merged_dep.features = match (merged_dep.features.clone(), features.clone()) {
1023+
(Some(dep_feat), Some(inherit_feat)) => Some(
1024+
dep_feat
1025+
.into_iter()
1026+
.chain(inherit_feat)
1027+
.collect::<Vec<String>>(),
1028+
),
1029+
(Some(dep_fet), None) => Some(dep_fet),
1030+
(None, Some(inherit_feat)) => Some(inherit_feat),
1031+
(None, None) => None,
1032+
};
1033+
merged_dep.optional = *optional;
1034+
merged_dep.public = *public;
1035+
Ok(manifest::TomlDependency::Detailed(merged_dep))
1036+
}
1037+
1038+
fn deprecated_ws_default_features(
1039+
label: &str,
1040+
ws_def_feat: Option<bool>,
1041+
edition: Edition,
1042+
warnings: &mut Vec<String>,
1043+
) -> CargoResult<()> {
1044+
let ws_def_feat = match ws_def_feat {
1045+
Some(true) => "true",
1046+
Some(false) => "false",
1047+
None => "not specified",
1048+
};
1049+
if Edition::Edition2024 <= edition {
1050+
anyhow::bail!("`default-features = false` cannot override workspace's `default-features`");
1051+
} else {
9791052
warnings.push(format!(
9801053
"`default-features` is ignored for {label}, since `default-features` was \
9811054
{ws_def_feat} for `workspace.dependencies.{label}`, \
9821055
this could become a hard error in the future"
983-
))
1056+
));
9841057
}
985-
inherit()?.get_dependency(name, package_root).map(|ws_dep| {
986-
let mut merged_dep = match ws_dep {
987-
manifest::TomlDependency::Simple(ws_version) => manifest::TomlDetailedDependency {
988-
version: Some(ws_version),
989-
..Default::default()
990-
},
991-
manifest::TomlDependency::Detailed(ws_dep) => ws_dep.clone(),
992-
};
993-
let manifest::TomlInheritedDependency {
994-
workspace: _,
995-
996-
features,
997-
optional,
998-
default_features,
999-
default_features2,
1000-
public,
1001-
1002-
_unused_keys: _,
1003-
} = &pkg_dep;
1004-
let default_features = default_features.or(*default_features2);
1005-
1006-
match (default_features, merged_dep.default_features()) {
1007-
// member: default-features = true and
1008-
// workspace: default-features = false should turn on
1009-
// default-features
1010-
(Some(true), Some(false)) => {
1011-
merged_dep.default_features = Some(true);
1012-
}
1013-
// member: default-features = false and
1014-
// workspace: default-features = true should ignore member
1015-
// default-features
1016-
(Some(false), Some(true)) => {
1017-
default_features_msg(name, Some(true), warnings);
1018-
}
1019-
// member: default-features = false and
1020-
// workspace: dep = "1.0" should ignore member default-features
1021-
(Some(false), None) => {
1022-
default_features_msg(name, None, warnings);
1023-
}
1024-
_ => {}
1025-
}
1026-
merged_dep.features = match (merged_dep.features.clone(), features.clone()) {
1027-
(Some(dep_feat), Some(inherit_feat)) => Some(
1028-
dep_feat
1029-
.into_iter()
1030-
.chain(inherit_feat)
1031-
.collect::<Vec<String>>(),
1032-
),
1033-
(Some(dep_fet), None) => Some(dep_fet),
1034-
(None, Some(inherit_feat)) => Some(inherit_feat),
1035-
(None, None) => None,
1036-
};
1037-
merged_dep.optional = *optional;
1038-
merged_dep.public = *public;
1039-
manifest::TomlDependency::Detailed(merged_dep)
1040-
})
1058+
Ok(())
10411059
}
10421060

10431061
#[tracing::instrument(skip_all)]

‎tests/testsuite/fix.rs‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2654,3 +2654,150 @@ baz = ["dep:baz"]
26542654
"#
26552655
);
26562656
}
2657+
2658+
#[cargo_test]
2659+
fn remove_ignored_default_features() {
2660+
Package::new("dep_simple", "0.1.0").publish();
2661+
Package::new("dep_df_true", "0.1.0").publish();
2662+
Package::new("dep_df_false", "0.1.0").publish();
2663+
2664+
let pkg_default = r#"
2665+
[package]
2666+
name = "pkg_default"
2667+
version = "0.1.0"
2668+
edition = "2021"
2669+
2670+
[dependencies]
2671+
dep_simple = { workspace = true }
2672+
dep_df_true = { workspace = true }
2673+
dep_df_false = { workspace = true }
2674+
2675+
[build-dependencies]
2676+
dep_simple = { workspace = true }
2677+
dep_df_true = { workspace = true }
2678+
dep_df_false = { workspace = true }
2679+
2680+
[target.'cfg(target_os = "linux")'.dependencies]
2681+
dep_simple = { workspace = true }
2682+
dep_df_true = { workspace = true }
2683+
dep_df_false = { workspace = true }
2684+
"#;
2685+
let pkg_df_true = r#"
2686+
[package]
2687+
name = "pkg_df_true"
2688+
version = "0.1.0"
2689+
edition = "2021"
2690+
2691+
[dependencies]
2692+
dep_simple = { workspace = true, default-features = true }
2693+
dep_df_true = { workspace = true, default-features = true }
2694+
dep_df_false = { workspace = true, default-features = true }
2695+
2696+
[build-dependencies]
2697+
dep_simple = { workspace = true, default-features = true }
2698+
dep_df_true = { workspace = true, default-features = true }
2699+
dep_df_false = { workspace = true, default-features = true }
2700+
2701+
[target.'cfg(target_os = "linux")'.dependencies]
2702+
dep_simple = { workspace = true, default-features = true }
2703+
dep_df_true = { workspace = true, default-features = true }
2704+
dep_df_false = { workspace = true, default-features = true }
2705+
"#;
2706+
let pkg_df_false = r#"
2707+
[package]
2708+
name = "pkg_df_false"
2709+
version = "0.1.0"
2710+
edition = "2021"
2711+
2712+
[dependencies]
2713+
dep_simple = { workspace = true, default-features = false }
2714+
dep_df_true = { workspace = true, default-features = false }
2715+
dep_df_false = { workspace = true, default-features = false }
2716+
2717+
[build-dependencies]
2718+
dep_simple = { workspace = true, default-features = false }
2719+
dep_df_true = { workspace = true, default-features = false }
2720+
dep_df_false = { workspace = true, default-features = false }
2721+
2722+
[target.'cfg(target_os = "linux")'.dependencies]
2723+
dep_simple = { workspace = true, default-features = false }
2724+
dep_df_true = { workspace = true, default-features = false }
2725+
dep_df_false = { workspace = true, default-features = false }
2726+
"#;
2727+
let p = project()
2728+
.file(
2729+
"Cargo.toml",
2730+
r#"
2731+
[workspace]
2732+
members = ["pkg_default", "pkg_df_true", "pkg_df_false"]
2733+
resolver = "2"
2734+
2735+
[workspace.dependencies]
2736+
dep_simple = "0.1.0"
2737+
dep_df_true = { version = "0.1.0", default-features = true }
2738+
dep_df_false = { version = "0.1.0", default-features = false }
2739+
"#,
2740+
)
2741+
.file("pkg_default/Cargo.toml", pkg_default)
2742+
.file("pkg_default/src/lib.rs", "")
2743+
.file("pkg_df_true/Cargo.toml", pkg_df_true)
2744+
.file("pkg_df_true/src/lib.rs", "")
2745+
.file("pkg_df_false/Cargo.toml", pkg_df_false)
2746+
.file("pkg_df_false/src/lib.rs", "")
2747+
.build();
2748+
2749+
p.cargo("fix --all --edition --allow-no-vcs")
2750+
.masquerade_as_nightly_cargo(&["edition2024"])
2751+
.with_stderr_unordered(
2752+
"\
2753+
[MIGRATING] pkg_default/Cargo.toml from 2021 edition to 2024
2754+
[MIGRATING] pkg_df_true/Cargo.toml from 2021 edition to 2024
2755+
[MIGRATING] pkg_df_false/Cargo.toml from 2021 edition to 2024
2756+
[FIXED] pkg_df_false/Cargo.toml (6 fixes)
2757+
[UPDATING] `dummy-registry` index
2758+
[LOCKING] 6 packages to latest compatible versions
2759+
[DOWNLOADING] crates ...
2760+
[DOWNLOADED] dep_simple v0.1.0 (registry `dummy-registry`)
2761+
[DOWNLOADED] dep_df_true v0.1.0 (registry `dummy-registry`)
2762+
[DOWNLOADED] dep_df_false v0.1.0 (registry `dummy-registry`)
2763+
[CHECKING] dep_df_true v0.1.0
2764+
[CHECKING] dep_df_false v0.1.0
2765+
[CHECKING] dep_simple v0.1.0
2766+
[CHECKING] pkg_df_true v0.1.0 ([CWD]/pkg_df_true)
2767+
[CHECKING] pkg_df_false v0.1.0 ([CWD]/pkg_df_false)
2768+
[CHECKING] pkg_default v0.1.0 ([CWD]/pkg_default)
2769+
[MIGRATING] pkg_df_false/src/lib.rs from 2021 edition to 2024
2770+
[MIGRATING] pkg_df_true/src/lib.rs from 2021 edition to 2024
2771+
[MIGRATING] pkg_default/src/lib.rs from 2021 edition to 2024
2772+
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]s
2773+
",
2774+
)
2775+
.run();
2776+
2777+
assert_eq!(p.read_file("pkg_default/Cargo.toml"), pkg_default);
2778+
assert_eq!(p.read_file("pkg_df_true/Cargo.toml"), pkg_df_true);
2779+
assert_eq!(
2780+
p.read_file("pkg_df_false/Cargo.toml"),
2781+
r#"
2782+
[package]
2783+
name = "pkg_df_false"
2784+
version = "0.1.0"
2785+
edition = "2021"
2786+
2787+
[dependencies]
2788+
dep_simple = { workspace = true}
2789+
dep_df_true = { workspace = true}
2790+
dep_df_false = { workspace = true, default-features = false }
2791+
2792+
[build-dependencies]
2793+
dep_simple = { workspace = true}
2794+
dep_df_true = { workspace = true}
2795+
dep_df_false = { workspace = true, default-features = false }
2796+
2797+
[target.'cfg(target_os = "linux")'.dependencies]
2798+
dep_simple = { workspace = true}
2799+
dep_df_true = { workspace = true}
2800+
dep_df_false = { workspace = true, default-features = false }
2801+
"#
2802+
);
2803+
}

‎tests/testsuite/inheritable_workspace_fields.rs‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1518,6 +1518,56 @@ true for `workspace.dependencies.dep`, this could become a hard error in the fut
15181518
.run();
15191519
}
15201520

1521+
#[cargo_test(nightly, reason = "edition2024 is not stable")]
1522+
fn warn_inherit_def_feat_true_member_def_feat_false_2024_edition() {
1523+
Package::new("dep", "0.1.0")
1524+
.feature("default", &["fancy_dep"])
1525+
.add_dep(Dependency::new("fancy_dep", "0.2").optional(true))
1526+
.file("src/lib.rs", "")
1527+
.publish();
1528+
1529+
Package::new("fancy_dep", "0.2.4").publish();
1530+
1531+
let p = project()
1532+
.file(
1533+
"Cargo.toml",
1534+
r#"
1535+
cargo-features = ["edition2024"]
1536+
1537+
[package]
1538+
name = "bar"
1539+
version = "0.2.0"
1540+
edition = "2024"
1541+
authors = []
1542+
[dependencies]
1543+
dep = { workspace = true, default-features = false }
1544+
1545+
[workspace]
1546+
members = []
1547+
[workspace.dependencies]
1548+
dep = { version = "0.1.0", default-features = true }
1549+
"#,
1550+
)
1551+
.file("src/main.rs", "fn main() {}")
1552+
.build();
1553+
1554+
p.cargo("check")
1555+
.masquerade_as_nightly_cargo(&["edition2024"])
1556+
.with_status(101)
1557+
.with_stderr(
1558+
"\
1559+
[ERROR] failed to parse manifest at `[CWD]/Cargo.toml`
1560+
1561+
Caused by:
1562+
error inheriting `dep` from workspace root manifest's `workspace.dependencies.dep`
1563+
1564+
Caused by:
1565+
`default-features = false` cannot override workspace's `default-features`
1566+
",
1567+
)
1568+
.run();
1569+
}
1570+
15211571
#[cargo_test]
15221572
fn warn_inherit_simple_member_def_feat_false() {
15231573
Package::new("dep", "0.1.0")
@@ -1568,6 +1618,56 @@ not specified for `workspace.dependencies.dep`, this could become a hard error i
15681618
.run();
15691619
}
15701620

1621+
#[cargo_test(nightly, reason = "edition2024 is not stable")]
1622+
fn warn_inherit_simple_member_def_feat_false_2024_edition() {
1623+
Package::new("dep", "0.1.0")
1624+
.feature("default", &["fancy_dep"])
1625+
.add_dep(Dependency::new("fancy_dep", "0.2").optional(true))
1626+
.file("src/lib.rs", "")
1627+
.publish();
1628+
1629+
Package::new("fancy_dep", "0.2.4").publish();
1630+
1631+
let p = project()
1632+
.file(
1633+
"Cargo.toml",
1634+
r#"
1635+
cargo-features = ["edition2024"]
1636+
1637+
[package]
1638+
name = "bar"
1639+
version = "0.2.0"
1640+
edition = "2024"
1641+
authors = []
1642+
[dependencies]
1643+
dep = { workspace = true, default-features = false }
1644+
1645+
[workspace]
1646+
members = []
1647+
[workspace.dependencies]
1648+
dep = "0.1.0"
1649+
"#,
1650+
)
1651+
.file("src/main.rs", "fn main() {}")
1652+
.build();
1653+
1654+
p.cargo("check")
1655+
.masquerade_as_nightly_cargo(&["edition2024"])
1656+
.with_status(101)
1657+
.with_stderr(
1658+
"\
1659+
[ERROR] failed to parse manifest at `[CWD]/Cargo.toml`
1660+
1661+
Caused by:
1662+
error inheriting `dep` from workspace root manifest's `workspace.dependencies.dep`
1663+
1664+
Caused by:
1665+
`default-features = false` cannot override workspace's `default-features`
1666+
",
1667+
)
1668+
.run();
1669+
}
1670+
15711671
#[cargo_test]
15721672
fn inherit_def_feat_false_member_def_feat_true() {
15731673
Package::new("dep", "0.1.0")

0 commit comments

Comments
 (0)
Please sign in to comment.