Skip to content

Commit

Permalink
feat: check unconstrained trait impl method matches (#6057)
Browse files Browse the repository at this point in the history
# Description

## Problem

Fixes #5717

## Summary

In a previous PR we started allowing `unconstrained` and `comptime` in
trait impls. But two things remained:
1. Defining an `unconstrained` trait impl method would produce a
warning.
2. We should error if a trait method is unconstrained but the trait impl
method is not, or the other way around.

This PR will also include "unconstrained" in trait imp method stubs in
LSP.

## Additional Context

I initially also errored if there's a mismatch in `comptime`, but that's
probably not good as we have some implementation of `Append` be comptime
and some not. This made me wonder what's the purpose of `comptime` in
functions, if non-comptime functions can also be interpreted.

## Documentation

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: jfecher <jake@aztecprotocol.com>
  • Loading branch information
asterite and jfecher authored Sep 17, 2024
1 parent f476c4b commit aedc983
Show file tree
Hide file tree
Showing 7 changed files with 35 additions and 6 deletions.
9 changes: 9 additions & 0 deletions compiler/noirc_frontend/src/elaborator/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ pub(crate) fn check_trait_impl_method_matches_declaration(
function: FuncId,
) -> Vec<TypeCheckError> {
let meta = interner.function_meta(&function);
let modifiers = interner.function_modifiers(&function);
let method_name = interner.function_name(&function);
let mut errors = Vec::new();

Expand Down Expand Up @@ -267,6 +268,14 @@ pub(crate) fn check_trait_impl_method_matches_declaration(
// issue an error for it here.
if let Some(trait_fn_id) = trait_info.method_ids.get(method_name) {
let trait_fn_meta = interner.function_meta(trait_fn_id);
let trait_fn_modifiers = interner.function_modifiers(trait_fn_id);

if modifiers.is_unconstrained != trait_fn_modifiers.is_unconstrained {
let expected = trait_fn_modifiers.is_unconstrained;
let span = meta.name.location.span;
let item = method_name.to_string();
errors.push(TypeCheckError::UnconstrainedMismatch { item, expected, span });
}

if trait_fn_meta.direct_generics.len() != meta.direct_generics.len() {
let expected = trait_fn_meta.direct_generics.len();
Expand Down
10 changes: 10 additions & 0 deletions compiler/noirc_frontend/src/hir/type_check/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ pub enum TypeCheckError {
ParameterCountMismatch { expected: usize, found: usize, span: Span },
#[error("{item} expects {expected} generics but {found} were given")]
GenericCountMismatch { item: String, expected: usize, found: usize, span: Span },
#[error("{item} has incompatible `unconstrained`")]
UnconstrainedMismatch { item: String, expected: bool, span: Span },
#[error("Only integer and Field types may be casted to")]
UnsupportedCast { span: Span },
#[error("Index {index} is out of bounds for this tuple {lhs_type} of length {length}")]
Expand Down Expand Up @@ -264,6 +266,14 @@ impl<'a> From<&'a TypeCheckError> for Diagnostic {
let msg = format!("{item} expects {expected} generic{empty_or_s} but {found} {was_or_were} given");
Diagnostic::simple_error(msg, String::new(), *span)
}
TypeCheckError::UnconstrainedMismatch { item, expected, span } => {
let msg = if *expected {
format!("{item} is expected to be unconstrained")
} else {
format!("{item} is not expected to be unconstrained")
};
Diagnostic::simple_error(msg, String::new(), *span)
}
TypeCheckError::InvalidCast { span, .. }
| TypeCheckError::ExpectedFunction { span, .. }
| TypeCheckError::AccessUnknownMember { span, .. }
Expand Down
6 changes: 3 additions & 3 deletions compiler/noirc_frontend/src/parser/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ pub enum ParserErrorReason {
PatternInTraitFunctionParameter,
#[error("Patterns aren't allowed in a trait impl's associated constants")]
PatternInAssociatedConstant,
#[error("Modifiers are ignored on a trait impl method")]
TraitImplFunctionModifiers,
#[error("Visibility is ignored on a trait impl method")]
TraitImplVisibilityIgnored,
#[error("comptime keyword is deprecated")]
ComptimeDeprecated,
#[error("{0} are experimental and aren't fully supported yet")]
Expand Down Expand Up @@ -202,7 +202,7 @@ impl<'a> From<&'a ParserError> for Diagnostic {
ParserErrorReason::ExperimentalFeature(_) => {
Diagnostic::simple_warning(reason.to_string(), "".into(), error.span)
}
ParserErrorReason::TraitImplFunctionModifiers => {
ParserErrorReason::TraitImplVisibilityIgnored => {
Diagnostic::simple_warning(reason.to_string(), "".into(), error.span)
}
ParserErrorReason::ExpectedPatternButFoundType(ty) => Diagnostic::simple_error(
Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_frontend/src/parser/parser/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,8 @@ pub(super) fn trait_implementation() -> impl NoirParser<TopLevelStatementKind> {

fn trait_implementation_body() -> impl NoirParser<Vec<Documented<TraitImplItem>>> {
let function = function::function_definition(true).validate(|mut f, span, emit| {
if f.def().is_unconstrained || f.def().visibility != ItemVisibility::Private {
emit(ParserError::with_reason(ParserErrorReason::TraitImplFunctionModifiers, span));
if f.def().visibility != ItemVisibility::Private {
emit(ParserError::with_reason(ParserErrorReason::TraitImplVisibilityIgnored, span));
}
// Trait impl functions are always public
f.def_mut().visibility = ItemVisibility::Public;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,12 @@ impl<'a> CodeActionFinder<'a> {

for (name, func_id) in method_ids {
let func_meta = self.interner.function_meta(func_id);
let modifiers = self.interner.function_modifiers(func_id);

let mut generator = TraitImplMethodStubGenerator::new(
name,
func_meta,
modifiers,
trait_,
noir_trait_impl,
self.interner,
Expand Down
2 changes: 2 additions & 0 deletions tooling/lsp/src/requests/completion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -899,10 +899,12 @@ impl<'a> NodeFinder<'a> {
}

let func_meta = self.interner.function_meta(&func_id);
let modifiers = self.interner.function_modifiers(&func_id);

let mut generator = TraitImplMethodStubGenerator::new(
&name,
func_meta,
modifiers,
trait_,
noir_trait_impl,
self.interner,
Expand Down
8 changes: 7 additions & 1 deletion tooling/lsp/src/trait_impl_method_stub_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use noirc_frontend::{
},
hir_def::{function::FuncMeta, stmt::HirPattern, traits::Trait},
macros_api::{ModuleDefId, NodeInterner},
node_interner::ReferenceId,
node_interner::{FunctionModifiers, ReferenceId},
Kind, ResolvedGeneric, Type, TypeVariableKind,
};

Expand All @@ -18,6 +18,7 @@ use crate::modules::relative_module_id_path;
pub(crate) struct TraitImplMethodStubGenerator<'a> {
name: &'a str,
func_meta: &'a FuncMeta,
modifiers: &'a FunctionModifiers,
trait_: &'a Trait,
noir_trait_impl: &'a NoirTraitImpl,
interner: &'a NodeInterner,
Expand All @@ -33,6 +34,7 @@ impl<'a> TraitImplMethodStubGenerator<'a> {
pub(crate) fn new(
name: &'a str,
func_meta: &'a FuncMeta,
modifiers: &'a FunctionModifiers,
trait_: &'a Trait,
noir_trait_impl: &'a NoirTraitImpl,
interner: &'a NodeInterner,
Expand All @@ -43,6 +45,7 @@ impl<'a> TraitImplMethodStubGenerator<'a> {
Self {
name,
func_meta,
modifiers,
trait_,
noir_trait_impl,
interner,
Expand All @@ -63,6 +66,9 @@ impl<'a> TraitImplMethodStubGenerator<'a> {
let indent_string = " ".repeat(self.indent);

self.string.push_str(&indent_string);
if self.modifiers.is_unconstrained {
self.string.push_str("unconstrained ");
}
self.string.push_str("fn ");
self.string.push_str(self.name);
self.append_resolved_generics(&self.func_meta.direct_generics);
Expand Down

0 comments on commit aedc983

Please sign in to comment.