From a381dc512db82e666af515b6a6afe471c95dbb8f Mon Sep 17 00:00:00 2001 From: f01dab1e Date: Mon, 14 Aug 2023 09:41:16 +0000 Subject: [PATCH 1/3] chore: better return type error message --- .../src/hir/type_check/errors.rs | 3 +++ .../noirc_frontend/src/hir/type_check/mod.rs | 27 ++++++++++++++++--- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/crates/noirc_frontend/src/hir/type_check/errors.rs b/crates/noirc_frontend/src/hir/type_check/errors.rs index 84bf511d314..c9b2dc25289 100644 --- a/crates/noirc_frontend/src/hir/type_check/errors.rs +++ b/crates/noirc_frontend/src/hir/type_check/errors.rs @@ -23,6 +23,8 @@ pub enum Source { Comparison, #[error("BinOp")] BinOp, + #[error("Return")] + Return, } #[derive(Error, Debug, Clone, PartialEq, Eq)] @@ -195,6 +197,7 @@ impl From for Diagnostic { Source::StringLen => format!("Can only compare strings of the same length. Here LHS is of length {lhs}, and RHS is {rhs}"), Source::Comparison => format!("Unsupported types for comparison: {lhs} and {rhs}"), Source::BinOp => format!("Unsupported types for binary operation: {lhs} and {rhs}"), + Source::Return => format!("Expected return type {lhs}, but a value of type {rhs} was actually returned"), }; Diagnostic::simple_error(message, String::new(), span) diff --git a/crates/noirc_frontend/src/hir/type_check/mod.rs b/crates/noirc_frontend/src/hir/type_check/mod.rs index 8768120af2f..c4f04e7437e 100644 --- a/crates/noirc_frontend/src/hir/type_check/mod.rs +++ b/crates/noirc_frontend/src/hir/type_check/mod.rs @@ -14,6 +14,7 @@ mod stmt; pub use errors::TypeCheckError; use crate::{ + hir_def::expr::HirExpression, node_interner::{ExprId, FuncId, NodeInterner, StmtId}, Type, }; @@ -55,6 +56,13 @@ pub fn type_check_func(interner: &mut NodeInterner, func_id: FuncId) -> Vec Vec Date: Mon, 14 Aug 2023 23:09:20 +0000 Subject: [PATCH 2/3] track return type span --- crates/noirc_frontend/src/ast/expression.rs | 19 +++++++++++- crates/noirc_frontend/src/ast/function.rs | 7 +++-- crates/noirc_frontend/src/ast/traits.rs | 4 +-- .../src/hir/resolution/resolver.rs | 1 + .../src/hir/type_check/errors.rs | 22 ++++++++++++-- .../noirc_frontend/src/hir/type_check/mod.rs | 30 +++++++++++++------ crates/noirc_frontend/src/hir_def/function.rs | 4 ++- crates/noirc_frontend/src/parser/parser.rs | 23 +++++++------- 8 files changed, 83 insertions(+), 27 deletions(-) diff --git a/crates/noirc_frontend/src/ast/expression.rs b/crates/noirc_frontend/src/ast/expression.rs index b1170ff0ed0..c7ed4d0c766 100644 --- a/crates/noirc_frontend/src/ast/expression.rs +++ b/crates/noirc_frontend/src/ast/expression.rs @@ -364,11 +364,19 @@ pub struct FunctionDefinition { pub body: BlockExpression, pub span: Span, pub where_clause: Vec, - pub return_type: UnresolvedType, + pub return_type: FunctionReturnType, pub return_visibility: noirc_abi::AbiVisibility, pub return_distinctness: noirc_abi::AbiDistinctness, } +#[derive(Debug, PartialEq, Eq, Clone)] +pub enum FunctionReturnType { + /// Returns type is not specified. + Default(Span), + /// Everything else. + Ty(UnresolvedType, Span), +} + /// Describes the types of smart contract functions that are allowed. /// - All Noir programs in the non-contract context can be seen as `Secret`. #[derive(serde::Serialize, serde::Deserialize, Debug, Copy, Clone, PartialEq, Eq)] @@ -636,3 +644,12 @@ impl Display for FunctionDefinition { ) } } + +impl Display for FunctionReturnType { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + FunctionReturnType::Default(_) => f.write_str(""), + FunctionReturnType::Ty(ty, _) => write!(f, "{ty}"), + } + } +} diff --git a/crates/noirc_frontend/src/ast/function.rs b/crates/noirc_frontend/src/ast/function.rs index 02af960f7a8..02bd9d085a5 100644 --- a/crates/noirc_frontend/src/ast/function.rs +++ b/crates/noirc_frontend/src/ast/function.rs @@ -1,6 +1,6 @@ use std::fmt::Display; -use crate::{token::Attribute, Ident, Pattern}; +use crate::{token::Attribute, Ident, Pattern, FunctionReturnType}; use super::{FunctionDefinition, UnresolvedType}; @@ -41,7 +41,10 @@ impl NoirFunction { } pub fn return_type(&self) -> UnresolvedType { - self.def.return_type.clone() + match &self.def.return_type { + FunctionReturnType::Default(_) => UnresolvedType::Unit, + FunctionReturnType::Ty(ty, _) => ty.clone(), + } } pub fn name(&self) -> &str { &self.name_ident().0.contents diff --git a/crates/noirc_frontend/src/ast/traits.rs b/crates/noirc_frontend/src/ast/traits.rs index bb5d2117037..b7670ad2fc4 100644 --- a/crates/noirc_frontend/src/ast/traits.rs +++ b/crates/noirc_frontend/src/ast/traits.rs @@ -3,7 +3,7 @@ use std::fmt::Display; use iter_extended::vecmap; use noirc_errors::Span; -use crate::{BlockExpression, Expression, Ident, NoirFunction, UnresolvedGenerics, UnresolvedType}; +use crate::{BlockExpression, Expression, Ident, NoirFunction, UnresolvedGenerics, UnresolvedType, FunctionReturnType}; /// AST node for trait definitions: /// `trait name { ... items ... }` @@ -24,7 +24,7 @@ pub enum TraitItem { name: Ident, generics: Vec, parameters: Vec<(Ident, UnresolvedType)>, - return_type: UnresolvedType, + return_type: FunctionReturnType, where_clause: Vec, body: Option, }, diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 4bfb5428ed7..1e019e0d51b 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -726,6 +726,7 @@ impl<'a> Resolver<'a> { location, typ, parameters: parameters.into(), + return_span: func.def.return_type.clone(), return_visibility: func.def.return_visibility, return_distinctness: func.def.return_distinctness, has_body: !func.def.body.is_empty(), diff --git a/crates/noirc_frontend/src/hir/type_check/errors.rs b/crates/noirc_frontend/src/hir/type_check/errors.rs index c9b2dc25289..43bc1505512 100644 --- a/crates/noirc_frontend/src/hir/type_check/errors.rs +++ b/crates/noirc_frontend/src/hir/type_check/errors.rs @@ -5,6 +5,7 @@ use thiserror::Error; use crate::hir::resolution::errors::ResolverError; use crate::hir_def::expr::HirBinaryOp; use crate::hir_def::types::Type; +use crate::FunctionReturnType; use crate::Signedness; #[derive(Error, Debug, Clone, PartialEq, Eq)] @@ -24,7 +25,7 @@ pub enum Source { #[error("BinOp")] BinOp, #[error("Return")] - Return, + Return(FunctionReturnType, Option), } #[derive(Error, Debug, Clone, PartialEq, Eq)] @@ -197,7 +198,24 @@ impl From for Diagnostic { Source::StringLen => format!("Can only compare strings of the same length. Here LHS is of length {lhs}, and RHS is {rhs}"), Source::Comparison => format!("Unsupported types for comparison: {lhs} and {rhs}"), Source::BinOp => format!("Unsupported types for binary operation: {lhs} and {rhs}"), - Source::Return => format!("Expected return type {lhs}, but a value of type {rhs} was actually returned"), + Source::Return(ret_ty, return_expr) => { + let ret_ty_span = match ret_ty { + FunctionReturnType::Default(span) | FunctionReturnType::Ty(_, span) => span + }; + + let message = format!("expected type {lhs}, found type {rhs}"); + let mut diagnostic = Diagnostic::simple_error(message.clone(), format!("expected {lhs} because of return type"), ret_ty_span); + + if let FunctionReturnType::Default(_) = ret_ty { + diagnostic.add_note(format!("help: try adding a return type: `-> {rhs}`")); + } + + if let Some(expr_span) = return_expr { + diagnostic.add_secondary(message, expr_span); + } + + return diagnostic + }, }; Diagnostic::simple_error(message, String::new(), span) diff --git a/crates/noirc_frontend/src/hir/type_check/mod.rs b/crates/noirc_frontend/src/hir/type_check/mod.rs index c4f04e7437e..b620dcf4a77 100644 --- a/crates/noirc_frontend/src/hir/type_check/mod.rs +++ b/crates/noirc_frontend/src/hir/type_check/mod.rs @@ -14,11 +14,13 @@ mod stmt; pub use errors::TypeCheckError; use crate::{ - hir_def::expr::HirExpression, + hir_def::{expr::HirExpression, stmt::HirStatement}, node_interner::{ExprId, FuncId, NodeInterner, StmtId}, Type, }; +use self::errors::Source; + type TypeCheckFn = Box Result<(), TypeCheckError>>; pub struct TypeChecker<'interner> { @@ -56,12 +58,21 @@ pub fn type_check_func(interner: &mut NodeInterner, func_id: FuncId) -> Vec Vec impl NoirParser { .map(|ret| ret.unwrap_or(UnresolvedType::Unspecified)) } -fn function_return_type() -> impl NoirParser<((AbiDistinctness, AbiVisibility), UnresolvedType)> { +fn function_return_type() -> impl NoirParser<((AbiDistinctness, AbiVisibility), FunctionReturnType)> +{ just(Token::Arrow) .ignore_then(optional_distinctness()) .then(optional_visibility()) - .then(parse_type()) + .then(parse_type().map_with_span(|ty, span| (ty, span))) .or_not() - .map(|ret| { - ret.unwrap_or(( + .map_with_span(|ret, span| match ret { + Some((head, (ty, span))) => (head, FunctionReturnType::Ty(ty, span)), + None => ( (AbiDistinctness::DuplicationAllowed, AbiVisibility::Private), - UnresolvedType::Unit, - )) + FunctionReturnType::Default(span), + ), }) } From 29e74f950beeda8f5f7cca06219943d973899d7a Mon Sep 17 00:00:00 2001 From: f01dab1e Date: Tue, 15 Aug 2023 17:49:24 +0000 Subject: [PATCH 3/3] apply feedback --- crates/noirc_frontend/src/ast/function.rs | 2 +- crates/noirc_frontend/src/ast/traits.rs | 5 ++- .../src/hir/resolution/resolver.rs | 2 +- .../src/hir/type_check/errors.rs | 11 ++--- .../noirc_frontend/src/hir/type_check/mod.rs | 44 +++++++++++-------- crates/noirc_frontend/src/hir_def/function.rs | 4 +- crates/noirc_frontend/src/parser/parser.rs | 3 +- 7 files changed, 40 insertions(+), 31 deletions(-) diff --git a/crates/noirc_frontend/src/ast/function.rs b/crates/noirc_frontend/src/ast/function.rs index 02bd9d085a5..43db3294dd9 100644 --- a/crates/noirc_frontend/src/ast/function.rs +++ b/crates/noirc_frontend/src/ast/function.rs @@ -1,6 +1,6 @@ use std::fmt::Display; -use crate::{token::Attribute, Ident, Pattern, FunctionReturnType}; +use crate::{token::Attribute, FunctionReturnType, Ident, Pattern}; use super::{FunctionDefinition, UnresolvedType}; diff --git a/crates/noirc_frontend/src/ast/traits.rs b/crates/noirc_frontend/src/ast/traits.rs index b7670ad2fc4..4a649a64cc6 100644 --- a/crates/noirc_frontend/src/ast/traits.rs +++ b/crates/noirc_frontend/src/ast/traits.rs @@ -3,7 +3,10 @@ use std::fmt::Display; use iter_extended::vecmap; use noirc_errors::Span; -use crate::{BlockExpression, Expression, Ident, NoirFunction, UnresolvedGenerics, UnresolvedType, FunctionReturnType}; +use crate::{ + BlockExpression, Expression, FunctionReturnType, Ident, NoirFunction, UnresolvedGenerics, + UnresolvedType, +}; /// AST node for trait definitions: /// `trait name { ... items ... }` diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 1e019e0d51b..6f30c9ded50 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -726,7 +726,7 @@ impl<'a> Resolver<'a> { location, typ, parameters: parameters.into(), - return_span: func.def.return_type.clone(), + return_type: func.def.return_type.clone(), return_visibility: func.def.return_visibility, return_distinctness: func.def.return_distinctness, has_body: !func.def.body.is_empty(), diff --git a/crates/noirc_frontend/src/hir/type_check/errors.rs b/crates/noirc_frontend/src/hir/type_check/errors.rs index 43bc1505512..686c7c807b7 100644 --- a/crates/noirc_frontend/src/hir/type_check/errors.rs +++ b/crates/noirc_frontend/src/hir/type_check/errors.rs @@ -25,7 +25,7 @@ pub enum Source { #[error("BinOp")] BinOp, #[error("Return")] - Return(FunctionReturnType, Option), + Return(FunctionReturnType, Span), } #[derive(Error, Debug, Clone, PartialEq, Eq)] @@ -198,21 +198,18 @@ impl From for Diagnostic { Source::StringLen => format!("Can only compare strings of the same length. Here LHS is of length {lhs}, and RHS is {rhs}"), Source::Comparison => format!("Unsupported types for comparison: {lhs} and {rhs}"), Source::BinOp => format!("Unsupported types for binary operation: {lhs} and {rhs}"), - Source::Return(ret_ty, return_expr) => { + Source::Return(ret_ty, expr_span) => { let ret_ty_span = match ret_ty { FunctionReturnType::Default(span) | FunctionReturnType::Ty(_, span) => span }; - let message = format!("expected type {lhs}, found type {rhs}"); - let mut diagnostic = Diagnostic::simple_error(message.clone(), format!("expected {lhs} because of return type"), ret_ty_span); + let mut diagnostic = Diagnostic::simple_error(format!("expected type {lhs}, found type {rhs}"), format!("expected {lhs} because of return type"), ret_ty_span); if let FunctionReturnType::Default(_) = ret_ty { diagnostic.add_note(format!("help: try adding a return type: `-> {rhs}`")); } - if let Some(expr_span) = return_expr { - diagnostic.add_secondary(message, expr_span); - } + diagnostic.add_secondary(format!("{rhs} returned here"), expr_span); return diagnostic }, diff --git a/crates/noirc_frontend/src/hir/type_check/mod.rs b/crates/noirc_frontend/src/hir/type_check/mod.rs index b620dcf4a77..67107206683 100644 --- a/crates/noirc_frontend/src/hir/type_check/mod.rs +++ b/crates/noirc_frontend/src/hir/type_check/mod.rs @@ -58,24 +58,10 @@ pub fn type_check_func(interner: &mut NodeInterner, func_id: FuncId) -> Vec Vec Vec (noirc_errors::Span, bool) { + let (expr_span, empty_function) = + if let HirExpression::Block(block) = interner.expression(function_body_id) { + let last_stmt = block.statements().last(); + let mut span = interner.expr_span(function_body_id); + + if let Some(last_stmt) = last_stmt { + if let HirStatement::Expression(expr) = interner.statement(last_stmt) { + span = interner.expr_span(&expr); + } + } + + (span, last_stmt.is_none()) + } else { + (interner.expr_span(function_body_id), false) + }; + (expr_span, empty_function) +} + impl<'interner> TypeChecker<'interner> { fn new(interner: &'interner mut NodeInterner) -> Self { Self { delayed_type_checks: Vec::new(), interner, errors: vec![] } @@ -267,7 +275,7 @@ mod test { return_visibility: noirc_abi::AbiVisibility::Private, return_distinctness: noirc_abi::AbiDistinctness::DuplicationAllowed, has_body: true, - return_span: FunctionReturnType::Default(Span::default()), + return_type: FunctionReturnType::Default(Span::default()), }; interner.push_fn_meta(func_meta, func_id); diff --git a/crates/noirc_frontend/src/hir_def/function.rs b/crates/noirc_frontend/src/hir_def/function.rs index fee0251a713..3a4cb2a1036 100644 --- a/crates/noirc_frontend/src/hir_def/function.rs +++ b/crates/noirc_frontend/src/hir_def/function.rs @@ -7,7 +7,7 @@ use super::stmt::HirPattern; use crate::hir::def_map::ModuleId; use crate::node_interner::{ExprId, NodeInterner}; use crate::{token::Attribute, FunctionKind}; -use crate::{ContractFunctionType, Type, FunctionReturnType}; +use crate::{ContractFunctionType, FunctionReturnType, Type}; /// A Hir function is a block expression /// with a list of statements @@ -137,7 +137,7 @@ pub struct FuncMeta { pub parameters: Parameters, - pub return_span: FunctionReturnType, + pub return_type: FunctionReturnType, pub return_visibility: AbiVisibility, diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index 711e6206cca..c70a2e1dae8 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -23,6 +23,7 @@ //! prevent other parsers from being tried afterward since there is no longer an error. Thus, they should //! be limited to cases like the above `fn` example where it is clear we shouldn't back out of the //! current parser to try alternative parsers in a `choice` expression. +use super::spanned; use super::{ foldl_with_span, labels::ParsingRuleLabel, parameter_name_recovery, parameter_recovery, parenthesized, then_commit, then_commit_ignore, top_level_statement_recovery, ExprParser, @@ -264,7 +265,7 @@ fn function_return_type() -> impl NoirParser<((AbiDistinctness, AbiVisibility), just(Token::Arrow) .ignore_then(optional_distinctness()) .then(optional_visibility()) - .then(parse_type().map_with_span(|ty, span| (ty, span))) + .then(spanned(parse_type())) .or_not() .map_with_span(|ret, span| match ret { Some((head, (ty, span))) => (head, FunctionReturnType::Ty(ty, span)),