Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(toml): Error on [project] in Edition 2024 #13747

Merged
merged 10 commits into from
Apr 16, 2024
7 changes: 6 additions & 1 deletion src/bin/cargo/commands/fix.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::command_prelude::*;

use cargo::core::Workspace;
use cargo::ops;

pub fn cli() -> Command {
Expand Down Expand Up @@ -60,7 +61,6 @@ pub fn cli() -> Command {
}

pub fn exec(gctx: &mut GlobalContext, args: &ArgMatches) -> CliResult {
let ws = args.workspace(gctx)?;
// This is a legacy behavior that causes `cargo fix` to pass `--test`.
let test = matches!(
args.get_one::<String>("profile").map(String::as_str),
Expand All @@ -70,6 +70,9 @@ pub fn exec(gctx: &mut GlobalContext, args: &ArgMatches) -> CliResult {

// Unlike other commands default `cargo fix` to all targets to fix as much
// code as we can.
let root_manifest = args.root_manifest(gctx)?;
epage marked this conversation as resolved.
Show resolved Hide resolved
let mut ws = Workspace::new(&root_manifest, gctx)?;
ws.set_honor_rust_version(args.honor_rust_version());
let mut opts = args.compile_options(gctx, mode, Some(&ws), ProfileChecking::LegacyTestOnly)?;

if !opts.filter.is_specific() {
Expand All @@ -78,7 +81,9 @@ pub fn exec(gctx: &mut GlobalContext, args: &ArgMatches) -> CliResult {
}

ops::fix(
gctx,
&ws,
&root_manifest,
&mut ops::FixOptions {
edition: args.flag("edition"),
idioms: args.flag("edition-idioms"),
Expand Down
4 changes: 4 additions & 0 deletions src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,10 @@ impl<'gctx> Workspace<'gctx> {
self.honor_rust_version = honor_rust_version;
}

pub fn honor_rust_version(&self) -> Option<bool> {
self.honor_rust_version
}

pub fn resolve_honors_rust_version(&self) -> bool {
self.gctx().cli_unstable().msrv_policy && self.honor_rust_version.unwrap_or(true)
}
Expand Down
84 changes: 78 additions & 6 deletions src/cargo/ops/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ use tracing::{debug, trace, warn};
use crate::core::compiler::RustcTargetData;
use crate::core::resolver::features::{DiffMap, FeatureOpts, FeatureResolver, FeaturesFor};
use crate::core::resolver::{HasDevUnits, Resolve, ResolveBehavior};
use crate::core::{Edition, MaybePackage, PackageId, Workspace};
use crate::core::PackageIdSpecQuery as _;
use crate::core::{Edition, MaybePackage, Package, PackageId, Workspace};
use crate::ops::resolve::WorkspaceResolve;
use crate::ops::{self, CompileOptions};
use crate::util::diagnostic_server::{Message, RustfixDiagnosticServer};
Expand Down Expand Up @@ -87,11 +88,26 @@ pub struct FixOptions {
pub broken_code: bool,
}

pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions) -> CargoResult<()> {
check_version_control(ws.gctx(), opts)?;
pub fn fix(
gctx: &GlobalContext,
original_ws: &Workspace<'_>,
root_manifest: &Path,
opts: &mut FixOptions,
) -> CargoResult<()> {
check_version_control(gctx, opts)?;

if opts.edition {
check_resolver_change(ws, opts)?;
let specs = opts.compile_opts.spec.to_package_id_specs(&original_ws)?;
let members: Vec<&Package> = original_ws
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, there seems to be some problem here, it doesn't fetch all members of the workspace. If the members contain [project], then cargo fix --edition won't complete the migration either.

Is this intentional, or am I misinterpreting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do filter by the --package flag which is us just doing what the user told us. I'm not seeing how presence of [proiject] causes a problem. We also have a test showing that it works.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't seem to have been clear.

What I meant was that if the members in the workspace use [project] instead of [package], then executing the command cargo fix --edition in the workspace does not modify the members.

I'm wondering if the changes should be done on all members at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm still missing the problem case.

Could you create a test case to demonstrate it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a workspace ws containing the member hello.

which:

# [ws]/Cargo.toml
[workspace]
members = ["hello"]

[project]     # Pay attention here 
name = "ws"
version = "0.1.0"
edition = "2021"
# [ws]/hello/Cargo.toml
[project]     # Pay attention here 
name = "hello"
version = "0.1.0"
edition = "2021"

After running cargo fix --edition --allow-no-vcs in the workspace. The cargo.toml of ws completes the modification, but the cargo.toml of member hello does not.

I think both should be modified to [package] here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've written that up as a test case which is passing.

Yes, we aren't migrating hello's Cargo.toml but we also aren't migrating hellos src/lib.rs. Manifest and rust migration should be done hand-in-hand. The user has to opt-in to migrating hello with the --workspace flag. cargo fix --edition --workspace will migrate both packages, manifest and rust.

#[cargo_test]
fn migrate_project_to_package_ws() {
    let p = project()
        .file(
            "Cargo.toml",
            r#"
cargo-features = ["edition2024"]

[workspace]
members = ["hello"]

# Before project
[ project ] # After project header
# After project header line
name = "foo"
edition = "2021"
# After project table
"#,
        )
        .file("src/lib.rs", "")
        .file(
            "hello/Cargo.toml",
            r#"
cargo-features = ["edition2024"]

# Before project
[ project ] # After project header
# After project header line
name = "hello"
edition = "2021"
# After project table
"#,
        )
        .file("hello/src/lib.rs", "")
        .build();

    p.cargo("fix --edition --allow-no-vcs")
        .masquerade_as_nightly_cargo(&["edition2024"])
        .with_stderr(
            "\
[MIGRATING] Cargo.toml from 2021 edition to 2024
[FIXED] Cargo.toml (1 fix)
[WARNING] [CWD]/hello/Cargo.toml: `[project]` is deprecated in favor of `[package]`
[LOCKING] 2 packages to latest compatible versions
[CHECKING] foo v0.0.0 ([CWD])
[MIGRATING] src/lib.rs from 2021 edition to 2024
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]s
",
        )
        .run();
    assert_eq!(
        p.read_file("Cargo.toml"),
        r#"
cargo-features = ["edition2024"]

[workspace]
members = ["hello"]

# Before project
[ package ] # After project header
# After project header line
name = "foo"
edition = "2021"
# After project table
"#
    );
    assert_eq!(
        p.read_file("hello/Cargo.toml"),
        r#"
cargo-features = ["edition2024"]

# Before project
[ project ] # After project header
# After project header line
name = "hello"
edition = "2021"
# After project table
"#
    );
}

.members()
.filter(|m| specs.iter().any(|spec| spec.matches(m.package_id())))
.collect();
migrate_manifests(original_ws, &members)?;

check_resolver_change(&original_ws, opts)?;
}
let mut ws = Workspace::new(&root_manifest, gctx)?;
ws.set_honor_rust_version(original_ws.honor_rust_version());

// Spin up our lock server, which our subprocesses will use to synchronize fixes.
let lock_server = LockServer::new()?;
Expand Down Expand Up @@ -128,7 +144,7 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions) -> CargoResult<()> {
server.configure(&mut wrapper);
}

let rustc = ws.gctx().load_global_rustc(Some(ws))?;
let rustc = ws.gctx().load_global_rustc(Some(&ws))?;
wrapper.arg(&rustc.path);
// This is calling rustc in cargo fix-proxy-mode, so it also need to retry.
// The argfile handling are located at `FixArgs::from_args`.
Expand All @@ -138,7 +154,7 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions) -> CargoResult<()> {
// repeating build until there are no more changes to be applied
opts.compile_opts.build_config.primary_unit_rustc = Some(wrapper);

ops::compile(ws, &opts.compile_opts)?;
ops::compile(&ws, &opts.compile_opts)?;
Ok(())
}

Expand Down Expand Up @@ -215,6 +231,62 @@ fn check_version_control(gctx: &GlobalContext, opts: &FixOptions) -> CargoResult
);
}

fn migrate_manifests(ws: &Workspace<'_>, pkgs: &[&Package]) -> CargoResult<()> {
for pkg in pkgs {
let existing_edition = pkg.manifest().edition();
let prepare_for_edition = existing_edition.saturating_next();
if existing_edition == prepare_for_edition
|| (!prepare_for_edition.is_stable() && !ws.gctx().nightly_features_allowed)
{
continue;
}
let file = pkg.manifest_path();
let file = file.strip_prefix(ws.root()).unwrap_or(file);
let file = file.display();
ws.gctx().shell().status(
"Migrating",
format!("{file} from {existing_edition} edition to {prepare_for_edition}"),
)?;

if Edition::Edition2024 <= prepare_for_edition {
let mut document = pkg.manifest().document().clone().into_mut();
let mut fixes = 0;

let root = document.as_table_mut();
if rename_table(root, "project", "package") {
fixes += 1;
}

if 0 < fixes {
let verb = if fixes == 1 { "fix" } else { "fixes" };
let msg = format!("{file} ({fixes} {verb})");
ws.gctx().shell().status("Fixed", msg)?;

let s = document.to_string();
let new_contents_bytes = s.as_bytes();
cargo_util::paths::write_atomic(pkg.manifest_path(), new_contents_bytes)?;
}
}
}

Ok(())
}

fn rename_table(parent: &mut dyn toml_edit::TableLike, old: &str, new: &str) -> bool {
let Some(old_key) = parent.key(old).cloned() else {
return false;
};

let project = parent.remove(old).expect("returned early");
if !parent.contains_key(new) {
parent.insert(new, project);
let mut new_key = parent.key_mut(new).expect("just inserted");
*new_key.dotted_decor_mut() = old_key.dotted_decor().clone();
*new_key.leaf_decor_mut() = old_key.leaf_decor().clone();
}
true
}

fn check_resolver_change(ws: &Workspace<'_>, opts: &FixOptions) -> CargoResult<()> {
let root = ws.root_maybe();
match root {
Expand Down
33 changes: 13 additions & 20 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -935,26 +935,9 @@ fn to_real_manifest(
);
};

let original_package = match (&original_toml.package, &original_toml.project) {
(Some(_), Some(project)) => {
warnings.push(format!(
"manifest at `{}` contains both `project` and `package`, \
this could become a hard error in the future",
package_root.display()
));
project.clone()
}
(Some(package), None) => package.clone(),
(None, Some(project)) => {
warnings.push(format!(
"manifest at `{}` contains `[project]` instead of `[package]`, \
this could become a hard error in the future",
package_root.display()
));
project.clone()
}
(None, None) => bail!("no `package` section found"),
};
let original_package = original_toml
.package()
.ok_or_else(|| anyhow::format_err!("no `package` section found"))?;

let package_name = &original_package.name;
if package_name.contains(':') {
Expand Down Expand Up @@ -1044,6 +1027,16 @@ fn to_real_manifest(
)));
}

if original_toml.project.is_some() {
if Edition::Edition2024 <= edition {
anyhow::bail!(
"`[project]` is not supported as of the 2024 Edition, please use `[package]`"
);
} else {
warnings.push(format!("`[project]` is deprecated in favor of `[package]`"));
}
}

if resolved_package.metabuild.is_some() {
features.require(Feature::metabuild())?;
}
Expand Down
85 changes: 58 additions & 27 deletions tests/testsuite/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -979,6 +979,63 @@ fn rustc_workspace_wrapper_excludes_published_deps() {
.run();
}

#[cargo_test]
fn warn_manifest_with_project() {
let p = project()
.file(
"Cargo.toml",
r#"
[project]
name = "foo"
version = "0.0.1"
edition = "2015"
"#,
)
.file("src/main.rs", "fn main() {}")
.build();

p.cargo("check")
.with_stderr(
"\
[WARNING] `[project]` is deprecated in favor of `[package]`
[CHECKING] foo v0.0.1 ([CWD])
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]
",
)
.run();
}

#[cargo_test(nightly, reason = "edition2024")]
epage marked this conversation as resolved.
Show resolved Hide resolved
fn error_manifest_with_project_on_2024() {
let p = project()
.file(
"Cargo.toml",
r#"
cargo-features = ["edition2024"]

[project]
name = "foo"
version = "0.0.1"
edition = "2024"
"#,
)
.file("src/main.rs", "fn main() {}")
.build();

p.cargo("check")
.masquerade_as_nightly_cargo(&["edition2024"])
.with_status(101)
.with_stderr(
"\
[ERROR] failed to parse manifest at `[CWD]/Cargo.toml`

Caused by:
`[project]` is not supported as of the 2024 Edition, please use `[package]`
",
)
.run();
}

#[cargo_test]
fn warn_manifest_package_and_project() {
let p = project()
Expand All @@ -1002,7 +1059,7 @@ fn warn_manifest_package_and_project() {
p.cargo("check")
.with_stderr(
"\
[WARNING] manifest at `[CWD]` contains both `project` and `package`, this could become a hard error in the future
[WARNING] `[project]` is deprecated in favor of `[package]`
[CHECKING] foo v0.0.1 ([CWD])
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]
",
Expand Down Expand Up @@ -1065,32 +1122,6 @@ fn git_manifest_package_and_project() {
.run();
}

#[cargo_test]
fn warn_manifest_with_project() {
let p = project()
.file(
"Cargo.toml",
r#"
[project]
name = "foo"
version = "0.0.1"
edition = "2015"
"#,
)
.file("src/main.rs", "fn main() {}")
.build();

p.cargo("check")
.with_stderr(
"\
[WARNING] manifest at `[CWD]` contains `[project]` instead of `[package]`, this could become a hard error in the future
[CHECKING] foo v0.0.1 ([CWD])
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]
",
)
.run();
}

#[cargo_test]
fn git_manifest_with_project() {
let p = project();
Expand Down
Loading