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(frontend): Error for when impl is stricter than trait #5343

Merged
merged 36 commits into from
Jul 18, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
83b421d
accurately error on no impl found for a trait constraint on a trait i…
vezenovm Jun 26, 2024
952be4e
add impl stricter than trait error
vezenovm Jun 26, 2024
b424cd3
Merge branch 'master' into mv/regression-5210
vezenovm Jun 26, 2024
1ba38f5
master merge
vezenovm Jun 26, 2024
762ea58
nargo fmt
vezenovm Jun 26, 2024
6f5ef1e
reduce test
vezenovm Jun 26, 2024
2dcc646
improve error and fix typo
vezenovm Jun 26, 2024
86ea7b5
cleanup comment
vezenovm Jun 26, 2024
d9df0af
more cleanup
vezenovm Jun 26, 2024
dea35f4
fix regression_4635
vezenovm Jun 26, 2024
70206f3
Merge branch 'master' into mv/regression-5210
vezenovm Jun 28, 2024
e517bc5
conflcits w/ master
vezenovm Jul 8, 2024
18aad5d
Update compiler/noirc_frontend/src/elaborator/mod.rs
vezenovm Jul 9, 2024
17cc6d6
Merge branch 'master' into mv/regression-5210
vezenovm Jul 9, 2024
4abf257
initial work to determine if impl is stricter than trait for differen…
vezenovm Jul 10, 2024
b99e6c9
new strategy using substitute of new type bindings, only works for na…
vezenovm Jul 11, 2024
5f8d756
impl stricter than trait generalized for object types
vezenovm Jul 16, 2024
94372de
merge conflict w/ master
vezenovm Jul 16, 2024
e8e977a
cleanup mod.rs in elabroator package make separate trait_impls module?
vezenovm Jul 16, 2024
b069bd2
cleanup
vezenovm Jul 16, 2024
8f0af08
add comments to reset_generics_on_constraint_type
vezenovm Jul 16, 2024
af0ca70
merge conflicst w/ master
vezenovm Jul 16, 2024
19840ea
fix up method_generic_index reassignment
vezenovm Jul 16, 2024
baa0561
remove unnecessary dbg
vezenovm Jul 16, 2024
b39c76b
try a hacK
vezenovm Jul 16, 2024
66328d8
fix comparison direction
vezenovm Jul 16, 2024
74693fd
cargo fmt
vezenovm Jul 16, 2024
014643a
simpler strategy using just substitute
vezenovm Jul 17, 2024
2eebf76
remove dbgs
vezenovm Jul 17, 2024
bfa8ddd
one more leftover dbg
vezenovm Jul 17, 2024
10ece5c
Merge branch 'master' into mv/regression-5210
vezenovm Jul 17, 2024
9525c9c
Merge branch 'master' into mv/regression-5210
vezenovm Jul 18, 2024
8897de8
cleanup, remove unnecessary field from TraitFunction
vezenovm Jul 18, 2024
3ce5628
Update compiler/noirc_frontend/src/elaborator/trait_impls.rs
jfecher Jul 18, 2024
aebb975
fmt and clippy
vezenovm Jul 18, 2024
093ae58
Merge remote-tracking branch 'origin/mv/regression-5210' into mv/regr…
vezenovm Jul 18, 2024
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
12 changes: 3 additions & 9 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/elaborator/expressions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,7 @@ impl<'context> Elaborator<'context> {
typ: lhs_type.clone(),
trait_id: trait_id.trait_id,
trait_generics: Vec::new(),
span,
};
self.trait_constraints.push((constraint, expr_id));
self.type_check_operator_method(expr_id, trait_id, &lhs_type, span);
Expand Down
46 changes: 38 additions & 8 deletions compiler/noirc_frontend/src/elaborator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,7 @@ impl<'context> Elaborator<'context> {
});
}

Some(TraitConstraint { typ, trait_id, trait_generics })
Some(TraitConstraint { typ, trait_id, trait_generics, span })
}

