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: ICE when using a comptime let variable in runtime code #5391

Merged
merged 2 commits into from
Jul 3, 2024
Merged
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
1 change: 1 addition & 0 deletions aztec_macros/src/utils/hir_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@
file_id,
global.attributes.clone(),
false,
false,
);

// Add the statement to the scope so its path can be looked up later
Expand Down Expand Up @@ -359,7 +360,7 @@
}
}

pub fn get_global_numberic_const(

Check warning on line 363 in aztec_macros/src/utils/hir_utils.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (numberic)
context: &HirContext,
const_name: &str,
) -> Result<u128, MacroError> {
Expand Down
33 changes: 24 additions & 9 deletions compiler/noirc_frontend/src/elaborator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,6 @@ pub struct Elaborator<'context> {
/// Each constraint in the `where` clause of the function currently being resolved.
trait_bounds: Vec<TraitConstraint>,

current_function: Option<FuncId>,

jfecher marked this conversation as resolved.
Show resolved Hide resolved
/// This is a stack of function contexts. Most of the time, for each function we
/// expect this to be of length one, containing each type variable and trait constraint
/// used in the function. This is also pushed to when a `comptime {}` block is used within
Expand Down Expand Up @@ -196,7 +194,6 @@ impl<'context> Elaborator<'context> {
crate_id,
resolving_ids: BTreeSet::new(),
trait_bounds: Vec::new(),
current_function: None,
function_context: vec![FunctionContext::default()],
current_trait_impl: None,
comptime_scopes: vec![HashMap::default()],
Expand Down Expand Up @@ -329,8 +326,6 @@ impl<'context> Elaborator<'context> {
FunctionBody::Resolving => return,
};

let old_function = std::mem::replace(&mut self.current_function, Some(id));

self.scopes.start_function();
let old_item = std::mem::replace(&mut self.current_item, Some(DependencyId::Function(id)));

Expand Down Expand Up @@ -407,7 +402,6 @@ impl<'context> Elaborator<'context> {

self.trait_bounds.clear();
self.interner.update_fn(id, hir_func);
self.current_function = old_function;
self.current_item = old_item;
}

Expand Down Expand Up @@ -615,8 +609,6 @@ impl<'context> Elaborator<'context> {
func_id: FuncId,
is_trait_function: bool,
) {
self.current_function = Some(func_id);

let in_contract = if self.self_type.is_some() {
// Without this, impl methods can accidentally be placed in contracts.
// See: https://github.com/noir-lang/noir/issues/3254
Expand Down Expand Up @@ -738,7 +730,6 @@ impl<'context> Elaborator<'context> {
};

self.interner.push_fn_meta(meta, func_id);
self.current_function = None;
self.scopes.end_function();
self.current_item = None;
}
Expand Down Expand Up @@ -1468,6 +1459,30 @@ impl<'context> Elaborator<'context> {
}
}

/// True if we're currently within a `comptime` block, function, or global
fn in_comptime_context(&self) -> bool {
// The first context is the global context, followed by the function-specific context.
// Any context after that is a `comptime {}` block's.
if self.function_context.len() > 2 {
return true;
}

match self.current_item {
Some(DependencyId::Function(id)) => self.interner.function_modifiers(&id).is_comptime,
Some(DependencyId::Global(id)) => self.interner.get_global_definition(id).comptime,
_ => false,
}
}

/// True if we're currently within a constrained function.
/// Defaults to `true` if the current function is unknown.
fn in_constrained_function(&self) -> bool {
self.current_item.map_or(true, |id| match id {
DependencyId::Function(id) => !self.interner.function_modifiers(&id).is_unconstrained,
_ => true,
})
}

/// Filters out comptime items from non-comptime items.
/// Returns a pair of (comptime items, non-comptime items)
fn filter_comptime_items(mut items: CollectedItems) -> (CollectedItems, CollectedItems) {
Expand Down
26 changes: 21 additions & 5 deletions compiler/noirc_frontend/src/elaborator/patterns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use rustc_hash::FxHashSet as HashSet;
use crate::{
ast::ERROR_IDENT,
hir::{
comptime::Interpreter,
def_collector::dc_crate::CompilationError,
resolution::errors::ResolverError,
type_check::{Source, TypeCheckError},
Expand Down Expand Up @@ -279,19 +280,21 @@ impl<'context> Elaborator<'context> {
}

let location = Location::new(name.span(), self.file);
let name = name.0.contents;
let comptime = self.in_comptime_context();
let id =
self.interner.push_definition(name.0.contents.clone(), mutable, definition, location);
self.interner.push_definition(name.clone(), mutable, comptime, definition, location);
let ident = HirIdent::non_trait_method(id, location);
let resolver_meta =
ResolverMeta { num_times_used: 0, ident: ident.clone(), warn_if_unused };

let scope = self.scopes.get_mut_scope();
let old_value = scope.add_key_value(name.0.contents.clone(), resolver_meta);
let old_value = scope.add_key_value(name.clone(), resolver_meta);

if !allow_shadowing {
if let Some(old_value) = old_value {
self.push_err(ResolverError::DuplicateDefinition {
name: name.0.contents,
name,
first_span: old_value.ident.location.span,
second_span: location.span,
});
Expand Down Expand Up @@ -323,6 +326,7 @@ impl<'context> Elaborator<'context> {
name: Ident,
definition: DefinitionKind,
) -> HirIdent {
let comptime = self.in_comptime_context();
let scope = self.scopes.get_mut_scope();

// This check is necessary to maintain the same definition ids in the interner. Currently, each function uses a new resolver that has its own ScopeForest and thus global scope.
Expand All @@ -344,8 +348,8 @@ impl<'context> Elaborator<'context> {
(hir_ident, resolver_meta)
} else {
let location = Location::new(name.span(), self.file);
let id =
self.interner.push_definition(name.0.contents.clone(), false, definition, location);
let name = name.0.contents.clone();
let id = self.interner.push_definition(name, false, comptime, definition, location);
let ident = HirIdent::non_trait_method(id, location);
let resolver_meta =
ResolverMeta { num_times_used: 0, ident: ident.clone(), warn_if_unused: true };
Expand Down Expand Up @@ -394,12 +398,24 @@ impl<'context> Elaborator<'context> {
) -> (ExprId, Type) {
let span = variable.span;
let expr = self.resolve_variable(variable);
let definition_id = expr.id;

let id = self.interner.push_expr(HirExpression::Ident(expr.clone(), generics.clone()));

self.interner.push_expr_location(id, span, self.file);
let typ = self.type_check_variable(expr, id, generics);
self.interner.push_expr_type(id, typ.clone());

// Comptime variables must be replaced with their values
if let Some(definition) = self.interner.try_definition(definition_id) {
if definition.comptime && !self.in_comptime_context() {
let mut interpreter =
Interpreter::new(self.interner, &mut self.comptime_scopes, self.crate_id);
let value = interpreter.evaluate(id);
return self.inline_comptime_value(value, span);
}
}

(id, typ)
}

Expand Down
5 changes: 2 additions & 3 deletions compiler/noirc_frontend/src/elaborator/statements.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,9 +206,8 @@ impl<'context> Elaborator<'context> {
}

fn elaborate_jump(&mut self, is_break: bool, span: noirc_errors::Span) -> (HirStatement, Type) {
let in_constrained_function = self
.current_function
.map_or(true, |func_id| !self.interner.function_modifiers(&func_id).is_unconstrained);
let in_constrained_function = self.in_constrained_function();

if in_constrained_function {
self.push_err(ResolverError::JumpInConstrainedFn { is_break, span });
}
Expand Down
17 changes: 10 additions & 7 deletions compiler/noirc_frontend/src/elaborator/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ use crate::{
HirExpression, HirLiteral, HirStatement, Path, PathKind, SecondaryAttribute, Signedness,
UnaryOp, UnresolvedType, UnresolvedTypeData,
},
node_interner::{DefinitionKind, ExprId, GlobalId, TraitId, TraitImplKind, TraitMethodId},
node_interner::{
DefinitionKind, DependencyId, ExprId, GlobalId, TraitId, TraitImplKind, TraitMethodId,
},
Generics, Kind, ResolvedGeneric, Type, TypeBinding, TypeVariable, TypeVariableKind,
};

Expand Down Expand Up @@ -1160,9 +1162,11 @@ impl<'context> Elaborator<'context> {
None
}
Type::NamedGeneric(_, _, _) => {
let func_meta = self.interner.function_meta(
&self.current_function.expect("unexpected method outside a function"),
);
let func_id = match self.current_item {
Some(DependencyId::Function(id)) => id,
_ => panic!("unexpected method outside a function"),
};
let func_meta = self.interner.function_meta(&func_id);

for constraint in &func_meta.trait_constraints {
if *object_type == constraint.typ {
Expand Down Expand Up @@ -1233,9 +1237,8 @@ impl<'context> Elaborator<'context> {
lints::deprecated_function(elaborator.interner, call.func).map(Into::into)
});

let func_mod = self.current_function.map(|func| self.interner.function_modifiers(&func));
let is_current_func_constrained =
func_mod.map_or(true, |func_mod| !func_mod.is_unconstrained);
let is_current_func_constrained = self.in_constrained_function();

let is_unconstrained_call = self.is_unconstrained_call(call.func);
let crossing_runtime_boundary = is_current_func_constrained && is_unconstrained_call;
if crossing_runtime_boundary {
Expand Down
2 changes: 2 additions & 0 deletions compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,7 @@
self.file_id,
vec![],
false,
false,
);

if let Err((first_def, second_def)) = self.def_collector.def_map.modules
Expand All @@ -489,7 +490,7 @@
[trait_id.0.local_id.0]
.declare_type_alias(name.clone(), TypeAliasId::dummy_id())
{
let error = DefCollectorErrorKind::Duplicate {

Check warning on line 493 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 493 in compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (alexvitkov)
typ: DuplicateType::TraitAssociatedType,
first_def,
second_def,
Expand Down Expand Up @@ -806,6 +807,7 @@
file_id,
global.attributes.clone(),
matches!(global.pattern, Pattern::Mutable { .. }),
global.comptime,
);

// Add the statement to the scope so its path can be looked up later
Expand Down
8 changes: 4 additions & 4 deletions compiler/noirc_frontend/src/hir/resolution/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -399,8 +399,8 @@ impl<'a> Resolver<'a> {
}

let location = Location::new(name.span(), self.file);
let id =
self.interner.push_definition(name.0.contents.clone(), mutable, definition, location);
let var_name = name.0.contents.clone();
let id = self.interner.push_definition(var_name, mutable, false, definition, location);
let ident = HirIdent::non_trait_method(id, location);
let resolver_meta =
ResolverMeta { num_times_used: 0, ident: ident.clone(), warn_if_unused };
Expand Down Expand Up @@ -445,8 +445,8 @@ impl<'a> Resolver<'a> {
(hir_ident, resolver_meta)
} else {
let location = Location::new(name.span(), self.file);
let id =
self.interner.push_definition(name.0.contents.clone(), false, definition, location);
let var_name = name.0.contents.clone();
let id = self.interner.push_definition(var_name, false, false, definition, location);
let ident = HirIdent::non_trait_method(id, location);
let resolver_meta =
ResolverMeta { num_times_used: 0, ident: ident.clone(), warn_if_unused: true };
Expand Down
29 changes: 22 additions & 7 deletions compiler/noirc_frontend/src/hir/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -483,19 +483,34 @@ pub mod test {
// let z = x + y;
//
// Push x variable
let x_id =
interner.push_definition("x".into(), false, DefinitionKind::Local(None), location);
let x_id = interner.push_definition(
"x".into(),
false,
false,
DefinitionKind::Local(None),
location,
);

let x = HirIdent::non_trait_method(x_id, location);

// Push y variable
let y_id =
interner.push_definition("y".into(), false, DefinitionKind::Local(None), location);
let y_id = interner.push_definition(
"y".into(),
false,
false,
DefinitionKind::Local(None),
location,
);
let y = HirIdent::non_trait_method(y_id, location);

// Push z variable
let z_id =
interner.push_definition("z".into(), false, DefinitionKind::Local(None), location);
let z_id = interner.push_definition(
"z".into(),
false,
false,
DefinitionKind::Local(None),
location,
);
let z = HirIdent::non_trait_method(z_id, location);

// Push x and y as expressions
Expand Down Expand Up @@ -531,7 +546,7 @@ pub mod test {
let func_id = interner.push_fn(func);

let definition = DefinitionKind::Local(None);
let id = interner.push_definition("test_func".into(), false, definition, location);
let id = interner.push_definition("test_func".into(), false, false, definition, location);
let name = HirIdent::non_trait_method(id, location);

// Add function meta
Expand Down
15 changes: 11 additions & 4 deletions compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@
/// into `quoted` expressions, we preserve the original type by assigning it a unique id
/// and creating a `Token::QuotedType(id)` from this id. We cannot create a token holding
/// the actual type since types do not implement Send or Sync.
quoted_types: noirc_arena::Arena<Type>,

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

View workflow job for this annotation

GitHub Actions / Code

Unknown word (metaprogramming)

/// Store the location of the references in the graph
pub(crate) reference_graph: DiGraph<DependencyId, ()>,
Expand Down Expand Up @@ -438,6 +438,7 @@
pub struct DefinitionInfo {
pub name: String,
pub mutable: bool,
pub comptime: bool,
pub kind: DefinitionKind,
pub location: Location,
}
Expand Down Expand Up @@ -697,6 +698,7 @@
self.type_ref_locations.push((typ, location));
}

#[allow(clippy::too_many_arguments)]
fn push_global(
&mut self,
ident: Ident,
Expand All @@ -705,12 +707,13 @@
file: FileId,
attributes: Vec<SecondaryAttribute>,
mutable: bool,
comptime: bool,
) -> GlobalId {
let id = GlobalId(self.globals.len());
let location = Location::new(ident.span(), file);
let name = ident.to_string();
let definition_id =
self.push_definition(name, mutable, DefinitionKind::Global(id), location);
self.push_definition(name, mutable, comptime, DefinitionKind::Global(id), location);

self.globals.push(GlobalInfo {
id,
Expand All @@ -737,10 +740,11 @@
file: FileId,
attributes: Vec<SecondaryAttribute>,
mutable: bool,
comptime: bool,
) -> GlobalId {
let statement = self.push_stmt(HirStatement::Error);
let span = name.span();
let id = self.push_global(name, local_id, statement, file, attributes, mutable);
let id = self.push_global(name, local_id, statement, file, attributes, mutable, comptime);
self.push_stmt_location(statement, span, file);
id
}
Expand Down Expand Up @@ -784,6 +788,7 @@
&mut self,
name: String,
mutable: bool,
comptime: bool,
definition: DefinitionKind,
location: Location,
) -> DefinitionId {
Expand All @@ -792,7 +797,8 @@
self.function_definition_ids.insert(func_id, id);
}

self.definitions.push(DefinitionInfo { name, mutable, kind: definition, location });
let kind = definition;
self.definitions.push(DefinitionInfo { name, mutable, comptime, kind, location });
id
}

Expand Down Expand Up @@ -840,9 +846,10 @@
location: Location,
) -> DefinitionId {
let name = modifiers.name.clone();
let comptime = modifiers.is_comptime;
self.function_modifiers.insert(func, modifiers);
self.function_modules.insert(func, module);
self.push_definition(name, false, DefinitionKind::Function(func), location)
self.push_definition(name, false, comptime, DefinitionKind::Function(func), location)
}

pub fn set_function_trait(&mut self, func: FuncId, self_type: Type, trait_id: TraitId) {
Expand Down
11 changes: 11 additions & 0 deletions compiler/noirc_frontend/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1896,3 +1896,14 @@ fn quote_code_fragments() {
use InterpreterError::FailingConstraint;
assert!(matches!(&errors[0].0, CompilationError::InterpreterError(FailingConstraint { .. })));
}

// Regression for #5388
#[test]
fn comptime_let() {
let src = r#"fn main() {
comptime let my_var = 2;
assert_eq(my_var, 2);
}"#;
let errors = get_program_errors(src);
assert_eq!(errors.len(), 0);
}
Loading