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

feat: allow silencing an unused variable defined via let #6149

Merged
merged 5 commits into from
Sep 25, 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
5 changes: 4 additions & 1 deletion compiler/noirc_frontend/src/ast/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,13 +141,14 @@ impl StatementKind {
pattern: Pattern,
r#type: UnresolvedType,
expression: Expression,
attributes: Vec<SecondaryAttribute>,
) -> StatementKind {
StatementKind::Let(LetStatement {
pattern,
r#type,
expression,
comptime: false,
attributes: vec![],
attributes,
})
}

Expand Down Expand Up @@ -814,6 +815,7 @@ impl ForRange {
Pattern::Identifier(array_ident.clone()),
UnresolvedTypeData::Unspecified.with_span(Default::default()),
array,
vec![],
),
span: array_span,
};
Expand Down Expand Up @@ -858,6 +860,7 @@ impl ForRange {
Pattern::Identifier(identifier),
UnresolvedTypeData::Unspecified.with_span(Default::default()),
Expression::new(loop_element, array_span),
vec![],
),
span: array_span,
};
Expand Down
5 changes: 5 additions & 0 deletions compiler/noirc_frontend/src/ast/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
Struct,
Trait,
Function,
Let,
}

/// Implements the [Visitor pattern](https://en.wikipedia.org/wiki/Visitor_pattern) for Noir's AST.
Expand Down Expand Up @@ -1097,6 +1098,10 @@

