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: allow multiple _ parameters, and disallow _ as an expression you can read from #6657

Merged
merged 2 commits into from
Dec 2, 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
71 changes: 50 additions & 21 deletions compiler/noirc_frontend/src/debug/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,16 @@
self.insert_state_set_oracle(module, 8);
}

fn insert_var(&mut self, var_name: &str) -> SourceVarId {
fn insert_var(&mut self, var_name: &str) -> Option<SourceVarId> {
if var_name == "_" {
return None;
}

let var_id = SourceVarId(self.next_var_id);
self.next_var_id += 1;
self.variables.insert(var_id, var_name.to_string());
self.scope.last_mut().unwrap().insert(var_name.to_string(), var_id);
var_id
Some(var_id)
}

fn lookup_var(&self, var_name: &str) -> Option<SourceVarId> {
Expand Down Expand Up @@ -107,9 +111,9 @@
.flat_map(|param| {
pattern_vars(&param.pattern)
.iter()
.map(|(id, _is_mut)| {
let var_id = self.insert_var(&id.0.contents);
build_assign_var_stmt(var_id, id_expr(id))
.filter_map(|(id, _is_mut)| {
let var_id = self.insert_var(&id.0.contents)?;
Some(build_assign_var_stmt(var_id, id_expr(id)))
})
.collect::<Vec<_>>()
})
Expand Down Expand Up @@ -225,13 +229,28 @@
}
})
.collect();
let vars_exprs: Vec<ast::Expression> = vars.iter().map(|(id, _)| id_expr(id)).collect();
let vars_exprs: Vec<ast::Expression> = vars
.iter()
.map(|(id, _)| {
// We don't want to generate an expression to read from "_".
// And since this expression is going to be assigned to "_" so it doesn't matter
// what it is, we can use `()` for it.
if id.0.contents == "_" {
ast::Expression {
kind: ast::ExpressionKind::Literal(ast::Literal::Unit),
span: id.span(),
}
} else {
id_expr(id)
}
})
.collect();

let mut block_stmts =
vec![ast::Statement { kind: ast::StatementKind::Let(let_stmt.clone()), span: *span }];
block_stmts.extend(vars.iter().map(|(id, _)| {
let var_id = self.insert_var(&id.0.contents);
build_assign_var_stmt(var_id, id_expr(id))
block_stmts.extend(vars.iter().filter_map(|(id, _)| {
let var_id = self.insert_var(&id.0.contents)?;
Some(build_assign_var_stmt(var_id, id_expr(id)))
}));
block_stmts.push(ast::Statement {
kind: ast::StatementKind::Expression(ast::Expression {
Expand Down Expand Up @@ -289,7 +308,7 @@
}
ast::LValue::Dereference(_lv, span) => {
// TODO: this is a dummy statement for now, but we should
// somehow track the derefence and update the pointed to

Check warning on line 311 in compiler/noirc_frontend/src/debug/mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (derefence)
// variable
ast::Statement {
kind: ast::StatementKind::Expression(uint_expr(0, *span)),
Expand Down Expand Up @@ -422,21 +441,31 @@
let var_name = &for_stmt.identifier.0.contents;
let var_id = self.insert_var(var_name);

let set_stmt = build_assign_var_stmt(var_id, id_expr(&for_stmt.identifier));
let drop_stmt = build_drop_var_stmt(var_id, Span::empty(for_stmt.span.end()));
let set_and_drop_stmt = var_id.map(|var_id| {
(
build_assign_var_stmt(var_id, id_expr(&for_stmt.identifier)),
build_drop_var_stmt(var_id, Span::empty(for_stmt.span.end())),
)
});

self.walk_expr(&mut for_stmt.block);

let mut statements = Vec::new();
let block_statement = ast::Statement {
kind: ast::StatementKind::Semi(for_stmt.block.clone()),
span: for_stmt.block.span,
};

if let Some((set_stmt, drop_stmt)) = set_and_drop_stmt {
statements.push(set_stmt);
statements.push(block_statement);
statements.push(drop_stmt);
} else {
statements.push(block_statement);
}

for_stmt.block = ast::Expression {
kind: ast::ExpressionKind::Block(ast::BlockExpression {
statements: vec![
set_stmt,
ast::Statement {
kind: ast::StatementKind::Semi(for_stmt.block.clone()),
span: for_stmt.block.span,
},
drop_stmt,
],
}),
kind: ast::ExpressionKind::Block(ast::BlockExpression { statements }),
span: for_stmt.span,
};
}
Expand Down Expand Up @@ -675,9 +704,9 @@
ast::Pattern::Tuple(patterns, _) => {
stack.extend(patterns.iter().map(|pattern| (pattern, false)));
}
ast::Pattern::Struct(_, pids, _) => {

Check warning on line 707 in compiler/noirc_frontend/src/debug/mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (pids)
stack.extend(pids.iter().map(|(_, pattern)| (pattern, is_mut)));

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

View workflow job for this annotation

GitHub Actions / Code

Unknown word (pids)
vars.extend(pids.iter().map(|(id, _)| (id.clone(), false)));

Check warning on line 709 in compiler/noirc_frontend/src/debug/mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (pids)
}
ast::Pattern::Interned(_, _) => (),
}
Expand All @@ -688,7 +717,7 @@
fn pattern_to_string(pattern: &ast::Pattern) -> String {
match pattern {
ast::Pattern::Identifier(id) => id.0.contents.clone(),
ast::Pattern::Mutable(mpat, _, _) => format!("mut {}", pattern_to_string(mpat.as_ref())),

Check warning on line 720 in compiler/noirc_frontend/src/debug/mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (mpat)

Check warning on line 720 in compiler/noirc_frontend/src/debug/mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (mpat)
ast::Pattern::Tuple(elements, _) => format!(
"({})",
elements.iter().map(pattern_to_string).collect::<Vec<String>>().join(", ")
Expand Down
3 changes: 3 additions & 0 deletions compiler/noirc_frontend/src/elaborator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,9 @@ impl<'context> Elaborator<'context> {
// so we need to reintroduce the same IDs into scope here.
for parameter in &func_meta.parameter_idents {
let name = self.interner.definition_name(parameter.id).to_owned();
if name == "_" {
continue;
}
let warn_if_unused = !(func_meta.trait_impl.is_some() && name == "self");
self.add_existing_variable_to_scope(name, parameter.clone(), warn_if_unused);
}
Expand Down
22 changes: 12 additions & 10 deletions compiler/noirc_frontend/src/elaborator/patterns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -331,16 +331,18 @@ impl<'context> Elaborator<'context> {
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.clone(), resolver_meta);

if !allow_shadowing {
if let Some(old_value) = old_value {
self.push_err(ResolverError::DuplicateDefinition {
name,
first_span: old_value.ident.location.span,
second_span: location.span,
});
if name != "_" {
let scope = self.scopes.get_mut_scope();
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,
first_span: old_value.ident.location.span,
second_span: location.span,
});
}
}
}

Expand Down
20 changes: 15 additions & 5 deletions compiler/noirc_frontend/src/hir/resolution/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,11 +223,21 @@ impl<'a> From<&'a ResolverError> for Diagnostic {
*span,
)
}
ResolverError::VariableNotDeclared { name, span } => Diagnostic::simple_error(
format!("cannot find `{name}` in this scope "),
"not found in this scope".to_string(),
*span,
),
ResolverError::VariableNotDeclared { name, span } => {
if name == "_" {
Diagnostic::simple_error(
"in expressions, `_` can only be used on the left-hand side of an assignment".to_string(),
"`_` not allowed here".to_string(),
*span,
)
} else {
Diagnostic::simple_error(
format!("cannot find `{name}` in this scope"),
"not found in this scope".to_string(),
*span,
)
}
},
ResolverError::PathIsNotIdent { span } => Diagnostic::simple_error(
"cannot use path as an identifier".to_string(),
String::new(),
Expand Down
30 changes: 30 additions & 0 deletions compiler/noirc_frontend/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2002,7 +2002,7 @@
}

// TODO(https://github.com/noir-lang/noir/issues/6238):
// The EvaluatedGlobalIsntU32 warning is a stopgap

Check warning on line 2005 in compiler/noirc_frontend/src/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Isnt)
// (originally from https://github.com/noir-lang/noir/issues/6125)
#[test]
fn numeric_generic_field_larger_than_u32() {
Expand All @@ -2019,7 +2019,7 @@
assert_eq!(errors.len(), 2);
assert!(matches!(
errors[0].0,
CompilationError::TypeError(TypeCheckError::EvaluatedGlobalIsntU32 { .. }),

Check warning on line 2022 in compiler/noirc_frontend/src/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Isnt)
));
assert!(matches!(
errors[1].0,
Expand All @@ -2028,7 +2028,7 @@
}

// TODO(https://github.com/noir-lang/noir/issues/6238):
// The EvaluatedGlobalIsntU32 warning is a stopgap

Check warning on line 2031 in compiler/noirc_frontend/src/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Isnt)
// (originally from https://github.com/noir-lang/noir/issues/6126)
#[test]
fn numeric_generic_field_arithmetic_larger_than_u32() {
Expand Down Expand Up @@ -2057,7 +2057,7 @@

assert!(matches!(
errors[0].0,
CompilationError::TypeError(TypeCheckError::EvaluatedGlobalIsntU32 { .. }),

Check warning on line 2060 in compiler/noirc_frontend/src/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Isnt)
));

assert!(matches!(
Expand Down Expand Up @@ -3845,3 +3845,33 @@
));
});
}

#[test]
fn allows_multiple_underscore_parameters() {
let src = r#"
pub fn foo(_: i32, _: i64) {}

fn main() {}
"#;
assert_no_errors(src);
}

#[test]
fn disallows_underscore_on_right_hand_side() {
let src = r#"
fn main() {
let _ = 1;
let _x = _;
}
"#;
let errors = get_program_errors(src);
assert_eq!(errors.len(), 1);

let CompilationError::ResolverError(ResolverError::VariableNotDeclared { name, .. }) =
&errors[0].0
else {
panic!("Expected a VariableNotDeclared error, got {:?}", errors[0].0);
};

assert_eq!(name, "_");
}