/// Extract metadata from a NoirFunction
Expand Down Expand Up @@ -875,7 +875,14 @@ impl<'context> Elaborator<'context> {

if let Some(trait_id) = trait_impl.trait_id {
self.generics = trait_impl.resolved_generics.clone();
self.collect_trait_impl_methods(trait_id, trait_impl);

let where_clause = trait_impl
.where_clause
.iter()
.flat_map(|item| self.resolve_trait_constraint(item))
.collect::<Vec<_>>();

self.collect_trait_impl_methods(trait_id, trait_impl, &where_clause);

let span = trait_impl.object_type.span.expect("All trait self types should have spans");
self.declare_methods_on_struct(true, &mut trait_impl.methods, span);
Expand All @@ -885,12 +892,6 @@ impl<'context> Elaborator<'context> {
self.interner.set_function_trait(*func_id, self_type.clone(), trait_id);
}

let where_clause = trait_impl
.where_clause
.iter()
.flat_map(|item| self.resolve_trait_constraint(item))
.collect();

let trait_generics = trait_impl.resolved_trait_generics.clone();

let resolved_trait_impl = Shared::new(TraitImpl {
Expand Down Expand Up @@ -1025,6 +1026,7 @@ impl<'context> Elaborator<'context> {
&mut self,
trait_id: TraitId,
trait_impl: &mut UnresolvedTraitImpl,
trait_impl_where_clause: &[TraitConstraint],
) {
self.local_module = trait_impl.module_id;
self.file = trait_impl.file_id;
Expand Down Expand Up @@ -1081,6 +1083,34 @@ impl<'context> Elaborator<'context> {
}
} else {
for (_, func_id, _) in &overrides {
let func_meta = self.interner.function_meta(func_id);
for override_trait_constraint in func_meta.trait_constraints.clone() {
// We allow where clauses on impls but during definition collection they are included as part of the where
vezenovm marked this conversation as resolved.
Show resolved Hide resolved
// clause of each function. This check is necessary so we do not error about a
let override_constraint_is_from_impl =
trait_impl_where_clause.iter().any(|impl_constraint| {
impl_constraint.trait_id == override_trait_constraint.trait_id
});
if override_constraint_is_from_impl {
continue;
}
jfecher marked this conversation as resolved.
Show resolved Hide resolved

let override_constraint_is_on_trait_method =
method.trait_constraints.iter().any(|method_constraint| {
method_constraint.trait_id == override_trait_constraint.trait_id
});
if !override_constraint_is_on_trait_method {
let the_trait =
self.interner.get_trait(override_trait_constraint.trait_id);
self.push_err(DefCollectorErrorKind::ImplIsStricterThanTrait {
constraint_typ: override_trait_constraint.typ,
constraint_name: the_trait.name.0.contents.clone(),
constraint_span: override_trait_constraint.span,
trait_method_name: method.name.0.contents.clone(),
trait_method_span: method.location.span,
});
}
}
func_ids_in_trait.insert(*func_id);
}

Expand Down
1 change: 0 additions & 1 deletion compiler/noirc_frontend/src/elaborator/patterns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,6 @@ impl<'context> Elaborator<'context> {
if let Some(definition) = self.interner.try_definition(ident.id) {
if let DefinitionKind::Function(function) = definition.kind {
let function = self.interner.function_meta(&function);

for mut constraint in function.trait_constraints.clone() {
constraint.apply_bindings(&bindings);
self.trait_constraints.push((constraint, expr_id));
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/elaborator/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ impl<'context> Elaborator<'context> {
location: Location::new(name.span(), unresolved_trait.file_id),
default_impl,
default_impl_module_id: unresolved_trait.module_id,
trait_constraints: func_meta.trait_constraints.clone(),
});
});
}
Expand Down
2 changes: 2 additions & 0 deletions compiler/noirc_frontend/src/elaborator/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,7 @@ impl<'context> Elaborator<'context> {
generic.type_var.clone()
})),
trait_id,
span: path.span(),
};

return Some((method, constraint, false));
Expand Down Expand Up @@ -433,6 +434,7 @@ impl<'context> Elaborator<'context> {
generic.type_var.clone()
})),
trait_id,
span: path.span(),
};
return Some((method, constraint, false));
}
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@
let module = ModuleId { krate, local_id: self.module_id };

for (_, func_id, noir_function) in &mut unresolved_functions.functions {
// Attach any trait constraints on the impl to the function
noir_function.def.where_clause.append(&mut trait_impl.where_clause.clone());
let location = Location::new(noir_function.def.span, self.file_id);
context.def_interner.push_function(*func_id, &noir_function.def, module, location);
Expand Down Expand Up @@ -518,7 +519,7 @@
}
}
TraitItem::Type { name } => {
// TODO(nickysn or alexvitkov): implement context.def_interner.push_empty_type_alias and get an id, instead of using TypeAliasId::dummy_id()

Check warning on line 522 in compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (nickysn)

Check warning on line 522 in compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (alexvitkov)
if let Err((first_def, second_def)) = self.def_collector.def_map.modules
[trait_id.0.local_id.0]
.declare_type_alias(name.clone(), TypeAliasId::dummy_id())
Expand Down
21 changes: 21 additions & 0 deletions compiler/noirc_frontend/src/hir/def_collector/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,14 @@ pub enum DefCollectorErrorKind {
MacroError(MacroError),
#[error("The only supported types of numeric generics are integers, fields, and booleans")]
UnsupportedNumericGenericType { ident: Ident, typ: UnresolvedTypeData },
#[error("impl has stricter requirements than trait")]
ImplIsStricterThanTrait {
constraint_typ: crate::Type,
constraint_name: String,
constraint_span: Span,
trait_method_name: String,
trait_method_span: Span,
},
}

/// An error struct that macro processors can return.
Expand Down Expand Up @@ -239,6 +247,19 @@ impl<'a> From<&'a DefCollectorErrorKind> for Diagnostic {
ident.0.span(),
)
}
DefCollectorErrorKind::ImplIsStricterThanTrait { constraint_typ, constraint_name: constraint, constraint_span, trait_method_name, trait_method_span } => {
// let constraint_name = &constraint_name.0.contents;
// let bound_name = bound_name;

let mut diag = Diagnostic::simple_error(
"impl has stricter requirements than trait".to_string(),
format!("impl has extra requirement `{constraint_typ}: {constraint}`"),
*constraint_span,
);
// diag.add_note(message)
diag.add_secondary(format!("definition of `{trait_method_name}` from trait"), *trait_method_span);
diag
}
}
}
}
6 changes: 5 additions & 1 deletion compiler/noirc_frontend/src/hir/resolution/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -544,7 +544,7 @@ impl<'a> Resolver<'a> {
});
}

