Skip to content

Commit

Permalink
feat: allow silencing an unused variable defined via let (#6149)
Browse files Browse the repository at this point in the history
# Description

## Problem

Resolves #6148

## Summary


![lsp-allow-unused](https://github.com/user-attachments/assets/738168ab-cfd6-468e-8abc-fbb0e30e60b6)

## Additional Context

This is only for `let`, which is what we currently need.

Should this be documented? I guess so, but I couldn't find where in our
docs we explain `let`. (on the other hand we have `deprecated` but it
seems to also be undocumented)

## Documentation

Check one:
- [ ] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
asterite authored Sep 25, 2024
1 parent acdfbbc commit a2bc059
Show file tree
Hide file tree
Showing 15 changed files with 120 additions and 22 deletions.
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 @@ pub enum AttributeTarget {
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 Statement {

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
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 @@ impl DebugInstrumenter {
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 @@ impl DebugInstrumenter {
}),
span: let_stmt.expression.span,
},
vec![],
),
span: *span,
}
Expand All @@ -274,6 +276,7 @@ impl DebugInstrumenter {
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 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",
_ => 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() {
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

0 comments on commit a2bc059

Please sign in to comment.