Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 1 addition & 14 deletions compiler/noirc_frontend/src/elaborator/enums.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use crate::{
},
hir::{
comptime::Value,
def_collector::dc_crate::{CompilationError, UnresolvedEnum},
def_collector::dc_crate::UnresolvedEnum,
resolution::{errors::ResolverError, import::PathResolutionError},
type_check::TypeCheckError,
},
Expand Down Expand Up @@ -335,19 +335,6 @@ impl Elaborator<'_> {

self.interner.push_fn_meta(meta, method_id);
if let Err(error) = self.interner.add_method(self_type, name_string, method_id, None) {
let error = if matches!(
error,
CompilationError::ResolverError(ResolverError::DuplicateDefinition { .. })
) {
// Expecting a DefCollectorErrorKind::Duplicate error in this case
TypeCheckError::ExpectingOtherError {
message: "define_enum_variant_function: duplicate definition".to_string(),
location,
}
.into()
} else {
error
};
self.push_err(error);
}

Expand Down
4 changes: 1 addition & 3 deletions compiler/noirc_frontend/src/elaborator/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,9 +239,7 @@ impl Elaborator<'_> {
for method_id in function_ids {
let method_name = self.interner.function_name(method_id).to_owned();

if let Err(error) =
self.interner.add_method(self_type, method_name.clone(), *method_id, None)
{
if let Err(error) = self.interner.add_method(self_type, method_name, *method_id, None) {
self.push_err(error);
}
}
Expand Down
116 changes: 115 additions & 1 deletion compiler/noirc_frontend/src/node_interner/methods.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
use std::collections::HashSet;

use crate::{
Type, TypeBindings,
Kind, Type, TypeBindings, TypeVariableId,
hir_def::types::NamedGeneric,
node_interner::{FuncId, TraitId},
};

Expand Down Expand Up @@ -33,6 +36,7 @@ pub struct Methods {
}

impl Methods {
/// Adds a method to this collection, without checking for overlaps.
pub(super) fn add_method(&mut self, method: FuncId, typ: Type, trait_id: Option<TraitId>) {
if let Some(trait_id) = trait_id {
let trait_impl_method = TraitImplMethod { typ, method, trait_id };
Expand All @@ -43,6 +47,116 @@ impl Methods {
}
}

/// Finds an existing direct (inherent) method whose type overlaps with the given type.
/// Returns `Some((method_id, method_type))` if an overlap is found.
///
/// Two types overlap if there exist concrete types that could match both.
/// For example:
/// - `Foo<T>` and `Foo<U>` overlap
/// - `Foo<T>` and `Foo<i32>` overlap (T can be i32)
/// - `Foo<i32>` and `Foo<u64>` don't overlap
pub(super) fn find_overlapping_method(
&self,
typ: &Type,
interner: &NodeInterner,
) -> Option<(FuncId, Type)> {
for existing in &self.direct {
// Check if two types overlap, by instantiating both types (replacing NamedGenerics
// with fresh TypeVariables) and then checking if they can unify.
let instantiate_existing = Self::instantiate_named_generics(&existing.typ, interner);
let instantiate_typ = Self::instantiate_named_generics(typ, interner);
Comment on lines +63 to +67
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for existing in &self.direct {
// Check if two types overlap, by instantiating both types (replacing NamedGenerics
// with fresh TypeVariables) and then checking if they can unify.
let instantiate_existing = Self::instantiate_named_generics(&existing.typ, interner);
let instantiate_typ = Self::instantiate_named_generics(typ, interner);
if self.direct.is_empty() {
return None;
}
let instantiate_typ = Self::instantiate_named_generics(typ, interner);
for existing in &self.direct {
// Check if two types overlap, by instantiating both types (replacing NamedGenerics
// with fresh TypeVariables) and then checking if they can unify.
let instantiate_existing = Self::instantiate_named_generics(&existing.typ, interner);


if Self::types_can_unify(&instantiate_existing, &instantiate_typ) {
return Some((existing.method, existing.typ.clone()));
}
}
None
}

/// Instantiate a type by finding all NamedGenerics and replacing them with
/// fresh type variables.
fn instantiate_named_generics(typ: &Type, interner: &NodeInterner) -> Type {
let mut named_generics = Vec::new();
Self::collect_named_generics(typ, &mut named_generics, &mut HashSet::new());

if named_generics.is_empty() {
return typ.clone();
}

// Create substitutions from each NamedGeneric to a fresh type variable
let substitutions: TypeBindings = named_generics
.into_iter()
.map(|(id, type_var, kind)| {
let fresh = interner.next_type_variable_with_kind(kind.clone());
(id, (type_var, kind, fresh))
})
.collect();

typ.substitute(&substitutions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my understanding: substitute does not actually bind type variables in the typ, it replaces the types that have them (TypeVariable and NamedGeneric) with the substitutions, so NamedGeneric itself is unchanged, right?

If that's right, the docs of substitute could be made clearer.

}

/// Recursively collect all NamedGenerics from a type.
fn collect_named_generics(
typ: &Type,
result: &mut Vec<(TypeVariableId, crate::TypeVariable, Kind)>,
seen: &mut HashSet<TypeVariableId>,
) {
match typ {
Type::NamedGeneric(NamedGeneric { type_var, .. }) => {
let id = type_var.id();
if !seen.contains(&id) {
seen.insert(id);
Comment on lines +107 to +108
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if !seen.contains(&id) {
seen.insert(id);
if !seen.insert(id) {

result.push((id, type_var.clone(), type_var.kind()));
}
}
Type::Array(size, element) => {
Self::collect_named_generics(size, result, seen);
Self::collect_named_generics(element, result, seen);
}
Type::Vector(element) | Type::String(element) | Type::Reference(element, _) => {
Self::collect_named_generics(element, result, seen);
}
Type::FmtString(size, fields) | Type::CheckedCast { from: size, to: fields } => {
Self::collect_named_generics(size, result, seen);
Self::collect_named_generics(fields, result, seen);
}
Type::DataType(_, args) | Type::Alias(_, args) => {
for arg in args {
Self::collect_named_generics(arg, result, seen);
}
}
Type::Tuple(fields) => {
for field in fields {
Self::collect_named_generics(field, result, seen);
}
}
Type::Function(args, ret, env, _) => {
for arg in args {
Self::collect_named_generics(arg, result, seen);
}
Self::collect_named_generics(ret, result, seen);
Self::collect_named_generics(env, result, seen);
}
Type::Forall(_, inner) => {
Self::collect_named_generics(inner, result, seen);
}
Type::InfixExpr(left, _, right, _) => {
Self::collect_named_generics(left, result, seen);
Self::collect_named_generics(right, result, seen);
}
// These types don't contain NamedGenerics
Type::FieldElement
| Type::Integer(..)
| Type::Bool
| Type::Unit
| Type::Quoted(..)
| Type::Error
| Type::TypeVariable(..)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I have seen a TypeVariable be bound to a NamedGeneric

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't explain it, but the example was here: #11385 (comment)

| Type::Constant(..)
| Type::TraitAsType(..) => {}
}
}

pub(super) fn find_direct_method(
&self,
typ: &Type,
Expand Down
42 changes: 24 additions & 18 deletions compiler/noirc_frontend/src/node_interner/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use crate::ast::{
use crate::graph::CrateId;
use crate::hir::comptime;
use crate::hir::def_collector::dc_crate::{CompilationError, UnresolvedTrait, UnresolvedTypeAlias};
use crate::hir::def_collector::errors::DefCollectorErrorKind;
use crate::hir::def_map::{LocalModuleId, ModuleDefId, ModuleId};
use crate::hir::resolution::errors::ResolverError;
use crate::hir::type_check::generics::TraitGenerics;
Expand Down Expand Up @@ -990,10 +991,11 @@ impl NodeInterner {
}
}

/// Adds a non-trait method to a type.
/// Adds a method to a type.
///
/// Returns `Some(duplicate)` if a matching method was already defined.
/// Returns `None` otherwise.
/// For inherent (non-trait) methods, this checks for overlapping implementations.
/// Returns `Ok(())` if the method was added successfully.
/// Returns `Err(error)` if there was an error (e.g., overlapping impl or unsupported type).
pub fn add_method(
&mut self,
self_type: &Type,
Expand All @@ -1016,25 +1018,29 @@ impl NodeInterner {
return Err(error.into());
};

if trait_id.is_none() && matches!(self_type, Type::DataType(..)) {
let check_self_param = false;
if let Some(existing) =
self.lookup_direct_method(self_type, &method_name, check_self_param)
let typ = self_type.clone();

// For inherent (non-trait) methods, check for overlapping implementations.
if trait_id.is_none() {
if let Some(existing_methods) =
self.methods.get(&key).and_then(|m| m.get(&method_name))
{
let first_location = self.function_ident(&existing).location();
let second_location = self.function_ident(&method_id).location();
let error = ResolverError::DuplicateDefinition {
name: method_name,
first_location,
second_location,
};
return Err(error.into());
if let Some((existing_method, existing_type)) =
existing_methods.find_overlapping_method(&typ, self)
{
let prev_location = self.function_ident(&existing_method).location();
let location = self.function_ident(&method_id).location();
let error = DefCollectorErrorKind::OverlappingImpl {
typ: existing_type,
location,
prev_location,
};
return Err(error.into());
}
}
}

// Only remember the actual type if it's FieldOrInt,
// so later we can disambiguate on calls like `u32::call`.
let typ = self_type.clone();
// Add the method to the collection
self.methods
.entry(key)
.or_default()
Expand Down
51 changes: 48 additions & 3 deletions compiler/noirc_frontend/src/tests/structs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,10 @@ fn check_impl_duplicate_method_without_self() {

impl Foo {
fn foo() {}
~~~ first definition found here
~~~ Previous impl defined here
fn foo() {}
^^^ duplicate definitions of foo found
~~~ second definition found here
^^^ Impl for type `Foo` overlaps with existing impl
~~~ Overlapping impl
}
";
check_errors(src);
Expand Down Expand Up @@ -345,3 +345,48 @@ fn abi_attribute_outside_contract() {
"#;
check_errors(src);
}

#[test]
fn overlapping_inherent_impls() {
let src = r#"
struct Foo<T> { _x: T }

impl<T> Foo<T> {
fn method(_self: Self) {}
^^^^^^ Impl for type `Foo<i32>` overlaps with existing impl
~~~~~~ Overlapping impl
}

impl Foo<i32> {
fn method(_self: Self) {}
~~~~~~ Previous impl defined here
}

fn main() {
let _ = Foo { _x: 1 };
}
"#;
check_errors(src);
}

#[test]
fn non_overlapping_inherent_impls() {
// Different concrete types don't overlap
let src = r#"
struct Foo<T> { _x: T }

impl Foo<i32> {
fn method(_self: Self) {}
}

impl Foo<u64> {
fn method(_self: Self) {}
}

fn main() {
let _ = Foo { _x: 1_i32 };
let _ = Foo { _x: 1_u64 };
}
"#;
assert_no_errors(src);
}
Loading