Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
53 changes: 0 additions & 53 deletions compiler/noirc_frontend/src/elaborator/patterns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -569,12 +569,6 @@ impl Elaborator<'_> {

pub(super) fn elaborate_variable(&mut self, variable: Path) -> (ExprId, Type) {
let variable = self.validate_path(variable);
if let Some((expr_id, typ)) =
self.elaborate_variable_as_self_method_or_associated_constant(&variable)
{
return (expr_id, typ);
}

let resolved_turbofish = variable.segments.last().unwrap().generics.clone();

let location = variable.location;
Expand Down Expand Up @@ -660,50 +654,6 @@ impl Elaborator<'_> {
}
}

/// Checks whether `variable` is `Self::method_name` or `Self::AssociatedConstant` when we are inside a trait impl and `Self`
/// resolves to a primitive type.
///
/// In the first case we elaborate this as if it were a TypePath
/// (for example, if `Self` is `u32` then we consider this the same as `u32::method_name`).
/// A regular path lookup won't work here for the same reason `TypePath` exists.
///
/// In the second case we solve the associated constant by looking up its value, later
/// turning it into a literal.
fn elaborate_variable_as_self_method_or_associated_constant(
&mut self,
variable: &TypedPath,
) -> Option<(ExprId, Type)> {
if !(variable.segments.len() == 2 && variable.segments[0].ident.is_self_type_name()) {
return None;
}

let location = variable.location;
let name = variable.segments[1].ident.as_str();
let self_type = self.self_type.as_ref()?;
let trait_impl_id = &self.current_trait_impl?;

// Check the `Self::AssociatedConstant` case when inside a trait impl
if let Some((definition_id, numeric_type)) =
self.interner.get_trait_impl_associated_constant(*trait_impl_id, name).cloned()
{
let hir_ident = HirIdent::non_trait_method(definition_id, location);
let hir_expr = HirExpression::Ident(hir_ident, None);
let id = self.interner.push_expr(hir_expr);
self.interner.push_expr_location(id, location);
self.interner.push_expr_type(id, numeric_type.clone());
return Some((id, numeric_type));
}

// Check the `Self::method_name` case when `Self` is a primitive type
if matches!(self.self_type, Some(Type::DataType(..))) {
return None;
}

let ident = variable.segments[1].ident.clone();
let typ_location = variable.segments[0].location;
Some(self.elaborate_type_path_impl(self_type.clone(), ident, None, typ_location))
}

pub(crate) fn validate_path(&mut self, path: Path) -> TypedPath {
let mut segments = vecmap(path.segments, |segment| self.validate_path_segment(segment));

Expand Down Expand Up @@ -936,9 +886,6 @@ impl Elaborator<'_> {

self.interner.add_local_reference(hir_ident.id, location);
}
DefinitionKind::AssociatedConstant(..) => {
// Nothing to do here
}
}
}

Expand Down
12 changes: 5 additions & 7 deletions compiler/noirc_frontend/src/elaborator/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -716,12 +716,6 @@
&mut self,
path: &TypedPath,
) -> Option<TraitPathResolution> {
// If we are inside a trait impl, `Self` is known to be a concrete type so we don't have
// to solve the path via trait method lookup.
if self.current_trait_impl.is_some() {
return None;
}

let trait_id = if let Some(current_trait) = self.current_trait {
current_trait
} else {
Expand All @@ -738,7 +732,11 @@
// Allow referring to trait constants via Self:: as well
let definition =
the_trait.find_method_or_constant(method.as_str(), self.interner)?;
let constraint = the_trait.as_constraint(path.location);
let mut constraint = the_trait.as_constraint(path.location);
if let Some(self_type) = &self.self_type {
constraint.typ = self_type.clone();
}

let trait_method = TraitItem { definition, constraint, assumed: true };
let method = TraitPathResolutionMethod::TraitItem(trait_method);
return Some(TraitPathResolution { method, item: None, errors: Vec::new() });
Expand Down Expand Up @@ -2185,7 +2183,7 @@
if existing != new {
panic!(
"Overwriting an existing type binding with a different type!\n {type_var:?} <- {existing:?}\n {type_var:?} <- {new:?}"
);

Check warning on line 2186 in compiler/noirc_frontend/src/elaborator/types.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (intential)
}
}
}
Expand Down
23 changes: 0 additions & 23 deletions compiler/noirc_frontend/src/hir/comptime/interpreter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@
// To match the monomorphizer, we need to call follow_bindings on each of
// the instantiation bindings before we unbind the generics from the previous function.
// This is because the instantiation bindings refer to variables from the call site.
for (_tvar, kind, binding) in instantiation_bindings.values_mut() {

Check warning on line 106 in compiler/noirc_frontend/src/hir/comptime/interpreter.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (tvar)
*kind = kind.follow_bindings();
*binding = binding.follow_bindings();
}
Expand Down Expand Up @@ -264,7 +264,7 @@
}
} else {
let name = self.elaborator.interner.function_name(&function);
unreachable!("Non-builtin, lowlevel or oracle builtin fn '{name}'")

Check warning on line 267 in compiler/noirc_frontend/src/hir/comptime/interpreter.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (lowlevel)
}
}

