misc: changes from code review

crates/mun_hir/src/item_scope.rs

@@ -17,8 +17,8 @@ pub(crate) enum ImportType {
 }
 
 /// A struct that holds information on which name was imported via a glob import. This information
-/// is used by the `PackageDef` collector to keep track of duplicates so that this doesnt result in
-/// a duplicate name error:
+/// is used by the `PackageDef` collector to keep track of duplicates so that this doesn't result in
+/// a duplicate name error; e.g. :
 /// ```mun
 /// use foo::{Foo, *};
 /// ```
@@ -48,7 +48,7 @@ pub(crate) struct AddResolutionFromImportResult {
     /// Whether or not adding the resolution changed the item scope
     pub changed: bool,
 
-    /// Whether or not adding the resolution will overwrite and existing entry
+    /// Whether or not adding the resolution will overwrite an existing entry
     pub duplicate: bool,
 }
 
@@ -85,7 +85,7 @@ impl ItemScope {
     }
 
     /// Adds a named item resolution into the scope. Returns true if adding the resolution changes
-    /// the scope or not.
+    /// the scope.
     pub(crate) fn add_resolution(
         &mut self,
         name: Name,
@@ -109,7 +109,7 @@ impl ItemScope {
     }
 
     /// Adds a named item resolution into the scope which is the result of a `use` statement.
-    /// Returns true if adding the resolution changes the scope or not.
+    /// Returns true if adding the resolution changes the scope.
     pub(crate) fn add_resolution_from_import(
         &mut self,
         glob_imports: &mut PerNsGlobImports,
@@ -134,7 +134,7 @@ impl ItemScope {
                         match $def_import_type {
                             // If this is a wildcard import, add it to the list of items we imported
                             // via a glob. This information is stored so if we later explicitly
-                            // import this type or value, it doesnt cause a conflict.
+                            // import this type or value, it doesn't cause a conflict.
                             ImportType::Glob => {
                                 $glob_imports.$field.insert($lookup.clone());
                             }

crates/mun_hir/src/module_tree.rs

@@ -12,7 +12,7 @@ use std::sync::Arc;
 
 /// Represents the tree of modules of a package.
 ///
-/// The `ModuleTree` is build by looking at all the source files of the source root of a package and
+/// The `ModuleTree` is built by looking at all the source files of the source root of a package and
 /// creating a tree based on their relative paths. See the [`ModuleTree::module_tree_query`] method.
 /// When constructing the `ModuleTree` extra empty modules may be added for missing files. For
 /// instance for the relative path `foo/bar/baz.mun`, besides the module `foo::bar::baz` the modules

crates/mun_hir/src/name_resolution/path_resolution.rs

@@ -9,7 +9,7 @@ use std::iter::successors;
 
 /// Indicates whether or not any newly resolved import statements will actually change the outcome
 /// of an operation. This is useful to know if more iterations of an algorithm might be required, or
-/// if its hopeless.
+/// whether its hopeless.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum ReachedFixedPoint {
     Yes,

crates/mun_hir/src/package_defs.rs

@@ -55,12 +55,14 @@ mod diagnostics {
     use mun_syntax::ast::Use;
     use mun_syntax::{ast, AstPtr};
 
+    /// A type of diagnostic that may be emitted during resolving all package definitions.
     #[derive(Debug, PartialEq, Eq)]
     enum DiagnosticKind {
         UnresolvedImport { ast: AstId<ast::Use>, index: usize },
         DuplicateImport { ast: AstId<ast::Use>, index: usize },
     }
 
+    /// A diagnostic that may be emitted during resolving all package definitions.
     #[derive(Debug, PartialEq, Eq)]
     pub(super) struct DefDiagnostic {
         /// The module that contains the diagnostic

crates/mun_hir/src/package_defs/collector.rs

@@ -1,10 +1,9 @@
 use super::PackageDefs;
-use crate::item_scope::PerNsGlobImports;
 use crate::{
     ids::ItemDefinitionId,
     ids::{FunctionLoc, Intern, StructLoc, TypeAliasLoc},
     item_scope::ImportType,
-    item_scope::ItemScope,
+    item_scope::{ItemScope, PerNsGlobImports},
     item_tree::{
         self, Function, ItemTree, ItemTreeId, LocalItemTreeId, ModItem, Struct, StructDefKind,
         TypeAlias,
@@ -18,13 +17,14 @@ use crate::{
 };
 use rustc_hash::FxHashMap;
 
+/// Result of resolving an import statement
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
-enum PartialResolvedImport {
-    /// None of any namespaces is resolved
+enum PartiallyResolvedImport {
+    /// None of the namespaces are resolved
     Unresolved,
     /// One of namespaces is resolved.
     Indeterminate(PerNs<(ItemDefinitionId, Visibility)>),
-    /// All namespaces are resolved, OR it is came from other crate
+    /// All namespaces are resolved, OR it came from another crate
     Resolved(PerNs<(ItemDefinitionId, Visibility)>),
 }
 
@@ -38,16 +38,17 @@ struct ImportResolution {
     resolution: PerNs<(ItemDefinitionId, Visibility)>,
 }
 
-impl PartialResolvedImport {
+impl PartiallyResolvedImport {
     fn namespaces(&self) -> PerNs<(ItemDefinitionId, Visibility)> {
         match self {
-            PartialResolvedImport::Unresolved => PerNs::none(),
-            PartialResolvedImport::Indeterminate(ns) => *ns,
-            PartialResolvedImport::Resolved(ns) => *ns,
+            PartiallyResolvedImport::Unresolved => PerNs::none(),
+            PartiallyResolvedImport::Indeterminate(ns) => *ns,
+            PartiallyResolvedImport::Resolved(ns) => *ns,
         }
     }
 }
 
+/// Definition of a single import statement
 #[derive(Clone, Debug, Eq, PartialEq)]
 struct Import {
     /// The path of the import (e.g. foo::Bar). Note that group imports have been desugared, each
@@ -92,7 +93,7 @@ struct ImportDirective {
     import: Import,
 
     /// The current status of the import.
-    status: PartialResolvedImport,
+    status: PartiallyResolvedImport,
 }
 
 pub(super) fn collect(db: &dyn DefDatabase, package_id: PackageId) -> PackageDefs {
@@ -121,7 +122,7 @@ struct DefCollector<'db> {
     unresolved_imports: Vec<ImportDirective>,
     resolved_imports: Vec<ImportDirective>,
 
-    /// A mapping from local module to wildcard imports to other modules
+    /// A mapping from local module to wildcard imports of other modules
     glob_imports:
         FxHashMap<LocalModuleId, Vec<(LocalModuleId, Visibility, ItemTreeId<item_tree::Import>)>>,
 
@@ -135,7 +136,7 @@ impl<'db> DefCollector<'db> {
         // Collect all definitions in each module
         let module_tree = self.package_defs.module_tree.clone();
 
-        // Start by collecting the definitions from all modules. This ensures that very every module
+        // Start by collecting the definitions from all modules. This ensures that, for every module,
         // all local definitions are accessible. This is the starting point for the import
         // resolution.
         collect_modules_recursive(self, module_tree.root, None);
@@ -156,21 +157,21 @@ impl<'db> DefCollector<'db> {
                 // Check the status of the import, if the import is still considered unresolved, try
                 // again in the next round.
                 match directive.status {
-                    PartialResolvedImport::Indeterminate(_) => {
+                    PartiallyResolvedImport::Indeterminate(_) => {
                         self.record_resolved_import(&directive);
-                        // FIXME: To avoid performance regression, we consider an imported resolved
+                        // FIXME: To avoid performance regression, we consider an import resolved
                         // if it is indeterminate (i.e not all namespace resolved). This might not
                         // completely resolve correctly in the future if we can have values and
                         // types with the same name.
                         self.resolved_imports.push(directive);
                         resolved_something = true;
                     }
-                    PartialResolvedImport::Resolved(_) => {
+                    PartiallyResolvedImport::Resolved(_) => {
                         self.record_resolved_import(&directive);
                         self.resolved_imports.push(directive);
                         resolved_something = true;
                     }
-                    PartialResolvedImport::Unresolved => {
+                    PartiallyResolvedImport::Unresolved => {
                         self.unresolved_imports.push(directive);
                     }
                 }
@@ -238,27 +239,27 @@ impl<'db> DefCollector<'db> {
     }
 
     /// Given an import, try to resolve it.
-    fn resolve_import(&self, module_id: LocalModuleId, import: &Import) -> PartialResolvedImport {
+    fn resolve_import(&self, module_id: LocalModuleId, import: &Import) -> PartiallyResolvedImport {
         let res = self
             .package_defs
             .resolve_path_with_fixedpoint(self.db, module_id, &import.path);
 
         let def = res.resolved_def;
         if res.reached_fixedpoint == ReachedFixedPoint::No || def.is_none() {
-            return PartialResolvedImport::Unresolved;
+            return PartiallyResolvedImport::Unresolved;
         }
 
         if let Some(package) = res.package {
             if package != self.package_defs.module_tree.package {
-                return PartialResolvedImport::Resolved(def);
+                return PartiallyResolvedImport::Resolved(def);
             }
         }
 
-        // Check whether all namespace is resolved
+        // Check whether all namespaces have been resolved
         if def.take_types().is_some() && def.take_values().is_some() {
-            PartialResolvedImport::Resolved(def)
+            PartiallyResolvedImport::Resolved(def)
         } else {
-            PartialResolvedImport::Indeterminate(def)
+            PartiallyResolvedImport::Indeterminate(def)
         }
     }
 
@@ -316,7 +317,7 @@ impl<'db> DefCollector<'db> {
                     }
                 }
                 Some((_, _)) => {
-                    // Happens when wildcard importing something other than a module. I guess its ok to do nothing here?
+                    // Happens when wildcard importing something other than a module. I guess it's ok to do nothing here?
                 }
                 None => {
                     // Happens if a wildcard import refers to something other than a type?
@@ -383,14 +384,14 @@ impl<'db> DefCollector<'db> {
 
         let mut changed = false;
         for ImportResolution { name, resolution } in resolutions {
-            // TODO: Add an error if the visibility of the item does not allow exposing with the
+            // TODO(#309): Add an error if the visibility of the item does not allow exposing with the
             // import visibility. e.g.:
             // ```mun
             // //- foo.mun
             // pub(package) struct Foo;
             //
             // //- main.mun
-            // pub foo::Foo; // This is not allowed because Foo is only public for the package.
+            // pub foo::Foo; // This is not allowed because Foo is only public within the package.
             // ```
 
             match name {
@@ -526,7 +527,7 @@ impl<'a> ModCollectorContext<'a, '_> {
         self.def_collector.unresolved_imports.push(ImportDirective {
             module_id: self.module_id,
             import: Import::from_use(&self.item_tree, InFile::new(self.file_id, id)),
-            status: PartialResolvedImport::Unresolved,
+            status: PartiallyResolvedImport::Unresolved,
         });
         None
     }

crates/mun_hir/src/path.rs

@@ -16,6 +16,7 @@ pub enum PathKind {
     Package,
 }
 
+/// A possible import alias e.g. `Foo as Bar` or `Foo as _`.
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub enum ImportAlias {
     /// Unnamed alias, as in `use Foo as _;`
@@ -77,7 +78,7 @@ impl Path {
         None
     }
 
-    /// Construct a path from its segments
+    /// Constructs a path from its segments.
     pub fn from_segments(kind: PathKind, segments: impl IntoIterator<Item = Name>) -> Path {
         let segments = segments.into_iter().collect::<Vec<_>>();
         Path { kind, segments }

crates/mun_hir/src/visibility.rs

@@ -58,7 +58,7 @@ pub enum Visibility {
 }
 
 impl Visibility {
-    /// Returns true if an item with this visibility is accessible from the module from the
+    /// Returns true if an item with this visibility is accessible from the module of the
     /// specified `PackageDefs`.
     pub(crate) fn is_visible_from_module_tree(
         self,

crates/mun_syntax/src/parsing/grammar/declarations.rs

@@ -151,7 +151,7 @@ fn use_tree(p: &mut Parser, top_level: bool) {
             if top_level {
                 p.error_recover(msg, DECLARATION_RECOVERY_SET);
             } else {
-                // if we are parsing a nested tree, we have to eat a token to main balanced `{}`
+                // if we are parsing a nested tree, we have to eat a token to remain balanced `{}`
                 p.error_and_bump(msg);
             }
             return;