Skip to content

Commit

Permalink
fix: require generic trait impls to be in scope to call them (noir-la…
Browse files Browse the repository at this point in the history
…ng/noir#6913)

feat!: require trait primitive functions/calls to have their trait in scope (noir-lang/noir#6901)
feat(lsp): use trait method docs for trait impl method docs on hover (noir-lang/noir#7003)
chore: turn on averaging for protocol circuits metrics in CI (noir-lang/noir#6999)
feat(comptime): Implement to_be_bits and to_le_bits in the interpreter (noir-lang/noir#7008)
chore: Add short circuit in ssa-gen for known if conditions (noir-lang/noir#7007)
chore: Only resolved globals monomorphization (noir-lang/noir#7006)
chore: Remove resolve_is_unconstrained pass (noir-lang/noir#7004)
chore: require safety doc comment for unsafe instead of `//@safety` (noir-lang/noir#6992)
fix: Reproduce and fix bytecode blowup (noir-lang/noir#6972)
chore: mark casts as able to be deduplicated (noir-lang/noir#6996)
fix: return trait impl method as FuncId if there's only one (noir-lang/noir#6989)
chore(ci): fail properly in `external-repo-checks` (noir-lang/noir#6988)
fix: allow multiple trait impls for the same trait as long as one is in scope (noir-lang/noir#6987)
chore: Use DFG in SSA printer (noir-lang/noir#6986)
chore!: Reserve `enum` and `match` keywords (noir-lang/noir#6961)
  • Loading branch information
AztecBot committed Jan 10, 2025
2 parents a95a252 + e9c5985 commit 271966d
Show file tree
Hide file tree
Showing 10 changed files with 152 additions and 91 deletions.
2 changes: 1 addition & 1 deletion .noir-sync-commit
Original file line number Diff line number Diff line change
@@ -1 +1 @@
56c931a8e59e9e67e8bd5aae4a114e85fe40ff98
5300ec321fb99ddaad32e83f751aed28e175736f
22 changes: 11 additions & 11 deletions noir/noir-repo/.github/workflows/reports.yml
Original file line number Diff line number Diff line change
Expand Up @@ -274,15 +274,15 @@ jobs:
run: |
./execution_report.sh 0 1
mv execution_report.json ../execution_report.json
- name: Upload compilation report
uses: actions/upload-artifact@v4
with:
name: in_progress_compilation_report
path: compilation_report.json
retention-days: 3
overwrite: true

- name: Upload execution report
uses: actions/upload-artifact@v4
with:
Expand Down Expand Up @@ -360,7 +360,7 @@ jobs:
chmod +x parse_time.sh
cp /home/runner/work/noir/noir/scripts/test_programs/parse_time.sh ./parse_time.sh
./compilation_report.sh 1 1
- name: Generate execution report without averages
working-directory: ./test-repo/${{ matrix.project.path }}
if: ${{ !matrix.project.is_library && !matrix.project.take_average }}
Expand Down Expand Up @@ -395,7 +395,7 @@ jobs:
PACKAGE_NAME=$(basename $PACKAGE_NAME)
mv ./test-repo/${{ matrix.project.path }}/execution_report.json ./execution_report_$PACKAGE_NAME.json
echo "execution_report_name=$PACKAGE_NAME" >> $GITHUB_OUTPUT
- name: Upload compilation report
uses: actions/upload-artifact@v4
with:
Expand All @@ -413,10 +413,10 @@ jobs:
overwrite: true

upload_compilation_report:
name: Upload compilation report
name: Upload compilation report
needs: [generate_compilation_and_execution_report, external_repo_compilation_and_execution_report]
# We want this job to run even if one variation of the matrix in `external_repo_compilation_and_execution_report` fails
if: always()
if: always()
runs-on: ubuntu-latest
permissions:
pull-requests: write
Expand Down Expand Up @@ -550,7 +550,7 @@ jobs:
overwrite: true

upload_compilation_memory_report:
name: Upload compilation memory report
name: Upload compilation memory report
needs: [generate_memory_report, external_repo_memory_report]
# We want this job to run even if one variation of the matrix in `external_repo_memory_report` fails
if: always()
Expand Down Expand Up @@ -594,10 +594,10 @@ jobs:
message: ${{ steps.compilation_mem_report.outputs.markdown }}

upload_execution_memory_report:
name: Upload execution memory report
name: Upload execution memory report
needs: [generate_memory_report, external_repo_memory_report]
# We want this job to run even if one variation of the matrix in `external_repo_memory_report` fails
if: always()
if: always()
runs-on: ubuntu-latest
permissions:
pull-requests: write
Expand Down Expand Up @@ -640,10 +640,10 @@ jobs:
message: ${{ steps.execution_mem_report.outputs.markdown }}

upload_execution_report:
name: Upload execution report
name: Upload execution report
needs: [generate_compilation_and_execution_report, external_repo_compilation_and_execution_report]
# We want this job to run even if one variation of the matrix in `external_repo_compilation_and_execution_report` fails
if: always()
if: always()
runs-on: ubuntu-latest
permissions:
pull-requests: write
Expand Down
4 changes: 4 additions & 0 deletions noir/noir-repo/.github/workflows/test-js-packages.yml
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,10 @@ jobs:
fi
env:
NARGO_IGNORE_TEST_FAILURES_FROM_FOREIGN_CALLS: true

- name: Compare test results
working-directory: ./noir-repo
run: .github/scripts/check_test_results.sh .github/critical_libraries_status/${{ matrix.project.repo }}/${{ matrix.project.path }}.failures.jsonl .github/critical_libraries_status/${{ matrix.project.repo }}/${{ matrix.project.path }}.actual.jsonl

- name: Compare test results
working-directory: ./noir-repo
Expand Down
80 changes: 46 additions & 34 deletions noir/noir-repo/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 noir/noir-repo/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 noir/noir-repo/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");
}
2 changes: 1 addition & 1 deletion noir/noir-repo/noir_stdlib/src/meta/expr.nr
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,7 @@ comptime fn new_unary_op(op: UnaryOp, rhs: Expr) -> Expr {

comptime fn new_unsafe(exprs: [Expr]) -> Expr {
let exprs = join_expressions(exprs, quote { ; });
quote {
quote {
/// Safety: generated by macro
unsafe { $exprs }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -553,9 +553,9 @@ mod tests {
#[test]
fn test_expr_as_unsafe() {
comptime {
let expr = quote {
let expr = quote {
/// Safety: test
unsafe { 1; 4; 23 }
unsafe { 1; 4; 23 }
}
.as_expr()
.unwrap();
Expand All @@ -567,9 +567,9 @@ mod tests {
#[test]
fn test_expr_modify_for_unsafe() {
comptime {
let expr = quote {
let expr = quote {
/// Safety: test
unsafe { 1; 4; 23 }
unsafe { 1; 4; 23 }
}
.as_expr()
.unwrap();
Expand Down
Loading

0 comments on commit 271966d

Please sign in to comment.