Auto merge of #17252 - davidbarsky:david/refactor-standalone-bools-into-struct, r=Veykril

internal: refactor `prefer_no_std`/`prefer_prelude` bools into a struct

I noticed that there's a large number of functions/arguments during an unrelated change that take two booleans and realized they're _probably_ better off being in a single struct—less error-prone, etc.

Feel free to suggest a better name than `ImportPathConfig`/close this entirely! I can also make these args enums; just hopefully making this a little more misuse-resistant.

Author: bors

src/tools/rust-analyzer/crates/hir-def/src/find_path.rs

@@ -1,9 +1,6 @@
 //! An algorithm to find a path to refer to a certain item.
 
-use std::{
-    cmp::Ordering,
-    iter::{self, once},
-};
+use std::{cmp::Ordering, iter};
 
 use hir_expand::{
     name::{known, AsName, Name},
@@ -17,7 +14,7 @@ use crate::{
     nameres::DefMap,
     path::{ModPath, PathKind},
     visibility::{Visibility, VisibilityExplicitness},
-    ModuleDefId, ModuleId,
+    ImportPathConfig, ModuleDefId, ModuleId,
 };
 
 /// Find a path that can be used to refer to a certain item. This can depend on
@@ -28,21 +25,10 @@ pub fn find_path(
     from: ModuleId,
     prefix_kind: PrefixKind,
     ignore_local_imports: bool,
-    prefer_no_std: bool,
-    prefer_prelude: bool,
+    cfg: ImportPathConfig,
 ) -> Option<ModPath> {
     let _p = tracing::span!(tracing::Level::INFO, "find_path").entered();
-    find_path_inner(
-        FindPathCtx {
-            db,
-            prefix: prefix_kind,
-            prefer_no_std,
-            prefer_prelude,
-            ignore_local_imports,
-        },
-        item,
-        from,
-    )
+    find_path_inner(FindPathCtx { db, prefix: prefix_kind, cfg, ignore_local_imports }, item, from)
 }
 
 #[derive(Copy, Clone, Debug)]
@@ -88,16 +74,15 @@ impl PrefixKind {
 struct FindPathCtx<'db> {
     db: &'db dyn DefDatabase,
     prefix: PrefixKind,
-    prefer_no_std: bool,
-    prefer_prelude: bool,
+    cfg: ImportPathConfig,
     ignore_local_imports: bool,
 }
 
 /// Attempts to find a path to refer to the given `item` visible from the `from` ModuleId
 fn find_path_inner(ctx: FindPathCtx<'_>, item: ItemInNs, from: ModuleId) -> Option<ModPath> {
     // - if the item is a builtin, it's in scope
     if let ItemInNs::Types(ModuleDefId::BuiltinType(builtin)) = item {
-        return Some(ModPath::from_segments(PathKind::Plain, once(builtin.as_name())));
+        return Some(ModPath::from_segments(PathKind::Plain, iter::once(builtin.as_name())));
     }
 
     let def_map = from.def_map(ctx.db);
@@ -107,7 +92,11 @@ fn find_path_inner(ctx: FindPathCtx<'_>, item: ItemInNs, from: ModuleId) -> Opti
         let mut visited_modules = FxHashSet::default();
         return find_path_for_module(
             FindPathCtx {
-                prefer_no_std: ctx.prefer_no_std || ctx.db.crate_supports_no_std(crate_root.krate),
+                cfg: ImportPathConfig {
+                    prefer_no_std: ctx.cfg.prefer_no_std
+                        || ctx.db.crate_supports_no_std(crate_root.krate),
+                    ..ctx.cfg
+                },
                 ..ctx
             },
             &def_map,
@@ -132,7 +121,7 @@ fn find_path_inner(ctx: FindPathCtx<'_>, item: ItemInNs, from: ModuleId) -> Opti
         // - if the item is already in scope, return the name under which it is
         let scope_name = find_in_scope(ctx.db, &def_map, from, item, ctx.ignore_local_imports);
         if let Some(scope_name) = scope_name {
-            return Some(ModPath::from_segments(prefix.path_kind(), Some(scope_name)));
+            return Some(ModPath::from_segments(prefix.path_kind(), iter::once(scope_name)));
         }
     }
 
@@ -160,7 +149,11 @@ fn find_path_inner(ctx: FindPathCtx<'_>, item: ItemInNs, from: ModuleId) -> Opti
 
     calculate_best_path(
         FindPathCtx {
-            prefer_no_std: ctx.prefer_no_std || ctx.db.crate_supports_no_std(crate_root.krate),
+            cfg: ImportPathConfig {
+                prefer_no_std: ctx.cfg.prefer_no_std
+                    || ctx.db.crate_supports_no_std(crate_root.krate),
+                ..ctx.cfg
+            },
             ..ctx
         },
         &def_map,
@@ -213,7 +206,7 @@ fn find_path_for_module(
             } else {
                 PathKind::Plain
             };
-            return Some((ModPath::from_segments(kind, once(name.clone())), Stable));
+            return Some((ModPath::from_segments(kind, iter::once(name.clone())), Stable));
         }
     }
     let prefix = if module_id.is_within_block() { PrefixKind::Plain } else { ctx.prefix };
@@ -231,7 +224,10 @@ fn find_path_for_module(
         );
         if let Some(scope_name) = scope_name {
             // - if the item is already in scope, return the name under which it is
-            return Some((ModPath::from_segments(prefix.path_kind(), once(scope_name)), Stable));
+            return Some((
+                ModPath::from_segments(prefix.path_kind(), iter::once(scope_name)),
+                Stable,
+            ));
         }
     }
 
@@ -305,7 +301,7 @@ fn find_in_prelude(
         });
 
     if found_and_same_def.unwrap_or(true) {
-        Some(ModPath::from_segments(PathKind::Plain, once(name.clone())))
+        Some(ModPath::from_segments(PathKind::Plain, iter::once(name.clone())))
     } else {
         None
     }
@@ -381,9 +377,7 @@ fn calculate_best_path(
                 path.0.push_segment(name);
 
                 let new_path = match best_path.take() {
-                    Some(best_path) => {
-                        select_best_path(best_path, path, ctx.prefer_no_std, ctx.prefer_prelude)
-                    }
+                    Some(best_path) => select_best_path(best_path, path, ctx.cfg),
                     None => path,
                 };
                 best_path_len = new_path.0.len();
@@ -425,12 +419,7 @@ fn calculate_best_path(
                 );
 
                 let new_path_with_stab = match best_path.take() {
-                    Some(best_path) => select_best_path(
-                        best_path,
-                        path_with_stab,
-                        ctx.prefer_no_std,
-                        ctx.prefer_prelude,
-                    ),
+                    Some(best_path) => select_best_path(best_path, path_with_stab, ctx.cfg),
                     None => path_with_stab,
                 };
                 update_best_path(&mut best_path, new_path_with_stab);
@@ -446,8 +435,7 @@ fn calculate_best_path(
 fn select_best_path(
     old_path @ (_, old_stability): (ModPath, Stability),
     new_path @ (_, new_stability): (ModPath, Stability),
-    prefer_no_std: bool,
-    prefer_prelude: bool,
+    cfg: ImportPathConfig,
 ) -> (ModPath, Stability) {
     match (old_stability, new_stability) {
         (Stable, Unstable) => return old_path,
@@ -461,7 +449,7 @@ fn select_best_path(
         let (old_path, _) = &old;
         let new_has_prelude = new_path.segments().iter().any(|seg| seg == &known::prelude);
         let old_has_prelude = old_path.segments().iter().any(|seg| seg == &known::prelude);
-        match (new_has_prelude, old_has_prelude, prefer_prelude) {
+        match (new_has_prelude, old_has_prelude, cfg.prefer_prelude) {
             (true, false, true) | (false, true, false) => new,
             (true, false, false) | (false, true, true) => old,
             // no prelude difference in the paths, so pick the shorter one
@@ -482,7 +470,7 @@ fn select_best_path(
 
     match (old_path.0.segments().first(), new_path.0.segments().first()) {
         (Some(old), Some(new)) if STD_CRATES.contains(old) && STD_CRATES.contains(new) => {
-            let rank = match prefer_no_std {
+            let rank = match cfg.prefer_no_std {
                 false => |name: &Name| match name {
                     name if name == &known::core => 0,
                     name if name == &known::alloc => 1,
@@ -647,10 +635,9 @@ mod tests {
         {
             let found_path = find_path_inner(
                 FindPathCtx {
-                    prefer_no_std: false,
                     db: &db,
                     prefix,
-                    prefer_prelude,
+                    cfg: ImportPathConfig { prefer_no_std: false, prefer_prelude },
                     ignore_local_imports,
                 },
                 resolved,

src/tools/rust-analyzer/crates/hir-def/src/lib.rs

@@ -108,6 +108,15 @@ use crate::{
 
 type FxIndexMap<K, V> =
     indexmap::IndexMap<K, V, std::hash::BuildHasherDefault<rustc_hash::FxHasher>>;
+/// A wrapper around two booleans, [`ImportPathConfig::prefer_no_std`] and [`ImportPathConfig::prefer_prelude`].
+#[derive(Debug, Clone, PartialEq, Eq, Hash, Copy)]
+pub struct ImportPathConfig {
+    /// If true, prefer to unconditionally use imports of the `core` and `alloc` crate
+    /// over the std.
+    pub prefer_no_std: bool,
+    /// If true, prefer import paths containing a prelude module.
+    pub prefer_prelude: bool,
+}
 
 #[derive(Debug)]
 pub struct ItemLoc<N: ItemTreeNode> {

src/tools/rust-analyzer/crates/hir-ty/src/display.rs

@@ -21,7 +21,8 @@ use hir_def::{
     path::{Path, PathKind},
     type_ref::{TraitBoundModifier, TypeBound, TypeRef},
     visibility::Visibility,
-    HasModule, ItemContainerId, LocalFieldId, Lookup, ModuleDefId, ModuleId, TraitId,
+    HasModule, ImportPathConfig, ItemContainerId, LocalFieldId, Lookup, ModuleDefId, ModuleId,
+    TraitId,
 };
 use hir_expand::name::Name;
 use intern::{Internable, Interned};
@@ -1001,8 +1002,7 @@ impl HirDisplay for Ty {
                             module_id,
                             PrefixKind::Plain,
                             false,
-                            false,
-                            true,
+                            ImportPathConfig { prefer_no_std: false, prefer_prelude: true },
                         ) {
                             write!(f, "{}", path.display(f.db.upcast()))?;
                         } else {

src/tools/rust-analyzer/crates/hir/src/lib.rs

@@ -122,6 +122,7 @@ pub use {
         per_ns::Namespace,
         type_ref::{Mutability, TypeRef},
         visibility::Visibility,
+        ImportPathConfig,
         // FIXME: This is here since some queries take it as input that are used
         // outside of hir.
         {AdtId, MacroId, ModuleDefId},
@@ -792,17 +793,15 @@ impl Module {
         self,
         db: &dyn DefDatabase,
         item: impl Into<ItemInNs>,
-        prefer_no_std: bool,
-        prefer_prelude: bool,
+        cfg: ImportPathConfig,
     ) -> Option<ModPath> {
         hir_def::find_path::find_path(
             db,
             item.into().into(),
             self.into(),
             PrefixKind::Plain,
             false,
-            prefer_no_std,
-            prefer_prelude,
+            cfg,
         )
     }
 
@@ -813,18 +812,9 @@ impl Module {
         db: &dyn DefDatabase,
         item: impl Into<ItemInNs>,
         prefix_kind: PrefixKind,
-        prefer_no_std: bool,
-        prefer_prelude: bool,
+        cfg: ImportPathConfig,
     ) -> Option<ModPath> {
-        hir_def::find_path::find_path(
-            db,
-            item.into().into(),
-            self.into(),
-            prefix_kind,
-            true,
-            prefer_no_std,
-            prefer_prelude,
-        )
+        hir_def::find_path::find_path(db, item.into().into(), self.into(), prefix_kind, true, cfg)
     }
 }