Merge ecd68bba34 into eee4ea2f5a

fix(cargo-lints): Don't always inherit workspace lints

When working on changes for #13805, I noticed that we always passed the contents of `[workspace.lints.cargo]` into the currently implemented lints,  even if `[lints]` was not specified or did not contain `workspace = true`. This PR makes it so we only pass in the workspace cargo lints if `[lints]` contains `workspace = true`.

You can verify this change by looking at the first commit, where I added a test showing the current behavior, and looking at the second commit and seeing the test output no longer shows a warning about specifying `im-a-teapot`.

crates/resolver-tests/src/lib.rs

@@ -835,7 +835,7 @@ fn meta_test_multiple_versions_strategy() {
             .unwrap()
             .current();
         let reg = registry(input.clone());
-        for this in input.iter().rev().take(10) {
+        for this in input.iter().rev().take(11) {
             let res = resolve(
                 vec![dep_req(&this.name(), &format!("={}", this.version()))],
                 &reg,

crates/resolver-tests/tests/resolve.rs

@@ -8,7 +8,8 @@ use cargo_util::is_ci;
 use resolver_tests::{
     assert_contains, assert_same, dep, dep_kind, dep_loc, dep_req, loc_names, names, pkg, pkg_id,
     pkg_loc, registry, registry_strategy, remove_dep, resolve, resolve_and_validated,
-    resolve_with_global_context, PrettyPrintRegistry, SatResolve, ToDep, ToPkgId,
+    resolve_with_global_context, resolve_with_global_context_raw, PrettyPrintRegistry, SatResolve,
+    ToDep, ToPkgId,
 };
 
 use proptest::prelude::*;
@@ -425,6 +426,174 @@ fn test_resolving_maximum_version_with_transitive_deps() {
     assert!(!res.contains(&("util", "1.1.1").to_pkgid()));
 }
 
+#[test]
+fn test_wildcard_minor() {
+    let reg = registry(vec![
+        pkg!(("util", "0.1.0")),
+        pkg!(("util", "0.1.1")),
+        pkg!("foo" => [dep_req("util", "0.1.*")]),
+    ]);
+
+    let res = resolve_and_validated(
+        vec![dep_req("util", "=0.1.0"), dep_req("foo", "1.0.0")],
+        &reg,
+        None,
+    )
+    .unwrap();
+
+    assert_same(
+        &res,
+        &names(&[("root", "1.0.0"), ("foo", "1.0.0"), ("util", "0.1.0")]),
+    );
+}
+
+#[test]
+fn test_wildcard_major() {
+    let reg = registry(vec![
+        pkg!("foo" => [dep_req("util", "0.*")]),
+        pkg!(("util", "0.1.0")),
+        pkg!(("util", "0.2.0")),
+    ]);
+
+    let res = resolve_and_validated(
+        vec![dep_req("foo", "1.0.0"), dep_req("util", "=0.1.0")],
+        &reg,
+        None,
+    )
+    .unwrap();
+
+    assert_same(
+        &res,
+        &names(&[("root", "1.0.0"), ("foo", "1.0.0"), ("util", "0.1.0")]),
+    );
+}
+
+#[test]
+fn test_range_major() {
+    let reg = registry(vec![
+        pkg!("foo" => [dep_req("util", ">=0.1,<0.3")]),
+        pkg!(("util", "0.1.0")),
+        pkg!(("util", "0.2.0")),
+    ]);
+
+    let res = resolve_and_validated(
+        vec![dep_req("foo", "1.0.0"), dep_req("util", "0.1.0")],
+        &reg,
+        None,
+    )
+    .unwrap();
+
+    assert_same(
+        &res,
+        &names(&[("root", "1.0.0"), ("foo", "1.0.0"), ("util", "0.1.0")]),
+    );
+}
+
+#[test]
+fn test_wildcard_major_duplicate_selection() {
+    let reg = registry(vec![
+        pkg!("foo" => [dep_req("util", "0.*")]),
+        pkg!("bar" => [dep_req("util", "0.2")]),
+        pkg!("car" => [dep_req("util", "0.1")]),
+        pkg!(("util", "0.1.0")),
+        pkg!(("util", "0.2.0")),
+    ]);
+
+    let res = resolve_with_global_context_raw(
+        vec![
+            dep_req("foo", "1.0.0"),
+            dep_req("bar", "1.0.0"),
+            dep_req("car", "1.0.0"),
+        ],
+        &reg,
+        &GlobalContext::default().unwrap(),
+    )
+    .unwrap();
+
+    // In this case, both 0.1.0 and 0.2.0 satisfy foo. It should pick the highest
+    // version.
+    assert_eq!(
+        res.deps(pkg_id("foo")).next().unwrap().0,
+        ("util", "0.2.0").to_pkgid()
+    );
+
+    let res = res.sort();
+
+    assert_same(
+        &res,
+        &names(&[
+            ("root", "1.0.0"),
+            ("foo", "1.0.0"),
+            ("bar", "1.0.0"),
+            ("car", "1.0.0"),
+            ("util", "0.1.0"),
+            ("util", "0.2.0"),
+        ]),
+    );
+}
+
+#[test]
+fn test_wildcard_major_coerced_by_subdependency() {
+    let reg = registry(vec![
+        pkg!("foo" => [dep_req("util", "0.1")]),
+        pkg!(("util", "0.1.0")),
+        pkg!(("util", "0.2.0")),
+    ]);
+
+    let res = resolve(vec![dep_req("foo", "1.0.0"), dep_req("util", "0.*")], &reg).unwrap();
+
+    // In this case, both 0.1.0 and 0.2.0 satisfy root, but it's being coerced
+    // by the subdependency of foo.
+    assert_same(
+        &res,
+        &names(&[("root", "1.0.0"), ("foo", "1.0.0"), ("util", "0.1.0")]),
+    );
+}
+
+#[test]
+fn test_wildcard_major_coerced_by_indirect_subdependency() {
+    let reg = registry(vec![
+        pkg!("foo" => [dep_req("util", "0.1")]),
+        pkg!("bar" => [dep_req("car", "1.0.0")]),
+        pkg!("car" => [dep_req("util", "0.2")]),
+        pkg!(("util", "0.1.0")),
+        pkg!(("util", "0.2.0")),
+        pkg!(("util", "0.3.0")),
+    ]);
+
+    let res = resolve_with_global_context_raw(
+        vec![
+            dep_req("foo", "1.0.0"),
+            dep_req("bar", "1.0.0"),
+            dep_req("util", "0.*"),
+        ],
+        &reg,
+        &GlobalContext::default().unwrap(),
+    )
+    .unwrap();
+
+    // In this case, 0.1.0, 0.2.0 and 0.3.0 satisfy root. It should pick the highest
+    // version that exists in the dependency tree.
+    assert_eq!(
+        res.deps(pkg_id("root")).skip(2).next().unwrap().0,
+        ("util", "0.2.0").to_pkgid()
+    );
+
+    let res = res.sort();
+
+    assert_same(
+        &res,
+        &names(&[
+            ("root", "1.0.0"),
+            ("foo", "1.0.0"),
+            ("bar", "1.0.0"),
+            ("car", "1.0.0"),
+            ("util", "0.1.0"),
+            ("util", "0.2.0"),
+        ]),
+    );
+}
+
 #[test]
 fn test_resolving_minimum_version_with_transitive_deps() {
     let reg = registry(vec![
@@ -605,6 +774,35 @@ fn resolving_with_deep_backtracking() {
     );
 }
 
+#[test]
+fn resolving_with_sys_crates_duplicates() {
+    // This is based on issues/4902
+    // With `l` a normal library we get 2copies so everyone gets the newest compatible.
+    // But `l-sys` a library with a links attribute we make sure there is only one.
+    let reg = registry(vec![
+        pkg!(("l-sys", "0.9.1")),
+        pkg!(("l-sys", "0.10.0")),
+        pkg!(("l", "0.9.1") => [dep_req("l-sys", ">=0.8.0, <=0.10.0")]),
+        pkg!(("l", "0.10.0") => [dep_req("l-sys", "0.9")]),
+        pkg!(("d", "1.0.0") => [dep_req("l", "0.10")]),
+        pkg!(("r", "1.0.0") => [dep_req("l", "0.9")]),
+    ]);
+
+    let res = resolve(vec![dep_req("d", "1"), dep_req("r", "1")], &reg).unwrap();
+
+    assert_same(
+        &res,
+        &names(&[
+            ("root", "1.0.0"),
+            ("d", "1.0.0"),
+            ("r", "1.0.0"),
+            ("l-sys", "0.9.1"),
+            ("l", "0.9.1"),
+            ("l", "0.10.0"),
+        ]),
+    );
+}
+
 #[test]
 fn resolving_with_sys_crates() {
     // This is based on issues/4902
@@ -629,7 +827,6 @@ fn resolving_with_sys_crates() {
             ("r", "1.0.0"),
             ("l-sys", "0.9.1"),
             ("l", "0.9.1"),
-            ("l", "0.10.0"),
         ]),
     );
 }

src/cargo/core/resolver/mod.rs

@@ -258,16 +258,8 @@ fn activate_deps_loop(
                 .conflicting(&resolver_ctx, &dep)
                 .is_some();
 
-        let mut remaining_candidates = RemainingCandidates::new(&candidates);
-
-        // `conflicting_activations` stores all the reasons we were unable to
-        // activate candidates. One of these reasons will have to go away for
-        // backtracking to find a place to restart. It is also the list of
-        // things to explain in the error message if we fail to resolve.
-        //
-        // This is a map of package ID to a reason why that packaged caused a
-        // conflict for us.
-        let mut conflicting_activations = ConflictMap::new();
+        let (mut remaining_candidates, mut conflicting_activations) =
+            RemainingCandidates::new(&candidates, &resolver_ctx);
 
         // When backtracking we don't fully update `conflicting_activations`
         // especially for the cases that we didn't make a backtrack frame in the
@@ -277,7 +269,7 @@ fn activate_deps_loop(
         let mut backtracked = false;
 
         loop {
-            let next = remaining_candidates.next(&mut conflicting_activations, &resolver_ctx);
+            let next = remaining_candidates.next();
 
             let (candidate, has_another) = next.ok_or(()).or_else(|_| {
                 // If we get here then our `remaining_candidates` was just
@@ -710,47 +702,38 @@ struct BacktrackFrame {
 /// more inputs) but in general acts like one. Each `RemainingCandidates` is
 /// created with a list of candidates to choose from. When attempting to iterate
 /// over the list of candidates only *valid* candidates are returned. Validity
-/// is defined within a `Context`.
+/// is defined within a `Context`. In addition, the iterator will return
+/// candidates that already have been activated first.
 ///
 /// Candidates passed to `new` may not be returned from `next` as they could be
-/// filtered out, and as they are filtered the causes will be added to `conflicting_prev_active`.
+/// filtered out. The filtered candidates and the causes will be returned by `new`.
 #[derive(Clone)]
 struct RemainingCandidates {
-    remaining: RcVecIter<Summary>,
+    prioritized_candidates: RcVecIter<Summary>,
     // This is an inlined peekable generator
     has_another: Option<Summary>,
 }
 
 impl RemainingCandidates {
-    fn new(candidates: &Rc<Vec<Summary>>) -> RemainingCandidates {
-        RemainingCandidates {
-            remaining: RcVecIter::new(Rc::clone(candidates)),
-            has_another: None,
-        }
-    }
-
-    /// Attempts to find another candidate to check from this list.
-    ///
-    /// This method will attempt to move this iterator forward, returning a
-    /// candidate that's possible to activate. The `cx` argument is the current
-    /// context which determines validity for candidates returned, and the `dep`
-    /// is the dependency listing that we're activating for.
-    ///
-    /// If successful a `(Candidate, bool)` pair will be returned. The
-    /// `Candidate` is the candidate to attempt to activate, and the `bool` is
-    /// an indicator of whether there are remaining candidates to try of if
-    /// we've reached the end of iteration.
-    ///
-    /// If we've reached the end of the iterator here then `Err` will be
-    /// returned. The error will contain a map of package ID to conflict reason,
-    /// where each package ID caused a candidate to be filtered out from the
-    /// original list for the reason listed.
-    fn next(
-        &mut self,
-        conflicting_prev_active: &mut ConflictMap,
+    /// Prefilters and sorts the list of candidates to determine which ones are
+    /// valid to activate, and which ones should be prioritized.
+    fn new(
+        candidates: &Rc<Vec<Summary>>,
         cx: &ResolverContext,
-    ) -> Option<(Summary, bool)> {
-        for b in self.remaining.by_ref() {
+    ) -> (RemainingCandidates, ConflictMap) {
+        // `conflicting_activations` stores all the reasons we were unable to
+        // activate candidates. One of these reasons will have to go away for
+        // backtracking to find a place to restart. It is also the list of
+        // things to explain in the error message if we fail to resolve.
+        //
+        // This is a map of package ID to a reason why that packaged caused a
+        // conflict for us.
+        let mut conflicting_activations = ConflictMap::new();
+
+        let mut activated_candidates: Vec<Summary> = Vec::with_capacity(candidates.len());
+        let mut non_activated_candidates: Vec<Summary> = Vec::with_capacity(candidates.len());
+
+        for b in candidates.as_ref() {
             let b_id = b.package_id();
             // The `links` key in the manifest dictates that there's only one
             // package in a dependency graph, globally, with that particular
@@ -759,7 +742,7 @@ impl RemainingCandidates {
             if let Some(link) = b.links() {
                 if let Some(&a) = cx.links.get(&link) {
                     if a != b_id {
-                        conflicting_prev_active
+                        conflicting_activations
                             .entry(a)
                             .or_insert_with(|| ConflictReason::Links(link));
                         continue;
@@ -772,18 +755,50 @@ impl RemainingCandidates {
             // semver-compatible versions of a crate. For example we can't
             // simultaneously activate `foo 1.0.2` and `foo 1.2.0`. We can,
             // however, activate `1.0.2` and `2.0.0`.
-            //
-            // Here we throw out our candidate if it's *compatible*, yet not
-            // equal, to all previously activated versions.
             if let Some((a, _)) = cx.activations.get(&b_id.as_activations_key()) {
-                if *a != b {
-                    conflicting_prev_active
+                // If this candidate is already activated, then we want to put
+                // it in our prioritized list to try first.
+                if a == b {
+                    activated_candidates.push(b.clone());
+                    continue;
+                }
+                // Here we throw out our candidate if it's *compatible*, yet not
+                // equal, to all previously activated versions.
+                else {
+                    conflicting_activations
                         .entry(a.package_id())
                         .or_insert(ConflictReason::Semver);
                     continue;
                 }
+            } else {
+                non_activated_candidates.push(b.clone());
             }
+        }
 
+        // Combine the prioritized and non-prioritized candidates into one list
+        // such that the prioritized candidates are tried first.
+        activated_candidates.append(&mut non_activated_candidates);
+
+        (
+            RemainingCandidates {
+                prioritized_candidates: RcVecIter::new(Rc::new(activated_candidates)),
+                has_another: None,
+            },
+            conflicting_activations,
+        )
+    }
+
+    /// Attempts to find another candidate to check from this list.
+    ///
+    /// This method will attempt to move this iterator forward, returning a
+    /// candidate that's possible to activate.
+    ///
+    /// If successful a `(Candidate, bool)` pair will be returned. The
+    /// `Candidate` is the candidate to attempt to activate, and the `bool` is
+    /// an indicator of whether there are remaining candidates to try of if
+    /// we've reached the end of iteration.
+    fn next(&mut self) -> Option<(Summary, bool)> {
+        for b in self.prioritized_candidates.by_ref() {
             // Well if we made it this far then we've got a valid dependency. We
             // want this iterator to be inherently "peekable" so we don't
             // necessarily return the item just yet. Instead we stash it away to
@@ -959,9 +974,7 @@ fn find_candidate(
     };
 
     while let Some(mut frame) = backtrack_stack.pop() {
-        let next = frame
-            .remaining_candidates
-            .next(&mut frame.conflicting_activations, &frame.context);
+        let next = frame.remaining_candidates.next();
         let Some((candidate, has_another)) = next else {
             continue;
         };

src/cargo/core/workspace.rs

@@ -1147,11 +1147,26 @@ impl<'gctx> Workspace<'gctx> {
     }
 
     pub fn emit_warnings(&self) -> CargoResult<()> {
+        let ws_lints = self
+            .root_maybe()
+            .workspace_config()
+            .inheritable()
+            .and_then(|i| i.lints().ok())
+            .unwrap_or_default();
+
+        let ws_cargo_lints = ws_lints
+            .get("cargo")
+            .cloned()
+            .unwrap_or_default()
+            .into_iter()
+            .map(|(k, v)| (k.replace('-', "_"), v))
+            .collect();
+
         for (path, maybe_pkg) in &self.packages.packages {
             let path = path.join("Cargo.toml");
             if let MaybePackage::Package(pkg) = maybe_pkg {
                 if self.gctx.cli_unstable().cargo_lints {
-                    self.emit_lints(pkg, &path)?
+                    self.emit_lints(pkg, &path, &ws_cargo_lints)?
                 }
             }
             let warnings = match maybe_pkg {
@@ -1179,22 +1194,12 @@ impl<'gctx> Workspace<'gctx> {
         Ok(())
     }
 
-    pub fn emit_lints(&self, pkg: &Package, path: &Path) -> CargoResult<()> {
-        let ws_lints = self
-            .root_maybe()
-            .workspace_config()
-            .inheritable()
-            .and_then(|i| i.lints().ok())
-            .unwrap_or_default();
-
-        let ws_cargo_lints = ws_lints
-            .get("cargo")
-            .cloned()
-            .unwrap_or_default()
-            .into_iter()
-            .map(|(k, v)| (k.replace('-', "_"), v))
-            .collect();
-
+    pub fn emit_lints(
+        &self,
+        pkg: &Package,
+        path: &Path,
+        ws_cargo_lints: &manifest::TomlToolLints,
+    ) -> CargoResult<()> {
         let mut error_count = 0;
         let toml_lints = pkg
             .manifest()
@@ -1212,11 +1217,21 @@ impl<'gctx> Workspace<'gctx> {
             .map(|(name, lint)| (name.replace('-', "_"), lint))
             .collect();
 
+        // We should only be using workspace lints if the `[lints]` table is
+        // present in the manifest, and `workspace` is set to `true`
+        let ws_cargo_lints = pkg
+            .manifest()
+            .resolved_toml()
+            .lints
+            .as_ref()
+            .is_some_and(|l| l.workspace)
+            .then(|| ws_cargo_lints);
+
         check_im_a_teapot(
             pkg,
             &path,
             &normalized_lints,
-            &ws_cargo_lints,
+            ws_cargo_lints,
             &mut error_count,
             self.gctx,
         )?;
@@ -1224,7 +1239,7 @@ impl<'gctx> Workspace<'gctx> {
             pkg,
             &path,
             &normalized_lints,
-            &ws_cargo_lints,
+            ws_cargo_lints,
             &mut error_count,
             self.gctx,
         )?;
@@ -1232,7 +1247,7 @@ impl<'gctx> Workspace<'gctx> {
             pkg,
             &path,
             &normalized_lints,
-            &ws_cargo_lints,
+            ws_cargo_lints,
             &mut error_count,
             self.gctx,
         )?;

src/cargo/util/lints.rs

@@ -88,7 +88,7 @@ impl Lint {
     pub fn level(
         &self,
         pkg_lints: &TomlToolLints,
-        ws_lints: &TomlToolLints,
+        ws_lints: Option<&TomlToolLints>,
         edition: Edition,
     ) -> (LintLevel, LintLevelReason) {
         self.groups
@@ -188,7 +188,7 @@ fn level_priority(
     default_level: LintLevel,
     edition_lint_opts: Option<(Edition, LintLevel)>,
     pkg_lints: &TomlToolLints,
-    ws_lints: &TomlToolLints,
+    ws_lints: Option<&TomlToolLints>,
     edition: Edition,
 ) -> (LintLevel, LintLevelReason, i8) {
     let (unspecified_level, reason) = if let Some(level) = edition_lint_opts
@@ -211,7 +211,7 @@ fn level_priority(
             LintLevelReason::Package,
             defined_level.priority(),
         )
-    } else if let Some(defined_level) = ws_lints.get(name) {
+    } else if let Some(defined_level) = ws_lints.and_then(|l| l.get(name)) {
         (
             defined_level.level().into(),
             LintLevelReason::Workspace,
@@ -234,7 +234,7 @@ pub fn check_im_a_teapot(
     pkg: &Package,
     path: &Path,
     pkg_lints: &TomlToolLints,
-    ws_lints: &TomlToolLints,
+    ws_lints: Option<&TomlToolLints>,
     error_count: &mut usize,
     gctx: &GlobalContext,
 ) -> CargoResult<()> {
@@ -306,7 +306,7 @@ pub fn check_implicit_features(
     pkg: &Package,
     path: &Path,
     pkg_lints: &TomlToolLints,
-    ws_lints: &TomlToolLints,
+    ws_lints: Option<&TomlToolLints>,
     error_count: &mut usize,
     gctx: &GlobalContext,
 ) -> CargoResult<()> {
@@ -390,7 +390,7 @@ pub fn unused_dependencies(
     pkg: &Package,
     path: &Path,
     pkg_lints: &TomlToolLints,
-    ws_lints: &TomlToolLints,
+    ws_lints: Option<&TomlToolLints>,
     error_count: &mut usize,
     gctx: &GlobalContext,
 ) -> CargoResult<()> {

src/cargo/util/network/sleep.rs

@@ -90,13 +90,15 @@ impl<T> SleepTracker<T> {
 #[test]
 fn returns_in_order() {
     let mut s = SleepTracker::new();
-    s.push(3, 3);
+    s.push(30_000, 30_000);
     s.push(1, 1);
-    s.push(6, 6);
-    s.push(5, 5);
-    s.push(2, 2);
-    s.push(10000, 10000);
-    assert_eq!(s.len(), 6);
-    std::thread::sleep(Duration::from_millis(100));
-    assert_eq!(s.to_retry(), &[1, 2, 3, 5, 6]);
+    assert_eq!(s.len(), 2);
+    std::thread::sleep(Duration::from_millis(2));
+    assert_eq!(s.to_retry(), &[1]);
+    assert!(s.to_retry().is_empty());
+    let next = s.time_to_next().expect("should be next");
+    assert!(
+        next < Duration::from_millis(30_000),
+        "{next:?} should be less than 30s"
+    );
 }

tests/testsuite/lints_table.rs

@@ -982,3 +982,43 @@ error: `im_a_teapot` is specified
         )
         .run();
 }
+
+#[cargo_test]
+fn dont_always_inherit_workspace_lints() {
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+[workspace]
+members = ["foo"]
+
+[workspace.lints.cargo]
+im-a-teapot = "warn"
+"#,
+        )
+        .file(
+            "foo/Cargo.toml",
+            r#"
+cargo-features = ["test-dummy-unstable"]
+
+[package]
+name = "foo"
+version = "0.0.1"
+edition = "2015"
+authors = []
+im-a-teapot = true
+"#,
+        )
+        .file("foo/src/lib.rs", "")
+        .build();
+
+    p.cargo("check -Zcargo-lints")
+        .masquerade_as_nightly_cargo(&["cargo-lints"])
+        .with_stderr(
+            "\
+[CHECKING] foo v0.0.1 ([CWD]/foo)
+[FINISHED] [..]
+",
+        )
+        .run();
+}