Some(TraitConstraint { typ, trait_id, trait_generics })
Some(TraitConstraint { typ, trait_id, trait_generics, span })
}

/// Translates an UnresolvedType into a Type and appends any
Expand Down Expand Up @@ -1923,6 +1923,7 @@ impl<'a> Resolver<'a> {
generic.type_var.clone()
})),
trait_id,
span: path.span(),
};
return Some((method, constraint, false));
}
Expand All @@ -1940,6 +1941,7 @@ impl<'a> Resolver<'a> {

let mut trait_path = path.clone();
trait_path.pop();
let span = trait_path.span();
let trait_id = self.lookup(trait_path).ok()?;
let the_trait = self.interner.get_trait(trait_id);

Expand All @@ -1953,6 +1955,7 @@ impl<'a> Resolver<'a> {
generic.type_var.clone()
})),
trait_id,
span,
};
return Some((method, constraint, false));
}
Expand Down Expand Up @@ -1994,6 +1997,7 @@ impl<'a> Resolver<'a> {
trait_generics: vecmap(trait_bound.trait_generics, |typ| {
self.resolve_type(typ)
}),
span: trait_bound.trait_path.span(),
};
return Some((method, constraint, true));
}
Expand Down
2 changes: 2 additions & 0 deletions compiler/noirc_frontend/src/hir/resolution/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,8 @@ fn resolve_trait_methods(
location: Location::new(name.span(), unresolved_trait.file_id),
default_impl,
default_impl_module_id: unresolved_trait.module_id,
// This field is only used by the elaborator
trait_constraints: vec![],
});

let errors = resolver.take_errors().into_iter();
Expand Down
2 changes: 2 additions & 0 deletions compiler/noirc_frontend/src/hir/type_check/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,8 @@ impl<'interner> TypeChecker<'interner> {
typ: lhs_type.clone(),
trait_id: id.trait_id,
trait_generics: Vec::new(),
// This field is used by the elaborator
span: Span::default(),
};
self.trait_constraints.push((constraint, *expr_id));
self.typecheck_operator_method(*expr_id, id, &lhs_type, span);
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/hir_def/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ impl HirMethodCallExpression {
typ: object_type,
trait_id: method_id.trait_id,
trait_generics: generics.clone(),
span: location.span,
};
(id, ImplKind::TraitMethod(*method_id, constraint, false))
}
Expand Down
8 changes: 5 additions & 3 deletions compiler/noirc_frontend/src/hir_def/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub struct TraitFunction {
pub location: Location,
pub default_impl: Option<Box<NoirFunction>>,
pub default_impl_module_id: crate::hir::def_map::LocalModuleId,
pub trait_constraints: Vec<TraitConstraint>,
}

#[derive(Clone, Debug, PartialEq, Eq)]
Expand Down Expand Up @@ -82,16 +83,17 @@ pub struct TraitImpl {
pub where_clause: Vec<TraitConstraint>,
}

#[derive(Debug, Clone)]
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct TraitConstraint {
pub typ: Type,
pub trait_id: TraitId,
pub trait_generics: Vec<Type>,
pub span: Span,
}

impl TraitConstraint {
pub fn new(typ: Type, trait_id: TraitId, trait_generics: Vec<Type>) -> Self {
Self { typ, trait_id, trait_generics }
pub fn new(typ: Type, trait_id: TraitId, trait_generics: Vec<Type>, span: Span) -> Self {
Self { typ, trait_id, trait_generics, span }
}

pub fn apply_bindings(&mut self, type_bindings: &TypeBindings) {
Expand Down
10 changes: 8 additions & 2 deletions compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
use iter_extended::vecmap;
use noirc_arena::{Arena, Index};
use noirc_errors::{Location, Span, Spanned};
use petgraph::algo::tarjan_scc;

Check warning on line 10 in compiler/noirc_frontend/src/node_interner.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (petgraph)
use petgraph::prelude::DiGraph;
use petgraph::prelude::NodeIndex as PetGraphIndex;

Expand Down Expand Up @@ -1277,8 +1277,14 @@
type_bindings: &mut TypeBindings,
recursion_limit: u32,
) -> Result<TraitImplKind, Vec<TraitConstraint>> {
let make_constraint =
|| TraitConstraint::new(object_type.clone(), trait_id, trait_generics.to_vec());
let make_constraint = || {
TraitConstraint::new(
object_type.clone(),
trait_id,
trait_generics.to_vec(),
Span::default(),
michaeljklein marked this conversation as resolved.
Show resolved Hide resolved
)
};

// Prevent infinite recursion when looking for impls
if recursion_limit == 0 {
Expand Down
Loading
Loading