Expand Down Expand Up @@ -629,29 +629,6 @@
let value = Type::TypeVariable(type_variable.clone());
self.evaluate_numeric_generic(value, numeric_typ, id)
}
DefinitionKind::AssociatedConstant(trait_impl_id, name) => {
let associated_types =
self.elaborator.interner.get_associated_types_for_impl(*trait_impl_id);
let associated_type = associated_types
.iter()
.find(|typ| typ.name.as_str() == name)
.expect("Expected to find associated type");
let Kind::Numeric(numeric_type) = associated_type.typ.kind() else {
unreachable!("Expected associated type to be numeric");
};
let location = self.elaborator.interner.expr_location(&id);
match associated_type
.typ
.evaluate_to_field_element(&associated_type.typ.kind(), location)
{
Ok(value) => self.evaluate_integer(value.into(), id),
Err(err) => Err(InterpreterError::NonIntegerArrayLength {
typ: associated_type.typ.clone(),
err: Some(Box::new(err)),
location,
}),
}
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/hir_def/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ impl Trait {
let ordered = vecmap(&self.generics, |generic| generic.clone().as_named_generic());
let named = vecmap(&self.associated_types, |generic| {
let name = Ident::new(generic.name.to_string(), location);
NamedType { name, typ: generic.clone().as_named_generic() }
NamedType { name, typ: Type::TypeVariable(generic.type_var.clone()) }
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfecher When I initially started this PR some tests were failing. Comparing with master I noticed that when we elaborate AsTraitPath the TraitGenerics we produce there have type variables for named arguments. Here we were returning NamedGeneric and that didn't work (they don't unify the same way as type variables). Given that the AsTraitPath was working well, I decided to change this code to also return TypeVariable. That fixed the issue but I'm not 100% sure this is fine.

});
TraitGenerics { ordered, named }
}
Expand Down
34 changes: 4 additions & 30 deletions compiler/noirc_frontend/src/monomorphization/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1112,32 +1112,6 @@ impl<'interner> Monomorphizer<'interner> {
let value = Type::TypeVariable(type_variable.clone());
self.numeric_generic(value, numeric_typ.as_ref().clone(), typ, location)
}
DefinitionKind::AssociatedConstant(trait_impl_id, name) => {
let location = ident.location;
let associated_types = self.interner.get_associated_types_for_impl(*trait_impl_id);
let associated_type = associated_types
.iter()
.find(|typ| typ.name.as_str() == name)
.expect("Expected to find associated type");
let Kind::Numeric(numeric_type) = associated_type.typ.kind() else {
unreachable!("Expected associated type to be numeric");
};
match associated_type
.typ
.evaluate_to_field_element(&associated_type.typ.kind(), location)
{
Ok(value) => {
let typ = Self::convert_type(&numeric_type, location)?;
let value = SignedField::positive(value);
Ok(ast::Expression::Literal(ast::Literal::Integer(value, typ, location)))
}
Err(err) => Err(MonomorphizationError::CannotComputeAssociatedConstant {
name: name.clone(),
err,
location,
}),
}
}
}
}

Expand Down Expand Up @@ -2587,14 +2561,14 @@ pub(crate) fn resolve_trait_item(
}
}

