Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: require generic trait impls to be in scope to call them #6913

Merged
merged 30 commits into from
Jan 9, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
1df848d
fix: track trait IDs for trait impls on structs
asterite Dec 19, 2024
33e7b97
feat: require trait methods in paths to be in scope
asterite Dec 19, 2024
ed171c3
Show trait full path in error message
asterite Dec 19, 2024
9294758
Fix frontend tests
asterite Dec 19, 2024
6eaf88f
Fix bug introduced in fully_qualified_module_path
asterite Dec 19, 2024
28fdbe2
Add a couple more comments
asterite Dec 19, 2024
f661567
Fix: multiple trait methods were not registered for structs
asterite Dec 20, 2024
c274629
Reuse `Elaborator::lookup_method` in interpreter
asterite Dec 20, 2024
c980099
Require traits to be in scope to call methods
asterite Dec 20, 2024
4797814
Merge branch 'master' into ab/require-trait-to-be-in-scope
asterite Dec 20, 2024
8b6b175
Merge branch 'ab/require-trait-to-be-in-scope' into ab/require-trait-…
asterite Dec 20, 2024
3cbe8d0
Move test somewhere else
asterite Dec 20, 2024
e0f2d0e
Put struct and primitive methods in the same HashMap
asterite Dec 20, 2024
8b8b8f9
Put struct and primitive methods in the same HashMap
asterite Dec 20, 2024
1d2e302
Try to unify method lookup a bit more
asterite Dec 20, 2024
c63002a
Unify struct and primitive method lookup
asterite Dec 20, 2024
dc6e95f
Make sure it also works the same for primitive method calls
asterite Dec 20, 2024
6ba3a77
fix: require generic trait impls to be in scope to call them
asterite Dec 23, 2024
07383b8
Merge branch 'master' into ab/require-trait-to-be-in-scope-for-methods
asterite Jan 3, 2025
c94448c
Remove duplicate method
asterite Jan 3, 2025
fdb2311
Merge branch 'master' into ab/require-trait-to-be-in-scope-for-methods
asterite Jan 5, 2025
91abe70
Merge branch 'master' into ab/require-trait-to-be-in-scope-for-methods
asterite Jan 6, 2025
44dbb3b
cargo fmt
asterite Jan 6, 2025
bddf052
Merge branch 'master' into ab/require-trait-to-be-in-scope-for-methods
asterite Jan 6, 2025
95366c0
Merge branch 'master' into ab/require-trait-to-be-in-scope-for-methods
asterite Jan 6, 2025
e2fe92d
Merge branch 'master' into ab/require-trait-to-be-in-scope-for-methods
TomAFrench Jan 6, 2025
3088b1d
Merge branch 'master' into ab/require-trait-to-be-in-scope-for-methods
asterite Jan 7, 2025
e6b2286
Merge branch 'ab/require-trait-to-be-in-scope-for-methods' into ab/tr…
asterite Jan 7, 2025
018fb02
Merge branch 'ab/trait-functions-in-scope-for-primitives' into ab/tra…
asterite Jan 7, 2025
7221420
Merge branch 'master' into ab/trait-functions-in-scope-for-generics
asterite Jan 9, 2025
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
80 changes: 46 additions & 34 deletions compiler/noirc_frontend/src/elaborator/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1361,7 +1361,7 @@ impl<'context> Elaborator<'context> {
span: Span,
has_self_arg: bool,
) -> Option<HirMethodReference> {
// First search in the struct methods
// First search in the type methods. If there is one, that's the one.
if let Some(method_id) =
self.interner.lookup_direct_method(object_type, method_name, has_self_arg)
{
Expand All @@ -1372,43 +1372,55 @@ impl<'context> Elaborator<'context> {
let trait_methods =
self.interner.lookup_trait_methods(object_type, method_name, has_self_arg);

if trait_methods.is_empty() {
// If we couldn't find any trait methods, search in
// impls for all types `T`, e.g. `impl<T> Foo for T`
if let Some(func_id) =
self.interner.lookup_generic_method(object_type, method_name, has_self_arg)
{
return Some(HirMethodReference::FuncId(func_id));
}
// If there's at least one matching trait method we need to see if only one is in scope.
if !trait_methods.is_empty() {
return self.return_trait_method_in_scope(&trait_methods, method_name, span);
}

if let Type::Struct(struct_type, _) = object_type {
let has_field_with_function_type =
struct_type.borrow().get_fields_as_written().into_iter().any(|field| {
field.name.0.contents == method_name && field.typ.is_function()
});
if has_field_with_function_type {
self.push_err(TypeCheckError::CannotInvokeStructFieldFunctionType {
method_name: method_name.to_string(),
object_type: object_type.clone(),
span,
});
} else {
self.push_err(TypeCheckError::UnresolvedMethodCall {
method_name: method_name.to_string(),
object_type: object_type.clone(),
span,
});
}
return None;
// If we couldn't find any trait methods, search in
// impls for all types `T`, e.g. `impl<T> Foo for T`
let generic_methods =
self.interner.lookup_generic_methods(object_type, method_name, has_self_arg);
if !generic_methods.is_empty() {
return self.return_trait_method_in_scope(&generic_methods, method_name, span);
}

if let Type::Struct(struct_type, _) = object_type {
let has_field_with_function_type = struct_type
.borrow()
.get_fields_as_written()
.into_iter()
.any(|field| field.name.0.contents == method_name && field.typ.is_function());
if has_field_with_function_type {
self.push_err(TypeCheckError::CannotInvokeStructFieldFunctionType {
method_name: method_name.to_string(),
object_type: object_type.clone(),
span,
});
} else {
// It could be that this type is a composite type that is bound to a trait,
// for example `x: (T, U) ... where (T, U): SomeTrait`
// (so this case is a generalization of the NamedGeneric case)
return self.lookup_method_in_trait_constraints(object_type, method_name, span);
self.push_err(TypeCheckError::UnresolvedMethodCall {
method_name: method_name.to_string(),
object_type: object_type.clone(),
span,
});
}
None
} else {
// It could be that this type is a composite type that is bound to a trait,
// for example `x: (T, U) ... where (T, U): SomeTrait`
// (so this case is a generalization of the NamedGeneric case)
self.lookup_method_in_trait_constraints(object_type, method_name, span)
}
}

// We found some trait methods... but is only one of them currently in scope?
/// Given a list of functions and the trait they belong to, returns the one function
/// that is in scope.
fn return_trait_method_in_scope(
&mut self,
trait_methods: &[(FuncId, TraitId)],
method_name: &str,
span: Span,
) -> Option<HirMethodReference> {
let module_id = self.module_id();
let module_data = self.get_module(module_id);

Expand Down Expand Up @@ -1490,7 +1502,7 @@ impl<'context> Elaborator<'context> {
fn trait_hir_method_reference(
&self,
trait_id: TraitId,
trait_methods: Vec<(FuncId, TraitId)>,
trait_methods: &[(FuncId, TraitId)],
method_name: &str,
span: Span,
) -> HirMethodReference {
Expand Down
45 changes: 10 additions & 35 deletions compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1746,7 +1746,7 @@ impl NodeInterner {
Ok(())
}

/// Looks up a method that's directly defined in the given struct.
/// Looks up a method that's directly defined in the given type.
pub fn lookup_direct_method(
&self,
typ: &Type,
Expand All @@ -1761,7 +1761,7 @@ impl NodeInterner {
.and_then(|methods| methods.find_direct_method(typ, has_self_arg, self))
}

/// Looks up a methods that apply to the given struct but are defined in traits.
/// Looks up a methods that apply to the given type but are defined in traits.
pub fn lookup_trait_methods(
&self,
typ: &Type,
Expand All @@ -1780,43 +1780,18 @@ impl NodeInterner {
}
}

/// Select the 1 matching method with an object type matching `typ`
fn find_matching_method(
&self,
typ: &Type,
methods: Option<&Methods>,
method_name: &str,
has_self_arg: bool,
) -> Option<FuncId> {
if let Some(method) = methods.and_then(|m| m.find_matching_method(typ, has_self_arg, self))
{
Some(method)
} else {
self.lookup_generic_method(typ, method_name, has_self_arg)
}
}

/// Looks up a method at impls for all types `T`, e.g. `impl<T> Foo for T`
pub fn lookup_generic_method(
/// Looks up methods at impls for all types `T`, e.g. `impl<T> Foo for T`
pub fn lookup_generic_methods(
&self,
typ: &Type,
method_name: &str,
has_self_arg: bool,
) -> Option<FuncId> {
let global_methods = self.methods.get(&TypeMethodKey::Generic)?.get(method_name)?;
global_methods.find_matching_method(typ, has_self_arg, self)
}

/// Looks up a given method name on the given primitive type.
pub fn lookup_primitive_method(
&self,
typ: &Type,
method_name: &str,
has_self_arg: bool,
) -> Option<FuncId> {
let key = get_type_method_key(typ)?;
let methods = self.methods.get(&key)?.get(method_name)?;
self.find_matching_method(typ, Some(methods), method_name, has_self_arg)
) -> Vec<(FuncId, TraitId)> {
self.methods
.get(&TypeMethodKey::Generic)
.and_then(|h| h.get(method_name))
.map(|methods| methods.find_trait_methods(typ, has_self_arg, self))
.unwrap_or_default()
}

/// Returns what the next trait impl id is expected to be.
Expand Down
70 changes: 70 additions & 0 deletions compiler/noirc_frontend/src/tests/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -879,6 +879,43 @@ fn errors_if_multiple_trait_methods_are_in_scope_for_function_call() {
assert_eq!(traits, vec!["private_mod::Foo", "private_mod::Foo2"]);
}

#[test]
fn warns_if_trait_is_not_in_scope_for_method_call_and_there_is_only_one_trait_method() {
let src = r#"
fn main() {
let bar = Bar { x: 42 };
let _ = bar.foo();
}

pub struct Bar {
x: i32,
}

mod private_mod {
pub trait Foo {
fn foo(self) -> i32;
}

impl Foo for super::Bar {
fn foo(self) -> i32 {
self.x
}
}
}
"#;
let errors = get_program_errors(src);
assert_eq!(errors.len(), 1);

let CompilationError::ResolverError(ResolverError::PathResolutionError(
PathResolutionError::TraitMethodNotInScope { ident, trait_name },
)) = &errors[0].0
else {
panic!("Expected a 'trait method not in scope' error");
};
assert_eq!(ident.to_string(), "foo");
assert_eq!(trait_name, "private_mod::Foo");
}

#[test]
fn calls_trait_method_if_it_is_in_scope() {
let src = r#"
Expand Down Expand Up @@ -1166,3 +1203,36 @@ fn warns_if_trait_is_not_in_scope_for_primitive_method_call_and_there_is_only_on
assert_eq!(ident.to_string(), "foo");
assert_eq!(trait_name, "private_mod::Foo");
}

#[test]
fn warns_if_trait_is_not_in_scope_for_generic_function_call_and_there_is_only_one_trait_method() {
let src = r#"
fn main() {
let x: i32 = 1;
let _ = x.foo();
}

mod private_mod {
pub trait Foo<T> {
fn foo(self) -> i32;
}

impl<T> Foo<T> for T {
fn foo(self) -> i32 {
42
}
}
}
"#;
let errors = get_program_errors(src);
assert_eq!(errors.len(), 1);

let CompilationError::ResolverError(ResolverError::PathResolutionError(
PathResolutionError::TraitMethodNotInScope { ident, trait_name },
)) = &errors[0].0
else {
panic!("Expected a 'trait method not in scope' error");
};
assert_eq!(ident.to_string(), "foo");
assert_eq!(trait_name, "private_mod::Foo");
}
Loading