fix: shadow type by module #19461
fix: shadow type by module #19461
Conversation
Signed-off-by: Hayashi Mikihiro <[email protected]>
rustbot
added
the
S-waiting-on-review
crates/hir-def/src/resolver.rs
Outdated
| db: &'a dyn DefDatabase, | ||
| path: &'a Path, | ||
| ) -> impl Iterator<Item = (ModuleOrTypeNs, Option<usize>, Option<ImportOrExternCrate>)> + 'a | ||
| { |
There was a problem hiding this comment.
Given edition 2024 new capture rules there shouldn't be a need to define the lifetime 'a.
crates/hir-def/src/resolver.rs
Outdated
| &'a self, | ||
| db: &'a dyn DefDatabase, | ||
| path: &'a Path, | ||
| ) -> Box< |
There was a problem hiding this comment.
Box<dyn Iterator> optimizes very bad and also incurs an allocation, and resolver is very hot.
In general I don't think an iterator is the correct thing to return here. Resolution doesn't have "multiple options". It has a single option. If it resolves the a module, it resolves to it and to nothing else. So this should return a single value.
Signed-off-by: Hayashi Mikihiro <[email protected]>
crates/hir-def/src/resolver.rs
Outdated
| if let Some(res) = m.resolve_path_in_module_or_type_ns(db, path) { | ||
| let res = match res.0 { | ||
| ModuleOrTypeNs::TypeNs(_) => res, | ||
| ModuleOrTypeNs::ModuleNs(_) => { |
There was a problem hiding this comment.
Thank you for the feedback. I agree that it should return a single value.
As an alternative, I tried checking for a conflict between a module and a builtin type in the scope directly, instead of using Box<dyn Iterator> and evaluating lazily.
crates/hir-def/src/resolver.rs
Outdated
| @@ -107,6 +107,12 @@ pub enum TypeNs { | |||
| // ModuleId(ModuleId) | |||
| } | |||
|
|
|||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] | |||
| pub enum ModuleOrTypeNs { | |||
There was a problem hiding this comment.
Modules are part of the type namespace, they're not their own namespace. They've been commented out in the TypeNs enum only because "the resolver is used when all module paths are fully resolved.". It may make more sense to just add the Module case back to TypeNs?
There was a problem hiding this comment.
Thank you. I agree that there's no real need to split modules out from TypeNs.
This enum is only used to check BuiltinType with module. I've included modules in TypeNs instead.
Signed-off-by: Hayashi Mikihiro <[email protected]>
| | TypeNs::ModuleId(_) => { | ||
| // FIXME diagnostic | ||
| (self.err_ty(), None) |
There was a problem hiding this comment.
mod Foo {}
fn main() {
Foo {}
// ^^^ not a struct, variant or union type
}Modules do not have variants, so this results in an error.
| @@ -285,7 +285,9 @@ impl<'a, 'b> PathLoweringContext<'a, 'b> { | |||
| TypeNs::BuiltinType(it) => self.lower_path_inner(it.into(), infer_args), | |||
| TypeNs::TypeAliasId(it) => self.lower_path_inner(it.into(), infer_args), | |||
| // FIXME: report error | |||
| TypeNs::EnumVariantId(_) => return (TyKind::Error.intern(Interner), None), | |||
| TypeNs::EnumVariantId(_) | TypeNs::ModuleId(_) => { | |||
| return (TyKind::Error.intern(Interner), None); | |||
There was a problem hiding this comment.
pub mod M {
pub mod Module {}
}
fn sample_function<Module>(x: Module) {
let x: Module = x;
{
use M::Module;
let x: Module = x;
// ^^^^^^ not a type
}
}
fn main() {}This kind of pattern is similar to the above code.
Here, Module refers to a module, not a type, so using it as a type results in an error.
That's why we return TyKind::Error for TypeNs::ModuleId.
fix: #19107