Skip to content

Add report if cargo fix --edition changes features. #9268

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

Merged
merged 2 commits into from
Mar 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cargo/core/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ pub use self::resolver::{Resolve, ResolveVersion};
pub use self::shell::{Shell, Verbosity};
pub use self::source::{GitReference, Source, SourceId, SourceMap};
pub use self::summary::{FeatureMap, FeatureValue, Summary};
pub use self::workspace::{Members, Workspace, WorkspaceConfig, WorkspaceRootConfig};
pub use self::workspace::{MaybePackage, Members, Workspace, WorkspaceConfig, WorkspaceRootConfig};

pub mod compiler;
pub mod dependency;
Expand Down
84 changes: 78 additions & 6 deletions src/cargo/core/resolver/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ use crate::core::resolver::{Resolve, ResolveBehavior};
use crate::core::{FeatureValue, PackageId, PackageIdSpec, PackageSet, Workspace};
use crate::util::interning::InternedString;
use crate::util::CargoResult;
use std::collections::{BTreeSet, HashMap, HashSet};
use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
use std::rc::Rc;

/// Map of activated features.
Expand Down Expand Up @@ -71,7 +71,7 @@ pub struct ResolvedFeatures {

/// Options for how the feature resolver works.
#[derive(Default)]
struct FeatureOpts {
pub struct FeatureOpts {
/// Use the new resolver instead of the old one.
new_resolver: bool,
/// Build deps and proc-macros will not share share features with other dep kinds.
Expand Down Expand Up @@ -123,7 +123,7 @@ impl FeaturesFor {
}

impl FeatureOpts {
fn new(
pub fn new(
ws: &Workspace<'_>,
has_dev_units: HasDevUnits,
force_all_targets: ForceAllTargets,
Expand Down Expand Up @@ -180,6 +180,20 @@ impl FeatureOpts {
}
Ok(opts)
}

/// Creates a new FeatureOpts for the given behavior.
pub fn new_behavior(behavior: ResolveBehavior, has_dev_units: HasDevUnits) -> FeatureOpts {
match behavior {
ResolveBehavior::V1 => FeatureOpts::default(),
ResolveBehavior::V2 => FeatureOpts {
new_resolver: true,
decouple_host_deps: true,
decouple_dev_deps: has_dev_units == HasDevUnits::No,
ignore_inactive_targets: true,
compare: false,
},
}
}
}

/// Features flags requested for a package.
Expand Down Expand Up @@ -286,6 +300,66 @@ impl ResolvedFeatures {
}
}
}

/// Compares the result against the original resolver behavior.
///
/// Used by `cargo fix --edition` to display any differences.
pub fn compare_legacy(&self, legacy: &ResolvedFeatures) -> FeatureDifferences {
let legacy_features = legacy.legacy_features.as_ref().unwrap();
let features = self
.activated_features
.iter()
.filter_map(|((pkg_id, for_host), new_features)| {
let old_features = match legacy_features.get(pkg_id) {
Some(feats) => feats.iter().cloned().collect(),
None => BTreeSet::new(),
};
// The new resolver should never add features.
assert_eq!(new_features.difference(&old_features).next(), None);
let removed_features: BTreeSet<_> =
old_features.difference(&new_features).cloned().collect();
if removed_features.is_empty() {
None
} else {
Some(((*pkg_id, *for_host), removed_features))
}
})
.collect();
let legacy_deps = legacy.legacy_dependencies.as_ref().unwrap();
let optional_deps = self
.activated_dependencies
.iter()
.filter_map(|((pkg_id, for_host), new_deps)| {
let old_deps = match legacy_deps.get(pkg_id) {
Some(deps) => deps.iter().cloned().collect(),
None => BTreeSet::new(),
};
// The new resolver should never add dependencies.
assert_eq!(new_deps.difference(&old_deps).next(), None);
let removed_deps: BTreeSet<_> = old_deps.difference(&new_deps).cloned().collect();
if removed_deps.is_empty() {
None
} else {
Some(((*pkg_id, *for_host), removed_deps))
}
})
.collect();
FeatureDifferences {
features,
optional_deps,
}
}
}

/// Map of differences.
///
/// Key is `(pkg_id, for_host)`. Value is a set of features or dependencies removed.
pub type DiffMap = BTreeMap<(PackageId, bool), BTreeSet<InternedString>>;

/// Differences between resolvers.
pub struct FeatureDifferences {
pub features: DiffMap,
pub optional_deps: DiffMap,
}

pub struct FeatureResolver<'a, 'cfg> {
Expand Down Expand Up @@ -334,13 +408,11 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
requested_features: &RequestedFeatures,
specs: &[PackageIdSpec],
requested_targets: &[CompileKind],
has_dev_units: HasDevUnits,
force_all_targets: ForceAllTargets,
opts: FeatureOpts,
) -> CargoResult<ResolvedFeatures> {
use crate::util::profile;
let _p = profile::start("resolve features");

let opts = FeatureOpts::new(ws, has_dev_units, force_all_targets)?;
if !opts.new_resolver {
// Legacy mode.
return Ok(ResolvedFeatures {
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ struct Packages<'cfg> {
}

#[derive(Debug)]
enum MaybePackage {
pub enum MaybePackage {
Package(Package),
Virtual(VirtualManifest),
}
Expand Down Expand Up @@ -342,7 +342,7 @@ impl<'cfg> Workspace<'cfg> {
}

/// Returns the root Package or VirtualManifest.
fn root_maybe(&self) -> &MaybePackage {
pub fn root_maybe(&self) -> &MaybePackage {
self.packages.get(self.root_manifest())
}

Expand Down
9 changes: 9 additions & 0 deletions src/cargo/ops/cargo_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -803,6 +803,15 @@ impl CompileFilter {
}
}

pub fn is_all_targets(&self) -> bool {
match *self {
CompileFilter::Only {
all_targets: true, ..
} => true,
_ => false,
}
}

pub(crate) fn contains_glob_patterns(&self) -> bool {
match self {
CompileFilter::Default { .. } => false,
Expand Down
111 changes: 110 additions & 1 deletion src/cargo/ops/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,16 @@ use log::{debug, trace, warn};
use rustfix::diagnostics::Diagnostic;
use rustfix::{self, CodeFix};

use crate::core::{Edition, Workspace};
use crate::core::compiler::RustcTargetData;
use crate::core::resolver::features::{FeatureOpts, FeatureResolver, RequestedFeatures};
use crate::core::resolver::{HasDevUnits, ResolveBehavior, ResolveOpts};
use crate::core::{Edition, MaybePackage, Workspace};
use crate::ops::{self, CompileOptions};
use crate::util::diagnostic_server::{Message, RustfixDiagnosticServer};
use crate::util::errors::CargoResult;
use crate::util::{self, paths, Config, ProcessBuilder};
use crate::util::{existing_vcs_repo, LockServer, LockServerClient};
use crate::{drop_eprint, drop_eprintln};

const FIX_ENV: &str = "__CARGO_FIX_PLZ";
const BROKEN_CODE_ENV: &str = "__CARGO_FIX_BROKEN_CODE";
Expand All @@ -74,6 +78,9 @@ pub struct FixOptions {

pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions) -> CargoResult<()> {
check_version_control(ws.config(), opts)?;
if opts.edition {
check_resolver_change(ws, opts)?;
}

// Spin up our lock server, which our subprocesses will use to synchronize fixes.
let lock_server = LockServer::new()?;
Expand Down Expand Up @@ -193,6 +200,108 @@ fn check_version_control(config: &Config, opts: &FixOptions) -> CargoResult<()>
);
}

fn check_resolver_change(ws: &Workspace<'_>, opts: &FixOptions) -> CargoResult<()> {
let root = ws.root_maybe();
match root {
MaybePackage::Package(root_pkg) => {
if root_pkg.manifest().resolve_behavior().is_some() {
// If explicitly specified by the user, no need to check.
return Ok(());
}
// Only trigger if updating the root package from 2018.
let pkgs = opts.compile_opts.spec.get_packages(ws)?;
if !pkgs.iter().any(|&pkg| pkg == root_pkg) {
// The root is not being migrated.
return Ok(());
}
if root_pkg.manifest().edition() != Edition::Edition2018 {
// V1 to V2 only happens on 2018 to 2021.
return Ok(());
}
}
MaybePackage::Virtual(_vm) => {
// Virtual workspaces don't have a global edition to set (yet).
return Ok(());
}
}
// 2018 without `resolver` set must be V1
assert_eq!(ws.resolve_behavior(), ResolveBehavior::V1);
let specs = opts.compile_opts.spec.to_package_id_specs(ws)?;
let resolve_opts = ResolveOpts::new(
/*dev_deps*/ true,
RequestedFeatures::from_command_line(
&opts.compile_opts.features,
opts.compile_opts.all_features,
!opts.compile_opts.no_default_features,
),
Copy link
Member

Choose a reason for hiding this comment

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

Drive-by comment not related to your PR, but this seems like it's ripe for a opts.copmile_opts.requested_features() method

);
let target_data = RustcTargetData::new(ws, &opts.compile_opts.build_config.requested_kinds)?;
// HasDevUnits::No because that may uncover more differences.
// This is not the same as what `cargo fix` is doing, since it is doing
// `--all-targets` which includes dev dependencies.
let ws_resolve = ops::resolve_ws_with_opts(
ws,
&target_data,
&opts.compile_opts.build_config.requested_kinds,
&resolve_opts,
&specs,
HasDevUnits::No,
crate::core::resolver::features::ForceAllTargets::No,
)?;

let feature_opts = FeatureOpts::new_behavior(ResolveBehavior::V2, HasDevUnits::No);
let v2_features = FeatureResolver::resolve(
ws,
&target_data,
&ws_resolve.targeted_resolve,
&ws_resolve.pkg_set,
&resolve_opts.features,
&specs,
&opts.compile_opts.build_config.requested_kinds,
feature_opts,
)?;

let differences = v2_features.compare_legacy(&ws_resolve.resolved_features);
if differences.features.is_empty() && differences.optional_deps.is_empty() {
// Nothing is different, nothing to report.
return Ok(());
}
let config = ws.config();
config.shell().note(
"Switching to Edition 2021 will enable the use of the version 2 feature resolver in Cargo.",
)?;
drop_eprintln!(
config,
"This may cause dependencies to resolve with a different set of features."
);
drop_eprintln!(
config,
"More information about the resolver changes may be found \
at https://doc.rust-lang.org/cargo/reference/features.html#feature-resolver-version-2"
);
drop_eprintln!(
config,
"The following differences were detected with the current configuration:\n"
);
let report = |changes: crate::core::resolver::features::DiffMap, what| {
for ((pkg_id, for_host), removed) in changes {
drop_eprint!(config, " {}", pkg_id);
if for_host {
drop_eprint!(config, " (as build dependency)");
}
if !removed.is_empty() {
let joined: Vec<_> = removed.iter().map(|s| s.as_str()).collect();
drop_eprint!(config, " removed {} `{}`", what, joined.join(","));
}
drop_eprint!(config, "\n");
}
};
report(differences.features, "features");
report(differences.optional_deps, "optional dependency");
drop_eprint!(config, "\n");
Ok(())
}

/// Entry point for `cargo` running as a proxy for `rustc`.
///
/// This is called every time `cargo` is run to check if it is in proxy mode.
Expand Down
8 changes: 5 additions & 3 deletions src/cargo/ops/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@

use crate::core::compiler::{CompileKind, RustcTargetData};
use crate::core::registry::PackageRegistry;
use crate::core::resolver::features::{FeatureResolver, ForceAllTargets, ResolvedFeatures};
use crate::core::resolver::features::{
FeatureOpts, FeatureResolver, ForceAllTargets, ResolvedFeatures,
};
use crate::core::resolver::{self, HasDevUnits, Resolve, ResolveOpts, ResolveVersion};
use crate::core::summary::Summary;
use crate::core::Feature;
Expand Down Expand Up @@ -143,6 +145,7 @@ pub fn resolve_ws_with_opts<'cfg>(
force_all_targets,
)?;

let feature_opts = FeatureOpts::new(ws, has_dev_units, force_all_targets)?;
let resolved_features = FeatureResolver::resolve(
ws,
target_data,
Expand All @@ -151,8 +154,7 @@ pub fn resolve_ws_with_opts<'cfg>(
&opts.features,
specs,
requested_targets,
has_dev_units,
force_all_targets,
feature_opts,
)?;

Ok(WorkspaceResolve {
Expand Down
Loading