impl LetStatement {
pub fn accept(&self, visitor: &mut impl Visitor) {
for attribute in &self.attributes {
attribute.accept(AttributeTarget::Let, visitor);
}

if visitor.visit_let_statement(self) {
self.accept_children(visitor);
}
Expand Down Expand Up @@ -1291,8 +1296,8 @@
UnresolvedTypeData::Unspecified => visitor.visit_unspecified_type(self.span),
UnresolvedTypeData::Quoted(typ) => visitor.visit_quoted_type(typ, self.span),
UnresolvedTypeData::FieldElement => visitor.visit_field_element_type(self.span),
UnresolvedTypeData::Integer(signdness, size) => {

Check warning on line 1299 in compiler/noirc_frontend/src/ast/visitor.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (signdness)
visitor.visit_integer_type(*signdness, *size, self.span);

Check warning on line 1300 in compiler/noirc_frontend/src/ast/visitor.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (signdness)
}
UnresolvedTypeData::Bool => visitor.visit_bool_type(self.span),
UnresolvedTypeData::Unit => visitor.visit_unit_type(self.span),
Expand Down
3 changes: 3 additions & 0 deletions compiler/noirc_frontend/src/debug/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@
ast::Pattern::Identifier(ident("__debug_expr", ret_expr.span)),
ast::UnresolvedTypeData::Unspecified.with_span(Default::default()),
ret_expr.clone(),
vec![],
),
span: ret_expr.span,
};
Expand Down Expand Up @@ -249,6 +250,7 @@
}),
span: let_stmt.expression.span,
},
vec![],
),
span: *span,
}
Expand All @@ -274,6 +276,7 @@
ast::Pattern::Identifier(ident("__debug_expr", assign_stmt.expression.span)),
ast::UnresolvedTypeData::Unspecified.with_span(Default::default()),
assign_stmt.expression.clone(),
vec![],
);
let expression_span = assign_stmt.expression.span;
let new_assign_stmt = match &assign_stmt.lvalue {
Expand All @@ -285,7 +288,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 291 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 @@ -671,9 +674,9 @@
ast::Pattern::Tuple(patterns, _) => {
stack.extend(patterns.iter().map(|pattern| (pattern, false)));
}
ast::Pattern::Struct(_, pids, _) => {

Check warning on line 677 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 678 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 679 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 @@ -684,7 +687,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 690 in compiler/noirc_frontend/src/debug/mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (mpat)

Check warning on line 690 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
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/elaborator/expressions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -790,7 +790,7 @@ impl<'context> Elaborator<'context> {
let parameter = DefinitionKind::Local(None);
let typ = self.resolve_inferred_type(typ);
arg_types.push(typ.clone());
(self.elaborate_pattern(pattern, typ.clone(), parameter), typ)
(self.elaborate_pattern(pattern, typ.clone(), parameter, true), typ)
});

let return_type = self.resolve_inferred_type(lambda.return_type);
Expand Down
9 changes: 7 additions & 2 deletions compiler/noirc_frontend/src/elaborator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -362,8 +362,12 @@ impl<'context> Elaborator<'context> {
if let Kind::Numeric(typ) = &generic.kind {
let definition = DefinitionKind::GenericType(generic.type_var.clone());
let ident = Ident::new(generic.name.to_string(), generic.span);
let hir_ident =
self.add_variable_decl_inner(ident, false, false, false, definition);
let hir_ident = self.add_variable_decl(
ident, false, // mutable
false, // allow_shadowing
false, // warn_if_unused
definition,
);
self.interner.push_definition_type(hir_ident.id, *typ.clone());
}
}
Expand Down Expand Up @@ -760,6 +764,7 @@ impl<'context> Elaborator<'context> {
typ.clone(),
DefinitionKind::Local(None),
&mut parameter_idents,
true, // warn_if_unused
None,
);

Expand Down
30 changes: 18 additions & 12 deletions compiler/noirc_frontend/src/elaborator/patterns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,15 @@ impl<'context> Elaborator<'context> {
pattern: Pattern,
expected_type: Type,
definition_kind: DefinitionKind,
warn_if_unused: bool,
) -> HirPattern {
self.elaborate_pattern_mut(
pattern,
expected_type,
definition_kind,
None,
&mut Vec::new(),
warn_if_unused,
None,
)
}
Expand All @@ -45,6 +47,7 @@ impl<'context> Elaborator<'context> {
expected_type: Type,
definition_kind: DefinitionKind,
created_ids: &mut Vec<HirIdent>,
warn_if_unused: bool,
global_id: Option<GlobalId>,
) -> HirPattern {
self.elaborate_pattern_mut(
Expand All @@ -53,17 +56,20 @@ impl<'context> Elaborator<'context> {
definition_kind,
None,
created_ids,
warn_if_unused,
global_id,
)
}

#[allow(clippy::too_many_arguments)]
fn elaborate_pattern_mut(
&mut self,
pattern: Pattern,
expected_type: Type,
definition: DefinitionKind,
mutable: Option<Span>,
new_definitions: &mut Vec<HirIdent>,
warn_if_unused: bool,
global_id: Option<GlobalId>,
) -> HirPattern {
match pattern {
Expand All @@ -80,7 +86,13 @@ impl<'context> Elaborator<'context> {
let location = Location::new(name.span(), self.file);
HirIdent::non_trait_method(id, location)
} else {
self.add_variable_decl(name, mutable.is_some(), true, definition)
self.add_variable_decl(
name,
mutable.is_some(),
true, // allow_shadowing
warn_if_unused,
definition,
)
};
self.interner.push_definition_type(ident.id, expected_type);
new_definitions.push(ident.clone());
Expand All @@ -97,6 +109,7 @@ impl<'context> Elaborator<'context> {
definition,
Some(span),
new_definitions,
warn_if_unused,
global_id,
);
let location = Location::new(span, self.file);
Expand Down Expand Up @@ -128,6 +141,7 @@ impl<'context> Elaborator<'context> {
definition.clone(),
mutable,
new_definitions,
warn_if_unused,
global_id,
)
});
Expand All @@ -151,6 +165,7 @@ impl<'context> Elaborator<'context> {
definition,
mutable,
new_definitions,
warn_if_unused,
global_id,
)
}
Expand Down Expand Up @@ -180,7 +195,7 @@ impl<'context> Elaborator<'context> {
// shadowing here lets us avoid further errors if we define ERROR_IDENT
// multiple times.
let name = ERROR_IDENT.into();
let identifier = this.add_variable_decl(name, false, true, definition.clone());
let identifier = this.add_variable_decl(name, false, true, true, definition.clone());
HirPattern::Identifier(identifier)
};

Expand Down Expand Up @@ -263,6 +278,7 @@ impl<'context> Elaborator<'context> {
definition.clone(),
mutable,
new_definitions,
true, // warn_if_unused
None,
);

Expand Down Expand Up @@ -295,16 +311,6 @@ impl<'context> Elaborator<'context> {
}

pub(super) fn add_variable_decl(
&mut self,
name: Ident,
mutable: bool,
allow_shadowing: bool,
definition: DefinitionKind,
) -> HirIdent {
self.add_variable_decl_inner(name, mutable, allow_shadowing, true, definition)
}

pub fn add_variable_decl_inner(
&mut self,
name: Ident,
mutable: bool,
Expand Down
11 changes: 10 additions & 1 deletion compiler/noirc_frontend/src/elaborator/statements.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,12 +98,16 @@ impl<'context> Elaborator<'context> {
}
}

let warn_if_unused =
!let_stmt.attributes.iter().any(|attr| attr.is_allow_unused_variables());

let r#type = annotated_type;
let pattern = self.elaborate_pattern_and_store_ids(
let_stmt.pattern,
r#type.clone(),
definition,
&mut Vec::new(),
warn_if_unused,
global_id,
);

Expand Down Expand Up @@ -215,7 +219,12 @@ impl<'context> Elaborator<'context> {
// TODO: For loop variables are currently mutable by default since we haven't
// yet implemented syntax for them to be optionally mutable.
let kind = DefinitionKind::Local(None);
let identifier = self.add_variable_decl(identifier, false, true, kind);
let identifier = self.add_variable_decl(
identifier, false, // mutable
true, // allow_shadowing
true, // warn_if_unused
kind,
);

// Check that start range and end range have the same types
let range_span = start_span.merge(end_span);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ impl HirStatement {
let pattern = let_stmt.pattern.to_display_ast(interner);
let r#type = interner.id_type(let_stmt.expression).to_display_ast();
let expression = let_stmt.expression.to_display_ast(interner);
StatementKind::new_let(pattern, r#type, expression)
StatementKind::new_let(pattern, r#type, expression, let_stmt.attributes.clone())
}
HirStatement::Constrain(constrain) => {
let expr = constrain.0.to_display_ast(interner);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2338,6 +2338,7 @@ fn function_def_set_parameters(
parameter_type.clone(),
DefinitionKind::Local(None),
&mut parameter_idents,
true, // warn_if_unused
None,
)
});
Expand Down
17 changes: 16 additions & 1 deletion compiler/noirc_frontend/src/lexer/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,7 @@ impl Attribute {
}
["varargs"] => Attribute::Secondary(SecondaryAttribute::Varargs),
["use_callers_scope"] => Attribute::Secondary(SecondaryAttribute::UseCallersScope),
["allow", tag] => Attribute::Secondary(SecondaryAttribute::Allow(tag.to_string())),
tokens => {
tokens.iter().try_for_each(|token| validate(token))?;
Attribute::Secondary(SecondaryAttribute::Custom(CustomAttribute {
Expand Down Expand Up @@ -925,6 +926,9 @@ pub enum SecondaryAttribute {
/// within the scope of the calling function/module rather than this one.
/// This affects functions such as `Expression::resolve` or `Quoted::as_type`.
UseCallersScope,

/// Allow chosen warnings to happen so they are silenced.
Allow(String),
}

impl SecondaryAttribute {
Expand All @@ -948,6 +952,14 @@ impl SecondaryAttribute {
SecondaryAttribute::Abi(_) => Some("abi".to_string()),
SecondaryAttribute::Varargs => Some("varargs".to_string()),
SecondaryAttribute::UseCallersScope => Some("use_callers_scope".to_string()),
SecondaryAttribute::Allow(_) => Some("allow".to_string()),
}
}

pub(crate) fn is_allow_unused_variables(&self) -> bool {
match self {
SecondaryAttribute::Allow(string) => string == "unused_variables",
michaeljklein marked this conversation as resolved.
Show resolved Hide resolved
_ => false,
}
}
}
Expand All @@ -966,6 +978,7 @@ impl fmt::Display for SecondaryAttribute {
SecondaryAttribute::Abi(ref k) => write!(f, "#[abi({k})]"),
SecondaryAttribute::Varargs => write!(f, "#[varargs]"),
SecondaryAttribute::UseCallersScope => write!(f, "#[use_callers_scope]"),
SecondaryAttribute::Allow(ref k) => write!(f, "#[allow(#{k})]"),
}
}
}
Expand Down Expand Up @@ -1011,7 +1024,9 @@ impl AsRef<str> for SecondaryAttribute {
SecondaryAttribute::Deprecated(Some(string)) => string,
SecondaryAttribute::Deprecated(None) => "",
SecondaryAttribute::Custom(attribute) => &attribute.contents,
SecondaryAttribute::Field(string) | SecondaryAttribute::Abi(string) => string,
SecondaryAttribute::Field(string)
| SecondaryAttribute::Abi(string)
| SecondaryAttribute::Allow(string) => string,
SecondaryAttribute::ContractLibraryMethod => "",
SecondaryAttribute::Export => "",
SecondaryAttribute::Varargs => "",
Expand Down
8 changes: 6 additions & 2 deletions compiler/noirc_frontend/src/parser/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -550,8 +550,12 @@ fn declaration<'a, P>(expr_parser: P) -> impl NoirParser<StatementKind> + 'a
where
P: ExprParser + 'a,
{
let_statement(expr_parser)
.map(|((pattern, typ), expr)| StatementKind::new_let(pattern, typ, expr))
attributes().then(let_statement(expr_parser)).validate(
|(attributes, ((pattern, typ), expr)), span, emit| {
let attributes = attributes::validate_secondary_attributes(attributes, span, emit);
StatementKind::new_let(pattern, typ, expr, attributes)
},
)
}

pub fn pattern() -> impl NoirParser<Pattern> {
Expand Down
16 changes: 15 additions & 1 deletion compiler/noirc_frontend/src/tests/unused_items.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
use crate::hir::{def_collector::dc_crate::CompilationError, resolution::errors::ResolverError};
use crate::{
hir::{def_collector::dc_crate::CompilationError, resolution::errors::ResolverError},
tests::assert_no_errors,
};

use super::get_program_errors;

Expand Down Expand Up @@ -157,3 +160,14 @@ fn errors_on_unused_trait() {
assert_eq!(ident.to_string(), "Foo");
assert_eq!(*item_type, "trait");
}

#[test]
fn silences_unused_variable_warning() {
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
let src = r#"
fn main() {
#[allow(unused_variables)]
let x = 1;
}
"#;
assert_no_errors(src);
}
9 changes: 9 additions & 0 deletions tooling/lsp/src/requests/completion/builtins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,15 @@ impl<'a> NodeFinder<'a> {
));
}
}
AttributeTarget::Let => {
if name_matches("allow", prefix) || name_matches("unused_variables", prefix) {
self.completion_items.push(simple_completion_item(
"allow(unused_variables)",
CompletionItemKind::METHOD,
None,
));
}
}
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions tooling/lsp/src/requests/completion/completion_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ impl<'a> NodeFinder<'a> {
AttributeTarget::Struct => Some(Type::Quoted(QuotedType::StructDefinition)),
AttributeTarget::Trait => Some(Type::Quoted(QuotedType::TraitDefinition)),
AttributeTarget::Function => Some(Type::Quoted(QuotedType::FunctionDefinition)),
AttributeTarget::Let => {
// No item can be suggested for a let statement attribute
return Vec::new();
}
}
} else {
None
Expand Down
Loading
Loading