fix: if item exsits on module, resolve as module instead of type

[Hmikihiro]

fix : #18941, #19107

This PR resolves modules if items exist in the module and treats them as modules.

duplicate-str-module.mov

If a type import overwrites a module, this PR ensures it is resolved as a type

duplicate_with_module_struct.mov

[ChayimFriedman2]

It cannot be an ADT (there will be an error - https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=66662b72d42109fcca7af9a68de608d7), only a primitive type can be shadowed with the same name and different resolutions

[ChayimFriedman2]

Now I see that we don't resolve them correctly even in the case of a module shadowing a struct (I haven't checked why), but this is a different issue - we should always resolve to the module in this case (https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=960405ae19d2bdd97128fb38e364ba08).

[Hmikihiro]

Thank you for comment.
Since defining an Adt and a module in the same scope causes an error, I fixed it by removing the check for whether an item exists in the module when there is a duplicate.

remove_only_std.str.mov

[ChayimFriedman2]

So I looked into this, and I think this isn't the correct approach. Instead, we need to do the following: resolve_hir_path_qualifier() currently tries the module path last (

rust-analyzer/crates/hir/src/source_analyzer.rs

Line 1571 in 208bc52

.resolve_module_path_in_items(db.upcast(), path.mod_path()?)

). I think we need to try it first. This should solve both issues (but please test that both str the type and str the module are resolved correctly).

As an unrelated note, can you please move the test to the goto definition feature? remove_unused_imports will eventually switch to a proper approach that requests the unused imports from analysis, and it'll be a shame to lose these tests in the way (also I believe most resolve_path() tests are there).

[ChayimFriedman2]

No. Wait. Even what I suggested won't solve the problem, because it will prefer the module even when the struct should be preferred, like the following:

mod foo {
    pub fn bar() {}
}

fn main() {
    struct foo;
    impl foo {
        pub fn bar() {}
    }
    foo::bar();
}

Something else is needed. I'll think about it more (it'd be perfect if we could solve the full path, and during the process record which way we went - but this will be too costly. Also, I've discovered that even our resolution is not perfect, in https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=ae501f2aa9e9b987a35d5cfb50146641 we should fail to resolve but we instead fall back to the associate fn foo()).

[Hmikihiro]

I think this function can resolve if those item is only ModuleDef.

rust-analyzer/crates/hir/src/source_analyzer.rs

Line 1571 in 208bc52

.resolve_module_path_in_items(db.upcast(), path.mod_path()?)

When I try it first, this foo can resolved.

mod foo {
    pub fn bar() {}
}

fn main() {
    struct foo;
    impl foo {
        pub fn bar() {}
    }
    foo::bar();
}

but I think resolve_module_path_in_items can't find TypeParam

trait Foo {
    fn fn_foo();
}

mod bar {
    pub mod Bar {
         // ^^^
        pub fn fn_bar() {}
    }
}

fn qux<Bar: Foo>() {
    // ^^^ should find mod bar::Bar
    use bar::Bar;
    Bar::fn_bar();
}

[ChayimFriedman2]

Sorry, but your comment is unclear. Are you claiming that swapping the order is enough?

[Hmikihiro]

Sorry for the confusion.
I think swapping the order is not enough but if the path doesn't conflict with type parameter, it is useful.

[ChayimFriedman2]

But you can't know if the path conflicts with a parameter, or a struct, or whatever.

I think what we need to do is this: the problem is that ModuleItemMap filters the resolutions it accepts:

rust-analyzer/crates/hir-def/src/resolver.rs

Lines 1017 to 1056 in 37355b3

	fn to_value_ns(per_ns: PerNs) -> Option<(ValueNs, Option<ImportOrGlob>)> {
	let (def, import) = per_ns.take_values_import()?;
	let res = match def {
	ModuleDefId::FunctionId(it) => ValueNs::FunctionId(it),
	ModuleDefId::AdtId(AdtId::StructId(it)) => ValueNs::StructId(it),
	ModuleDefId::EnumVariantId(it) => ValueNs::EnumVariantId(it),
	ModuleDefId::ConstId(it) => ValueNs::ConstId(it),
	ModuleDefId::StaticId(it) => ValueNs::StaticId(it),
	ModuleDefId::AdtId(AdtId::EnumId(_) | AdtId::UnionId(_))
	| ModuleDefId::TraitId(_)
	| ModuleDefId::TraitAliasId(_)
	| ModuleDefId::TypeAliasId(_)
	| ModuleDefId::BuiltinType(_)
	| ModuleDefId::MacroId(_)
	| ModuleDefId::ModuleId(_) => return None,
	};
	Some((res, import))
	}
	fn to_type_ns(per_ns: PerNs) -> Option<(TypeNs, Option<ImportOrExternCrate>)> {
	let def = per_ns.take_types_full()?;
	let res = match def.def {
	ModuleDefId::AdtId(it) => TypeNs::AdtId(it),
	ModuleDefId::EnumVariantId(it) => TypeNs::EnumVariantId(it),
	ModuleDefId::TypeAliasId(it) => TypeNs::TypeAliasId(it),
	ModuleDefId::BuiltinType(it) => TypeNs::BuiltinType(it),
	ModuleDefId::TraitId(it) => TypeNs::TraitId(it),
	ModuleDefId::TraitAliasId(it) => TypeNs::TraitAliasId(it),
	ModuleDefId::FunctionId(_)
	| ModuleDefId::ConstId(_)
	| ModuleDefId::MacroId(_)
	| ModuleDefId::StaticId(_)
	| ModuleDefId::ModuleId(_) => return None,
	};
	Some((res, def.import))
	}

This is incorrect. If we resolve to such item, that means we have no resolution, but we instead continue to search in outer scopes. This is also what leads to IDE resolution to fail here, because when resolving only part of the path these resolutions can be correct.

I think we need to remove this filtering, and instead let downstream (analysis, not IDE) err when the resolution is unexpected. IDE should just resolve to them.

Anyway I think we need to separate that from this PR. Make this one focus on builtin shadowing, and if you want make a new PR to solve shadowing of other types.

[Hmikihiro]

Thank you for your advice.
I'll focus only on builtin shadowing and remove the other changes from this PR.

At another time, I'll take a look at those functions and try to solve the shadowing of other types.

[ChayimFriedman2]

Thanks!