From 9290fd5d1282f718747f6f0023e030b835de3a5b Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Mon, 1 Jan 2024 14:38:23 +0100 Subject: [PATCH] fix: increase code coverage --- .../src/hir/duplicate_definition_error.rs | 16 +++++++--- crates/mun_hir/src/diagnostics.rs | 4 +-- crates/mun_hir/src/ids.rs | 1 + crates/mun_hir/src/item_tree/lower.rs | 18 +++++++---- ...r__item_tree__tests__duplicate_import.snap | 11 +++++++ ...ir__item_tree__tests__top_level_items.snap | 2 ++ crates/mun_hir/src/item_tree/tests.rs | 32 +++++++++++++++++-- crates/mun_hir/src/method_resolution.rs | 24 +++++++------- crates/mun_hir/src/package_defs/tests.rs | 1 + crates/mun_hir/src/path.rs | 14 ++++++++ crates/mun_syntax/src/ast/extensions.rs | 29 +++++++++++++++-- 11 files changed, 123 insertions(+), 29 deletions(-) create mode 100644 crates/mun_hir/src/item_tree/snapshots/mun_hir__item_tree__tests__duplicate_import.snap diff --git a/crates/mun_diagnostics/src/hir/duplicate_definition_error.rs b/crates/mun_diagnostics/src/hir/duplicate_definition_error.rs index 8c8e90cf..9d240033 100644 --- a/crates/mun_diagnostics/src/hir/duplicate_definition_error.rs +++ b/crates/mun_diagnostics/src/hir/duplicate_definition_error.rs @@ -32,6 +32,10 @@ fn syntax_node_signature_range( ast::StructDef::cast(syntax_node_ptr.to_node(parse.tree().syntax())) .map_or_else(|| syntax_node_ptr.range(), |s| s.signature_range()) } + SyntaxKind::TYPE_ALIAS_DEF => { + ast::TypeAliasDef::cast(syntax_node_ptr.to_node(parse.tree().syntax())) + .map_or_else(|| syntax_node_ptr.range(), |s| s.signature_range()) + } _ => syntax_node_ptr.range(), } } @@ -58,11 +62,13 @@ fn syntax_node_identifier_range( parse: &Parse, ) -> TextRange { match syntax_node_ptr.kind() { - SyntaxKind::FUNCTION_DEF | SyntaxKind::STRUCT_DEF => syntax_node_ptr - .to_node(parse.tree().syntax()) - .children() - .find(|n| n.kind() == SyntaxKind::NAME) - .map_or_else(|| syntax_node_ptr.range(), |name| name.text_range()), + SyntaxKind::FUNCTION_DEF | SyntaxKind::STRUCT_DEF | SyntaxKind::TYPE_ALIAS_DEF => { + syntax_node_ptr + .to_node(parse.tree().syntax()) + .children() + .find(|n| n.kind() == SyntaxKind::NAME) + .map_or_else(|| syntax_node_ptr.range(), |name| name.text_range()) + } _ => syntax_node_ptr.range(), } } diff --git a/crates/mun_hir/src/diagnostics.rs b/crates/mun_hir/src/diagnostics.rs index 39a9f90a..b4b93071 100644 --- a/crates/mun_hir/src/diagnostics.rs +++ b/crates/mun_hir/src/diagnostics.rs @@ -826,11 +826,11 @@ impl Diagnostic for PrivateTypeAlias { } #[derive(Debug)] -pub struct IncoherentImpl { +pub struct ImplForForeignType { pub impl_: InFile>, } -impl Diagnostic for IncoherentImpl { +impl Diagnostic for ImplForForeignType { fn message(&self) -> String { String::from("cannot define inherent `impl` for foreign type") } diff --git a/crates/mun_hir/src/ids.rs b/crates/mun_hir/src/ids.rs index a5e8228e..0c8e16b1 100644 --- a/crates/mun_hir/src/ids.rs +++ b/crates/mun_hir/src/ids.rs @@ -93,6 +93,7 @@ pub struct ModuleId { pub local_id: LocalModuleId, } +/// Represents an id of an item inside a item container such as a module or a `impl` block. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum ItemContainerId { ModuleId(ModuleId), diff --git a/crates/mun_hir/src/item_tree/lower.rs b/crates/mun_hir/src/item_tree/lower.rs index 05a7d905..1689ff70 100644 --- a/crates/mun_hir/src/item_tree/lower.rs +++ b/crates/mun_hir/src/item_tree/lower.rs @@ -21,11 +21,6 @@ use std::{collections::HashMap, convert::TryInto, marker::PhantomData, sync::Arc struct ModItems(SmallVec<[ModItem; 1]>); -struct Foo {} -impl Foo { - fn bar() {} -} - impl From for ModItems where T: Into, @@ -77,7 +72,18 @@ impl Context { ModItem::Function(item) => Some(&self.data.functions[item.index].name), ModItem::Struct(item) => Some(&self.data.structs[item.index].name), ModItem::TypeAlias(item) => Some(&self.data.type_aliases[item.index].name), - ModItem::Impl(_) | ModItem::Import(_) => None, + ModItem::Import(item) => { + let import = &self.data.imports[item.index]; + if import.is_glob { + None + } else { + import + .alias + .as_ref() + .map_or_else(|| import.path.last_segment(), |alias| alias.as_name()) + } + } + ModItem::Impl(_) => None, }; if let Some(name) = name { if let Some(first_item) = set.get(name) { diff --git a/crates/mun_hir/src/item_tree/snapshots/mun_hir__item_tree__tests__duplicate_import.snap b/crates/mun_hir/src/item_tree/snapshots/mun_hir__item_tree__tests__duplicate_import.snap new file mode 100644 index 00000000..0de4a8e1 --- /dev/null +++ b/crates/mun_hir/src/item_tree/snapshots/mun_hir__item_tree__tests__duplicate_import.snap @@ -0,0 +1,11 @@ +--- +source: crates/mun_hir/src/item_tree/tests.rs +expression: "print_item_tree(r#\"\n use foo::Bar;\n use baz::Bar;\n\n struct Bar {}\n \"#).unwrap()" +--- +use foo::Bar; +use baz::Bar; +struct Bar { +} + +18..26: the name `Bar` is defined multiple times +29..42: the name `Bar` is defined multiple times diff --git a/crates/mun_hir/src/item_tree/snapshots/mun_hir__item_tree__tests__top_level_items.snap b/crates/mun_hir/src/item_tree/snapshots/mun_hir__item_tree__tests__top_level_items.snap index 3d1f48ee..c604270b 100644 --- a/crates/mun_hir/src/item_tree/snapshots/mun_hir__item_tree__tests__top_level_items.snap +++ b/crates/mun_hir/src/item_tree/snapshots/mun_hir__item_tree__tests__top_level_items.snap @@ -44,3 +44,5 @@ struct Baz; type FooBar = Foo; type FooBar = package::Foo; +80..128: the name `bar` is defined multiple times +379..406: the name `FooBar` is defined multiple times diff --git a/crates/mun_hir/src/item_tree/tests.rs b/crates/mun_hir/src/item_tree/tests.rs index 7ca03682..a87c79d3 100644 --- a/crates/mun_hir/src/item_tree/tests.rs +++ b/crates/mun_hir/src/item_tree/tests.rs @@ -1,10 +1,25 @@ -use crate::{mock::MockDatabase, with_fixture::WithFixture, DefDatabase, Upcast}; +use crate::{mock::MockDatabase, with_fixture::WithFixture, DefDatabase, DiagnosticSink, Upcast}; use std::fmt; fn print_item_tree(text: &str) -> Result { let (db, file_id) = MockDatabase::with_single_file(text); let item_tree = db.item_tree(file_id); - super::pretty::print_item_tree(db.upcast(), &item_tree) + let mut result_str = super::pretty::print_item_tree(db.upcast(), &item_tree)?; + let mut sink = DiagnosticSink::new(|diag| { + result_str.push_str(&format!( + "\n{:?}: {}", + diag.highlight_range(), + diag.message() + )) + }); + + item_tree + .diagnostics + .iter() + .for_each(|diag| diag.add_to(&db, &item_tree, &mut sink)); + + drop(sink); + Ok(result_str) } #[test] @@ -62,3 +77,16 @@ fn test_impls() { ) .unwrap()); } + +#[test] +fn test_duplicate_import() { + insta::assert_snapshot!(print_item_tree( + r#" + use foo::Bar; + use baz::Bar; + + struct Bar {} + "# + ) + .unwrap()); +} diff --git a/crates/mun_hir/src/method_resolution.rs b/crates/mun_hir/src/method_resolution.rs index 2d23ef47..cc44d0c5 100644 --- a/crates/mun_hir/src/method_resolution.rs +++ b/crates/mun_hir/src/method_resolution.rs @@ -1,4 +1,4 @@ -use crate::diagnostics::{DuplicateDefinition, IncoherentImpl, InvalidSelfTyImpl}; +use crate::diagnostics::{DuplicateDefinition, ImplForForeignType, InvalidSelfTyImpl}; use crate::ids::AssocItemId; use crate::{ db::HirDatabase, @@ -24,7 +24,7 @@ pub enum InherentImplsDiagnostics { InvalidSelfTy(ImplId), /// The type in the impl is not defined in the same package as the impl. - IncoherentType(ImplId), + ImplForForeignType(ImplId), /// Duplicate definitions of an associated item DuplicateDefinitions(AssocItemId, AssocItemId), @@ -96,7 +96,7 @@ impl InherentImpls { // Make sure the struct is defined in the same package if s.module(db).package().id != package_defs.id { self.diagnostics - .push(InherentImplsDiagnostics::IncoherentType(impl_id)); + .push(InherentImplsDiagnostics::ImplForForeignType(impl_id)); } // Add the impl to the map @@ -177,13 +177,15 @@ impl InherentImplsDiagnostics { .as_ref() .map(AstPtr::new), }), - InherentImplsDiagnostics::IncoherentType(impl_id) => sink.push(IncoherentImpl { - impl_: impl_id - .lookup(db.upcast()) - .source(db.upcast()) - .as_ref() - .map(AstPtr::new), - }), + InherentImplsDiagnostics::ImplForForeignType(impl_id) => { + sink.push(ImplForForeignType { + impl_: impl_id + .lookup(db.upcast()) + .source(db.upcast()) + .as_ref() + .map(AstPtr::new), + }); + } InherentImplsDiagnostics::DuplicateDefinitions(first, second) => { sink.push(DuplicateDefinition { definition: assoc_item_syntax_node_ptr(db.upcast(), second), @@ -198,7 +200,7 @@ impl InherentImplsDiagnostics { match self { InherentImplsDiagnostics::LowerDiagnostic(impl_id, _) | InherentImplsDiagnostics::InvalidSelfTy(impl_id) - | InherentImplsDiagnostics::IncoherentType(impl_id) => impl_id.module(db), + | InherentImplsDiagnostics::ImplForForeignType(impl_id) => impl_id.module(db), InherentImplsDiagnostics::DuplicateDefinitions(_first, second) => second.module(db), } } diff --git a/crates/mun_hir/src/package_defs/tests.rs b/crates/mun_hir/src/package_defs/tests.rs index 5313ecf9..9d3d5849 100644 --- a/crates/mun_hir/src/package_defs/tests.rs +++ b/crates/mun_hir/src/package_defs/tests.rs @@ -48,6 +48,7 @@ fn use_duplicate_name() { mod mod +-- mod bar | +-- ERROR: 4..20: a second item with the same name imported. Try to use an alias. + | +-- ERROR: 23..37: the name `Ok` is defined multiple times | '-- struct Ok '-- mod foo '-- struct Ok diff --git a/crates/mun_hir/src/path.rs b/crates/mun_hir/src/path.rs index e8cfe7b3..5ae4b4e2 100644 --- a/crates/mun_hir/src/path.rs +++ b/crates/mun_hir/src/path.rs @@ -25,6 +25,15 @@ pub enum ImportAlias { Alias(Name), } +impl ImportAlias { + pub fn as_name(&self) -> Option<&Name> { + match self { + ImportAlias::Underscore => None, + ImportAlias::Alias(name) => Some(name), + } + } +} + impl Path { /// Converts an `ast::Path` to `Path`. pub fn from_ast(mut path: ast::Path) -> Option { @@ -97,6 +106,11 @@ impl Path { lower_use_tree(None, &tree, &mut cb); } } + + /// Returns the last segment of the path, if any. + pub fn last_segment(&self) -> Option<&Name> { + self.segments.last() + } } /// Given an `ast::UseTree` and an optional prefix, call a callback function for every item that is diff --git a/crates/mun_syntax/src/ast/extensions.rs b/crates/mun_syntax/src/ast/extensions.rs index 07483bd0..5dc9e6b6 100644 --- a/crates/mun_syntax/src/ast/extensions.rs +++ b/crates/mun_syntax/src/ast/extensions.rs @@ -30,6 +30,14 @@ impl ast::NameRef { } impl ast::FunctionDef { + /// Returns the signature range. + /// + /// ```rust, ignore + /// fn foo_bar() { + /// ^^^^^^^^^^^^___ this part + /// // ... + /// } + /// ``` pub fn signature_range(&self) -> TextRange { let fn_kw = self .syntax() @@ -153,6 +161,15 @@ impl ast::StructDef { pub fn kind(&self) -> StructKind { StructKind::from_node(self) } + + /// Returns the signature range. + /// + /// ```rust, ignore + /// pub(gc) struct Foo { + /// ^^^^^^^^^^___ this part + /// // ... + /// } + /// ``` pub fn signature_range(&self) -> TextRange { let struct_kw = self .syntax() @@ -212,8 +229,14 @@ impl ast::UseTree { } impl ast::TypeAliasDef { + /// Returns the signature range. + /// + /// ```rust, ignore + /// type FooBar = i32 + /// ^^^^^^^^^^^___ this part + /// ``` pub fn signature_range(&self) -> TextRange { - let struct_kw = self + let type_kw = self .syntax() .children_with_tokens() .find(|p| p.kind() == T![type]) @@ -221,11 +244,11 @@ impl ast::TypeAliasDef { let name = self.name().map(|n| n.syntax.text_range()); let start = - struct_kw.map_or_else(|| self.syntax.text_range().start(), rowan::TextRange::start); + type_kw.map_or_else(|| self.syntax.text_range().start(), rowan::TextRange::start); let end = name .map(rowan::TextRange::end) - .or_else(|| struct_kw.map(rowan::TextRange::end)) + .or_else(|| type_kw.map(rowan::TextRange::end)) .unwrap_or_else(|| self.syntax().text_range().end()); TextRange::new(start, end)