if let Some((id, expected_type)) = interner.get_trait_impl_associated_constant(impl_id, name) {
let trait_ = interner.get_trait(impl_.trait_id);
if let Some(id) = trait_.find_method_or_constant(name, interner) {
let expected_type = interner.definition_type(id);

// The lookup above returns the expected type but not the value that
// is expected to resolve to a Type::Constant - we have to look that up separately.
for item in interner.get_associated_types_for_impl(impl_id) {
if item.name.as_str() == name {
let id = *id;
let expected_type = expected_type.clone();

// We also need to apply any instantiation bindings if the expression has any
let instantiation_bindings = interner.try_get_instantiation_bindings(expr_id);
let value = if let Some(instantiation_bindings) = instantiation_bindings {
Expand Down
35 changes: 0 additions & 35 deletions compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,6 @@
/// created, when resolving the type signature of each method in the impl.
trait_impl_associated_types: HashMap<TraitImplId, Vec<NamedType>>,

trait_impl_associated_constants: HashMap<TraitImplId, HashMap<String, (DefinitionId, Type)>>,

/// Trait implementations on each type. This is expected to always have the same length as
/// `self.trait_implementations`.
///
Expand Down Expand Up @@ -220,12 +218,12 @@
interned_statement_kinds: noirc_arena::Arena<StatementKind>,

// Interned `UnresolvedTypeData`s during comptime code.
interned_unresolved_type_datas: noirc_arena::Arena<UnresolvedTypeData>,

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

View workflow job for this annotation

GitHub Actions / Code

Unknown word (datas)

// Interned `Pattern`s during comptime code.
interned_patterns: noirc_arena::Arena<Pattern>,

/// Determins whether to run in LSP mode. In LSP mode references are tracked.

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

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Determins)
pub(crate) lsp_mode: bool,

/// Store the location of the references in the graph.
Expand Down Expand Up @@ -597,8 +595,6 @@
/// Generic types in functions (T, U in `fn foo<T, U>(...)` are declared as variables
/// in scope in case they resolve to numeric generics later.
NumericGeneric(TypeVariable, Box<Type>),

AssociatedConstant(TraitImplId, String),
}

impl DefinitionKind {
Expand All @@ -614,7 +610,6 @@
DefinitionKind::Global(_) => None,
DefinitionKind::Local(id) => *id,
DefinitionKind::NumericGeneric(_, _) => None,
DefinitionKind::AssociatedConstant(_, _) => None,
}
}
}
Expand Down Expand Up @@ -710,7 +705,7 @@
quoted_types: Default::default(),
interned_expression_kinds: Default::default(),
interned_statement_kinds: Default::default(),
interned_unresolved_type_datas: Default::default(),

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

View workflow job for this annotation

GitHub Actions / Code

Unknown word (datas)
interned_patterns: Default::default(),
lsp_mode: false,
location_indices: LocationIndices::default(),
Expand All @@ -720,7 +715,6 @@
auto_import_names: HashMap::default(),
comptime_scopes: vec![HashMap::default()],
trait_impl_associated_types: HashMap::default(),
trait_impl_associated_constants: HashMap::default(),
doc_comments: HashMap::default(),
reexports: HashMap::default(),
}
Expand Down Expand Up @@ -2307,11 +2301,11 @@
&mut self,
typ: UnresolvedTypeData,
) -> InternedUnresolvedTypeData {
InternedUnresolvedTypeData(self.interned_unresolved_type_datas.insert(typ))

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

View workflow job for this annotation

GitHub Actions / Code

Unknown word (datas)
}

pub fn get_unresolved_type_data(&self, id: InternedUnresolvedTypeData) -> &UnresolvedTypeData {
&self.interned_unresolved_type_datas[id.0]

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

View workflow job for this annotation

GitHub Actions / Code

Unknown word (datas)
}

