Skip to content

Commit

Permalink
fix(lsp): replace panics with errors (#4209)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves <!-- Link to GitHub Issue -->

lsp panics when contract with #[event] #4203

## Summary\*

Replaces .expect calls with err returns.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** 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.

---------

Co-authored-by: jfecher <jake@aztecprotocol.com>
  • Loading branch information
kobyhallx and jfecher authored Jan 31, 2024
1 parent 3a90849 commit 26e9618
Show file tree
Hide file tree
Showing 3 changed files with 82 additions and 24 deletions.
86 changes: 65 additions & 21 deletions aztec_macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,12 @@ impl MacroProcessor for AztecMacro {
transform(ast, crate_id, context)
}

fn process_typed_ast(&self, crate_id: &CrateId, context: &mut HirContext) {
transform_hir(crate_id, context)
fn process_typed_ast(
&self,
crate_id: &CrateId,
context: &mut HirContext,
) -> Result<(), (MacroError, FileId)> {
transform_hir(crate_id, context).map_err(|(err, file_id)| (err.into(), file_id))
}
}

Expand All @@ -41,6 +45,7 @@ pub enum AztecMacroError {
ContractHasTooManyFunctions { span: Span },
ContractConstructorMissing { span: Span },
UnsupportedFunctionArgumentType { span: Span, typ: UnresolvedTypeData },
EventError { span: Span, message: String },
}

impl From<AztecMacroError> for MacroError {
Expand Down Expand Up @@ -71,6 +76,11 @@ impl From<AztecMacroError> for MacroError {
secondary_message: None,
span: Some(span),
},
AztecMacroError::EventError { span, message } => MacroError {
primary_message: message,
secondary_message: None,
span: Some(span),
},
}
}
}
Expand Down Expand Up @@ -237,8 +247,11 @@ fn transform(
//

/// Completes the Hir with data gathered from type resolution
fn transform_hir(crate_id: &CrateId, context: &mut HirContext) {
transform_events(crate_id, context);
fn transform_hir(
crate_id: &CrateId,
context: &mut HirContext,
) -> Result<(), (AztecMacroError, FileId)> {
transform_events(crate_id, context)
}

/// Includes an import to the aztec library if it has not been included yet
Expand Down Expand Up @@ -472,19 +485,30 @@ fn collect_crate_structs(crate_id: &CrateId, context: &HirContext) -> Vec<Struct
}

/// Substitutes the signature literal that was introduced in the selector method previously with the actual signature.
fn transform_event(struct_id: StructId, interner: &mut NodeInterner) {
fn transform_event(
struct_id: StructId,
interner: &mut NodeInterner,
) -> Result<(), (AztecMacroError, FileId)> {
let struct_type = interner.get_struct(struct_id);
let selector_id = interner
.lookup_method(&Type::Struct(struct_type, vec![]), struct_id, "selector", false)
.expect("Selector method not found");
.lookup_method(&Type::Struct(struct_type.clone(), vec![]), struct_id, "selector", false)
.ok_or_else(|| {
let error = AztecMacroError::EventError {
span: struct_type.borrow().location.span,
message: "Selector method not found".to_owned(),
};
(error, struct_type.borrow().location.file)
})?;
let selector_function = interner.function(&selector_id);

let compute_selector_statement = interner.statement(
selector_function
.block(interner)
.statements()
.first()
.expect("Compute selector statement not found"),
selector_function.block(interner).statements().first().ok_or_else(|| {
let error = AztecMacroError::EventError {
span: struct_type.borrow().location.span,
message: "Compute selector statement not found".to_owned(),
};
(error, struct_type.borrow().location.file)
})?,
);

let compute_selector_expression = match compute_selector_statement {
Expand All @@ -494,12 +518,21 @@ fn transform_event(struct_id: StructId, interner: &mut NodeInterner) {
},
_ => None,
}
.expect("Compute selector statement is not a call expression");

let first_arg_id = compute_selector_expression
.arguments
.first()
.expect("Missing argument for compute selector");
.ok_or_else(|| {
let error = AztecMacroError::EventError {
span: struct_type.borrow().location.span,
message: "Compute selector statement is not a call expression".to_owned(),
};
(error, struct_type.borrow().location.file)
})?;

let first_arg_id = compute_selector_expression.arguments.first().ok_or_else(|| {
let error = AztecMacroError::EventError {
span: struct_type.borrow().location.span,
message: "Compute selector statement is not a call expression".to_owned(),
};
(error, struct_type.borrow().location.file)
})?;

match interner.expression(first_arg_id) {
HirExpression::Literal(HirLiteral::Str(signature))
Expand All @@ -518,18 +551,29 @@ fn transform_event(struct_id: StructId, interner: &mut NodeInterner) {
selector_literal_id,
Type::String(Box::new(Type::Constant(signature.len() as u64))),
);
Ok(())
}
_ => unreachable!("Signature placeholder literal does not match"),
_ => Err((
AztecMacroError::EventError {
span: struct_type.borrow().location.span,
message: "Signature placeholder literal does not match".to_owned(),
},
struct_type.borrow().location.file,
)),
}
}

fn transform_events(crate_id: &CrateId, context: &mut HirContext) {
fn transform_events(
crate_id: &CrateId,
context: &mut HirContext,
) -> Result<(), (AztecMacroError, FileId)> {
for struct_id in collect_crate_structs(crate_id, context) {
let attributes = context.def_interner.struct_attributes(&struct_id);
if attributes.iter().any(|attr| matches!(attr, SecondaryAttribute::Event)) {
transform_event(struct_id, &mut context.def_interner);
transform_event(struct_id, &mut context.def_interner)?;
}
}
Ok(())
}

const SIGNATURE_PLACEHOLDER: &str = "SIGNATURE_PLACEHOLDER";
Expand Down
14 changes: 12 additions & 2 deletions compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use crate::hir::resolution::{
use crate::hir::type_check::{type_check_func, TypeCheckError, TypeChecker};
use crate::hir::Context;

use crate::macros_api::MacroProcessor;
use crate::macros_api::{MacroError, MacroProcessor};
use crate::node_interner::{FuncId, NodeInterner, StmtId, StructId, TraitId, TypeAliasId};

use crate::parser::{ParserError, SortedModule};
Expand Down Expand Up @@ -155,6 +155,12 @@ impl From<CompilationError> for CustomDiagnostic {
}
}

impl From<MacroError> for CompilationError {
fn from(value: MacroError) -> Self {
CompilationError::DefinitionError(DefCollectorErrorKind::MacroError(value))
}
}

impl From<ParserError> for CompilationError {
fn from(value: ParserError) -> Self {
CompilationError::ParseError(value)
Expand Down Expand Up @@ -359,7 +365,11 @@ impl DefCollector {
errors.extend(resolved_globals.errors);

for macro_processor in macro_processors {
macro_processor.process_typed_ast(&crate_id, context);
macro_processor.process_typed_ast(&crate_id, context).unwrap_or_else(
|(macro_err, file_id)| {
errors.push((macro_err.into(), file_id));
},
);
}
errors.extend(type_check_globals(&mut context.def_interner, resolved_globals.globals));

Expand Down
6 changes: 5 additions & 1 deletion compiler/noirc_frontend/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ pub mod macros_api {
) -> Result<SortedModule, (MacroError, FileId)>;
/// Function to manipulate the AST after type checking has been completed.
/// The AST after type checking has been done is called the HIR.
fn process_typed_ast(&self, crate_id: &CrateId, context: &mut HirContext);
fn process_typed_ast(
&self,
crate_id: &CrateId,
context: &mut HirContext,
) -> Result<(), (MacroError, FileId)>;
}
}

0 comments on commit 26e9618

Please sign in to comment.