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): Resolve object types from method calls a single time #5131

Merged
merged 10 commits into from
May 29, 2024
24 changes: 19 additions & 5 deletions compiler/noirc_frontend/src/hir/type_check/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,8 @@ impl<'interner> TypeChecker<'interner> {
trait_id: id.trait_id,
trait_generics: Vec::new(),
};
self.trait_constraints.push((constraint, *expr_id));
self.trait_constraints.entry(*expr_id).or_default().push(constraint);

jfecher marked this conversation as resolved.
Show resolved Hide resolved
self.typecheck_operator_method(*expr_id, id, &lhs_type, span);
}
typ
Expand Down Expand Up @@ -417,12 +418,25 @@ impl<'interner> TypeChecker<'interner> {
// Push any trait constraints required by this definition to the context
// to be checked later when the type of this variable is further constrained.
if let Some(definition) = self.interner.try_definition(ident.id) {
if let DefinitionKind::Function(function) = definition.kind {
let function = self.interner.function_meta(&function);
if let DefinitionKind::Function(func_id) = definition.kind {
let function = self.interner.function_meta(&func_id);

for mut constraint in function.trait_constraints.clone() {
constraint.apply_bindings(&bindings);
self.trait_constraints.push((constraint, *expr_id));
// When working on chained function calls we can potentially have trait constraints whose type references the result
// of a previous function call. This can lead to ambiguous expression types as we are unable bind the inner function calls
// return type. The logic below checks whether we already have an existing constraint for this identifier.
// If we do have an existing constraint it can be replaced with the constraint specified on the function.
let existing_constraints = self.trait_constraints.entry(*expr_id).or_default();
let existing_trait_index =
existing_constraints.iter().position(|existing_constraint| {
existing_constraint.trait_id == constraint.trait_id
});
if let Some(index) = existing_trait_index {
existing_constraints[index] = constraint;
} else {
existing_constraints.push(constraint);
}
jfecher marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand All @@ -439,7 +453,7 @@ impl<'interner> TypeChecker<'interner> {
// Currently only one impl can be selected per expr_id, so this
// constraint needs to be pushed after any other constraints so
// that monomorphization can resolve this trait method to the correct impl.
self.trait_constraints.push((constraint, *expr_id));
self.trait_constraints.entry(*expr_id).or_default().push(constraint);
}
}

Expand Down
37 changes: 20 additions & 17 deletions compiler/noirc_frontend/src/hir/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

pub use errors::TypeCheckError;
use noirc_errors::Span;
use rustc_hash::FxHashMap as HashMap;

use crate::{
hir_def::{
Expand All @@ -36,7 +37,7 @@
/// verified at the end of a function. This is because constraints arise
/// on each variable, but it is only until function calls when the types
/// needed for the trait constraint may become known.
trait_constraints: Vec<(TraitConstraint, ExprId)>,
trait_constraints: HashMap<ExprId, Vec<TraitConstraint>>,
jfecher marked this conversation as resolved.
Show resolved Hide resolved

/// All type variables created in the current function.
/// This map is used to default any integer type variables at the end of
Expand Down Expand Up @@ -135,28 +136,30 @@
// when multiple impls are available. Instead we default first to choose the Field or u64 impl.
for typ in &type_checker.type_variables {
if let Type::TypeVariable(variable, kind) = typ.follow_bindings() {
let msg = "TypeChecker should only track defaultable type vars";

Check warning on line 139 in compiler/noirc_frontend/src/hir/type_check/mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (defaultable)
variable.bind(kind.default_type().expect(msg));
}
}

// Verify any remaining trait constraints arising from the function body
for (mut constraint, expr_id) in std::mem::take(&mut type_checker.trait_constraints) {
let span = type_checker.interner.expr_span(&expr_id);
for (expr_id, constraints) in std::mem::take(&mut type_checker.trait_constraints) {
for mut constraint in constraints {
let span = type_checker.interner.expr_span(&expr_id);

if matches!(&constraint.typ, Type::MutableReference(_)) {
let (_, dereferenced_typ) =
type_checker.insert_auto_dereferences(expr_id, constraint.typ.clone());
constraint.typ = dereferenced_typ;
}

if matches!(&constraint.typ, Type::MutableReference(_)) {
let (_, dereferenced_typ) =
type_checker.insert_auto_dereferences(expr_id, constraint.typ.clone());
constraint.typ = dereferenced_typ;
type_checker.verify_trait_constraint(
&constraint.typ,
constraint.trait_id,
&constraint.trait_generics,
expr_id,
span,
);
}

type_checker.verify_trait_constraint(
&constraint.typ,
constraint.trait_id,
&constraint.trait_generics,
expr_id,
span,
);
}

// Now remove all the `where` clause constraints we added
Expand Down Expand Up @@ -358,7 +361,7 @@
Self {
interner,
errors: Vec::new(),
trait_constraints: Vec::new(),
trait_constraints: HashMap::default(),
type_variables: Vec::new(),
current_function: None,
}
Expand All @@ -375,7 +378,7 @@
let mut this = Self {
interner,
errors: Vec::new(),
trait_constraints: Vec::new(),
trait_constraints: HashMap::default(),
type_variables: Vec::new(),
current_function: None,
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "regression_5065_failure"
type = "bin"
authors = [""]
compiler_version = ">=0.30.0"

[dependencies]
40 changes: 40 additions & 0 deletions test_programs/compile_failure/regression_5065_failure/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
struct Wrapper<T> {
_value: T,
}

impl<T> Wrapper<T> {
fn new(value: T) -> Self {
Self { _value: value }
}

fn unwrap(self) -> T {
self._value
}
}

trait MyTrait {
fn new() -> Self;
}

struct MyType {}

impl MyTrait for MyType {
fn new() -> Self {
MyType {}
}
}

fn foo<T>() -> T where T: MyTrait {
MyTrait::new()
}

struct BadType {}

// Check that we get "No matching impl found for `BadType: MyTrait`"
fn concise_regression() -> BadType {
Wrapper::new(foo()).unwrap()
}

fn main() {
let _ = concise_regression();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "regression_5065"
type = "bin"
authors = [""]
compiler_version = ">=0.30.0"

[dependencies]
44 changes: 44 additions & 0 deletions test_programs/compile_success_empty/regression_5065/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
struct Wrapper<T> {
_value: T,
}

impl<T> Wrapper<T> {
fn new(value: T) -> Self {
Self { _value: value }
}

fn unwrap(self) -> T {
self._value
}
}

trait MyTrait {
fn new() -> Self;
}

struct MyType {}

impl MyTrait for MyType {
fn new() -> Self {
MyType {}
}
}

fn foo<T>() -> T where T: MyTrait {
MyTrait::new()
}

fn verbose_but_compiles() -> MyType {
let a = Wrapper::new(foo());
a.unwrap()
}

// Check that are able to infer the return type of the call to `foo`
fn concise_regression() -> MyType {
Wrapper::new(foo()).unwrap()
}

fn main() {
let _ = verbose_but_compiles();
let _ = concise_regression();
}
Loading