/// Returns the type of an operator (which is always a function), along with its return type.
Expand Down Expand Up @@ -2357,26 +2351,6 @@
impl_id: TraitImplId,
associated_types: Vec<NamedType>,
) {
// Wrap the named generics in type variables to be able to refer them as type variables
for associated_type in &associated_types {
let Kind::Numeric(numeric_type) = associated_type.typ.kind() else {
continue;
};

let name = associated_type.name.to_string();
let definition_id = self.push_definition(
associated_type.name.to_string(),
false,
false,
DefinitionKind::AssociatedConstant(impl_id, name.clone()),
associated_type.name.location(),
);
self.trait_impl_associated_constants
.entry(impl_id)
.or_default()
.insert(name, (definition_id, *numeric_type));
}

self.trait_impl_associated_types.insert(impl_id, associated_types);
}

Expand All @@ -2395,15 +2369,6 @@
types.iter().find(|typ| typ.name.as_str() == type_name).map(|typ| &typ.typ)
}

/// Returns the definition id for the associated constant of the given type variable.
pub fn get_trait_impl_associated_constant(
&self,
impl_id: TraitImplId,
name: &str,
) -> Option<&(DefinitionId, Type)> {
self.trait_impl_associated_constants.get(&impl_id).and_then(|map| map.get(name))
}

/// Return a set of TypeBindings to bind types from the parent trait to those from the trait impl.
pub fn trait_to_impl_bindings(
&self,
Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_frontend/src/tests/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1521,7 +1521,7 @@ fn accesses_associated_type_inside_trait_impl_using_self() {
let _ = i32::foo();
}
"#;
assert_no_errors!(src);
check_monomorphization_error!(src);
}

#[named]
Expand All @@ -1544,7 +1544,7 @@ fn accesses_associated_type_inside_trait_using_self() {
let _ = i32::foo();
}
"#;
assert_no_errors!(src);
check_monomorphization_error!(src);
}

#[named]
Expand Down
3 changes: 2 additions & 1 deletion tooling/nargo_cli/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@
];

/// `nargo interpret` ignored tests, either because they don't currently work or
/// becuase they are too slow to run.

Check warning on line 124 in tooling/nargo_cli/build.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (becuase)
const IGNORED_INTERPRET_EXECUTION_TESTS: [&str; 1] = [
// slow
"regression_4709",
Expand Down Expand Up @@ -212,7 +212,7 @@

/// These tests are ignored because of existing bugs in `nargo expand`.
/// As the bugs are fixed these tests should be removed from this list.
const IGNORED_NARGO_EXPAND_COMPILE_SUCCESS_NO_BUG_TESTS: [&str; 19] = [
const IGNORED_NARGO_EXPAND_COMPILE_SUCCESS_NO_BUG_TESTS: [&str; 20] = [
"noirc_frontend_tests_arithmetic_generics_checked_casts_do_not_prevent_canonicalization",
"noirc_frontend_tests_check_trait_as_type_as_fn_parameter",
"noirc_frontend_tests_check_trait_as_type_as_two_fn_parameters",
Expand All @@ -232,6 +232,7 @@
"noirc_frontend_tests_u32_globals_as_sizes_in_types",
"noirc_frontend_tests_unused_items_considers_struct_as_constructed_if_trait_method_is_called",
"noirc_frontend_tests_traits_associated_constant_of_generic_type_used_in_expression",
"noirc_frontend_tests_check_trait_implemented_for_all_t",
];

const IGNORED_NARGO_EXPAND_COMPILE_SUCCESS_WITH_BUG_TESTS: [&str; 1] =
Expand Down Expand Up @@ -792,7 +793,7 @@
writeln!(test_file, "}}").unwrap();
}

/// Here we check, for every program in `test_programs/exeuction_success`, that:

Check warning on line 796 in tooling/nargo_cli/build.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (exeuction)
/// 1. `nargo expand` works on it
/// 2. That the output of the original program is the same as the output of the expanded program
/// (that is, we run `nargo execute` on the original program and the expanded program and compare the output)
Expand Down
4 changes: 1 addition & 3 deletions tooling/nargo_expand/src/printer/hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -760,9 +760,7 @@ impl ItemPrinter<'_, '_> {
use_import,
);
}
DefinitionKind::Local(..)
| DefinitionKind::NumericGeneric(..)
| DefinitionKind::AssociatedConstant(..) => {
DefinitionKind::Local(..) | DefinitionKind::NumericGeneric(..) => {
let name = self.interner.definition_name(ident.id);

// The compiler uses '$' for some internal identifiers.
Expand Down
Loading