From 092151d6a075d4fb47535bffcc186d0f75a73add Mon Sep 17 00:00:00 2001 From: KtorZ Date: Thu, 19 Jan 2023 16:35:39 +0100 Subject: [PATCH 1/4] Remove dead-code and fix comment about discarded expressions While Gleam originally allowed various kinds of expressions to be discarded in a sequence, we simply do not allow expressions to be discarded implicitly. So any non-final expression in a sequence must be a let-binding. This prevents silly mistakes. --- crates/aiken-lang/src/tipo/expr.rs | 43 +++++++++--------------------- 1 file changed, 13 insertions(+), 30 deletions(-) diff --git a/crates/aiken-lang/src/tipo/expr.rs b/crates/aiken-lang/src/tipo/expr.rs index ec65725a..86c25906 100644 --- a/crates/aiken-lang/src/tipo/expr.rs +++ b/crates/aiken-lang/src/tipo/expr.rs @@ -168,25 +168,6 @@ impl<'a, 'b> ExprTyper<'a, 'b> { self.infer_fn_with_known_types(arguments, body, return_type) } - /// Emit a warning if the given expressions should not be discarded. - /// e.g. because it's a literal (why was it made in the first place?) - /// e.g. because it's of the `Result` type (errors should be handled) - fn _expression_discarded(&mut self, discarded: &TypedExpr) { - if discarded.is_literal() { - self.environment.warnings.push(Warning::UnusedLiteral { - location: discarded.location(), - }); - } - - if discarded.tipo().is_result() && !discarded.is_assignment() { - self.environment - .warnings - .push(Warning::ImplicitlyDiscardedResult { - location: discarded.location(), - }); - } - } - fn get_field_map( &mut self, constructor: &TypedExpr, @@ -210,6 +191,15 @@ impl<'a, 'b> ExprTyper<'a, 'b> { .field_map()) } + fn assert_assignment(&self, expr: &TypedExpr) -> Result<(), Error> { + if !matches!(*expr, TypedExpr::Assignment { .. }) { + return Err(Error::ImplicityDiscardedExpression { + location: expr.location(), + }); + } + Ok(()) + } + pub fn in_new_scope(&mut self, process_scope: impl FnOnce(&mut Self) -> T) -> T { // Create new scope let environment_reset_data = self.environment.open_new_scope(); @@ -1682,18 +1672,11 @@ impl<'a, 'b> ExprTyper<'a, 'b> { for (i, expression) in untyped.into_iter().enumerate() { let expression = self.infer(expression)?; - // This isn't the final expression in the sequence, so call the - // `expression_discarded` function to see if anything is being - // discarded that we think shouldn't be. We also want to make sure - // that there are no implicitly discarded expressions - if i < count - 1 { - // self.expression_discarded(&expression); - if !matches!(expression, TypedExpr::Assignment { .. }) { - return Err(Error::ImplicityDiscardedExpression { - location: expression.location(), - }); - } + // This isn't the final expression in the sequence, so it *must* + // be a let-binding; we do not allow anything else. + if i < count - 1 { + self.assert_assignment(&expression)?; } expressions.push(expression); From bb82b1bc1eef9b7d086ae8658baf04f41df4c170 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Thu, 19 Jan 2023 17:25:18 +0100 Subject: [PATCH 2/4] slightly rework hint for 'ImpliclyDiscardedExpression' --- crates/aiken-lang/src/tipo/error.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/aiken-lang/src/tipo/error.rs b/crates/aiken-lang/src/tipo/error.rs index 81475bc2..32261f9f 100644 --- a/crates/aiken-lang/src/tipo/error.rs +++ b/crates/aiken-lang/src/tipo/error.rs @@ -494,7 +494,12 @@ impl Diagnostic for Error { Self::FunctionTypeInData { .. } => Some(Box::new("Data types can't have functions in them due to how Plutus Data works.")), - Self::ImplicityDiscardedExpression { .. } => Some(Box::new("Everything is an expression and returns a value.\nTry assigning this expression to a variable.")), + Self::ImplicityDiscardedExpression { .. } => Some(Box::new(formatdoc! { + r#"A function can contain a sequence of expressions. However, any expression but the last one must be assign to a variable using the {let_keyword} keyword. If you really wish to discard an expression that is unused, you can prefix its name with '{discard}'. + "# + , let_keyword = "let".yellow() + , discard = "_".yellow() + })), Self::IncorrectFieldsArity { .. } => None, Self::IncorrectFunctionCallArity { expected, .. } => Some(Box::new(formatdoc! { From c5e876e81768cdc9492f03d4e601f04c6117fea6 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Thu, 19 Jan 2023 17:26:33 +0100 Subject: [PATCH 3/4] Fix typo in variant name: Implicity -> Implicitly --- crates/aiken-lang/src/tipo/error.rs | 10 +++++----- crates/aiken-lang/src/tipo/expr.rs | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/aiken-lang/src/tipo/error.rs b/crates/aiken-lang/src/tipo/error.rs index 32261f9f..6c8f1cf0 100644 --- a/crates/aiken-lang/src/tipo/error.rs +++ b/crates/aiken-lang/src/tipo/error.rs @@ -51,7 +51,7 @@ pub enum Error { FunctionTypeInData { location: Span }, #[error("I found a discarded expression not bound to a variable.")] - ImplicityDiscardedExpression { location: Span }, + ImplicitlyDiscardedExpression { location: Span }, #[error("I saw a {} fields in a context where there should be {}.\n", given.purple(), expected.purple())] IncorrectFieldsArity { @@ -375,7 +375,7 @@ impl Diagnostic for Error { Self::DuplicateName { .. } => Some(Box::new("duplicate_name")), Self::DuplicateTypeName { .. } => Some(Box::new("duplicate_type_name")), Self::FunctionTypeInData { .. } => Some(Box::new("function_type_in_data")), - Self::ImplicityDiscardedExpression { .. } => { + Self::ImplicitlyDiscardedExpression { .. } => { Some(Box::new("implicitly_discarded_expr")) } Self::IncorrectFieldsArity { .. } => Some(Box::new("incorrect_fields_arity")), @@ -494,7 +494,7 @@ impl Diagnostic for Error { Self::FunctionTypeInData { .. } => Some(Box::new("Data types can't have functions in them due to how Plutus Data works.")), - Self::ImplicityDiscardedExpression { .. } => Some(Box::new(formatdoc! { + Self::ImplicitlyDiscardedExpression { .. } => Some(Box::new(formatdoc! { r#"A function can contain a sequence of expressions. However, any expression but the last one must be assign to a variable using the {let_keyword} keyword. If you really wish to discard an expression that is unused, you can prefix its name with '{discard}'. "# , let_keyword = "let".yellow() @@ -1162,7 +1162,7 @@ impl Diagnostic for Error { vec![LabeledSpan::new_with_span(None, *location)].into_iter(), )), - Self::ImplicityDiscardedExpression { location, .. } => Some(Box::new( + Self::ImplicitlyDiscardedExpression { location, .. } => Some(Box::new( vec![LabeledSpan::new_with_span(None, *location)].into_iter(), )), Self::IncorrectFieldsArity { location, .. } => Some(Box::new( @@ -1298,7 +1298,7 @@ impl Diagnostic for Error { Self::DuplicateName { .. } => None, Self::DuplicateTypeName { .. } => None, Self::FunctionTypeInData { .. } => None, - Self::ImplicityDiscardedExpression { .. } => None, + Self::ImplicitlyDiscardedExpression { .. } => None, Self::IncorrectFieldsArity { .. } => Some(Box::new( "https://aiken-lang.org/language-tour/custom-types", )), diff --git a/crates/aiken-lang/src/tipo/expr.rs b/crates/aiken-lang/src/tipo/expr.rs index 86c25906..30f71d43 100644 --- a/crates/aiken-lang/src/tipo/expr.rs +++ b/crates/aiken-lang/src/tipo/expr.rs @@ -193,7 +193,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> { fn assert_assignment(&self, expr: &TypedExpr) -> Result<(), Error> { if !matches!(*expr, TypedExpr::Assignment { .. }) { - return Err(Error::ImplicityDiscardedExpression { + return Err(Error::ImplicitlyDiscardedExpression { location: expr.location(), }); } From 10fb7455f384c48e58cec90813f0654b69e1af63 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Thu, 19 Jan 2023 18:05:57 +0100 Subject: [PATCH 4/4] Forbid let-binding as last expression of a sequence Fixes #283 --- crates/aiken-lang/src/format.rs | 2 +- crates/aiken-lang/src/tipo/error.rs | 37 ++++++++++++++++++++++++++--- crates/aiken-lang/src/tipo/expr.rs | 33 ++++++++++++++++++------- 3 files changed, 59 insertions(+), 13 deletions(-) diff --git a/crates/aiken-lang/src/format.rs b/crates/aiken-lang/src/format.rs index 1288fd45..f8f7dd78 100644 --- a/crates/aiken-lang/src/format.rs +++ b/crates/aiken-lang/src/format.rs @@ -647,7 +647,7 @@ impl<'comments> Formatter<'comments> { } } - fn expr<'a>(&mut self, expr: &'a UntypedExpr) -> Document<'a> { + pub fn expr<'a>(&mut self, expr: &'a UntypedExpr) -> Document<'a> { let comments = self.pop_comments(expr.start_byte_index()); let document = match expr { diff --git a/crates/aiken-lang/src/tipo/error.rs b/crates/aiken-lang/src/tipo/error.rs index 6c8f1cf0..bacfe2cc 100644 --- a/crates/aiken-lang/src/tipo/error.rs +++ b/crates/aiken-lang/src/tipo/error.rs @@ -1,6 +1,7 @@ use super::Type; use crate::{ ast::{Annotation, BinOp, CallArg, Span, TodoKind, UntypedPattern}, + expr, format::Formatter, levenshtein, pretty::Documentable, @@ -53,6 +54,12 @@ pub enum Error { #[error("I found a discarded expression not bound to a variable.")] ImplicitlyDiscardedExpression { location: Span }, + #[error("I discovered a function who's ending with an assignment.")] + LastExpressionIsAssignment { + location: Span, + expr: expr::UntypedExpr, + }, + #[error("I saw a {} fields in a context where there should be {}.\n", given.purple(), expected.purple())] IncorrectFieldsArity { location: Span, @@ -378,6 +385,7 @@ impl Diagnostic for Error { Self::ImplicitlyDiscardedExpression { .. } => { Some(Box::new("implicitly_discarded_expr")) } + Self::LastExpressionIsAssignment { .. } => Some(Box::new("last_expr_is_assignment")), Self::IncorrectFieldsArity { .. } => Some(Box::new("incorrect_fields_arity")), Self::IncorrectFunctionCallArity { .. } => Some(Box::new("incorrect_fn_arity")), Self::IncorrectPatternArity { .. } => Some(Box::new("incorrect_pattern_arity")), @@ -495,11 +503,29 @@ impl Diagnostic for Error { Self::FunctionTypeInData { .. } => Some(Box::new("Data types can't have functions in them due to how Plutus Data works.")), Self::ImplicitlyDiscardedExpression { .. } => Some(Box::new(formatdoc! { - r#"A function can contain a sequence of expressions. However, any expression but the last one must be assign to a variable using the {let_keyword} keyword. If you really wish to discard an expression that is unused, you can prefix its name with '{discard}'. + r#"A function can contain a sequence of expressions. However, any expression but the last one must be assign to a variable using the {keyword_let} keyword. If you really wish to discard an expression that is unused, you can assign it to '{discard}'. "# - , let_keyword = "let".yellow() + , keyword_let = "let".yellow() , discard = "_".yellow() })), + + Self::LastExpressionIsAssignment { expr, .. } => Some(Box::new(formatdoc! { + r#"In Aiken, functions must return an explicit result in the form of an expression. While assignments are technically speaking expressions, they aren't allowed to be the last expression of a function because they convey a different meaning and this could be error-prone. + + If you really meant to return that last expression, try to replace it with the following: + + {sample} + "# + , sample = Formatter::new() + .expr(expr) + .to_pretty_string(70) + .lines() + .enumerate() + .map(|(ix, line)| if ix == 0 { format!("╰─▶ {}", line.yellow()) } else { format!(" {line}").yellow().to_string() }) + .collect::>() + .join("\n") + })), + Self::IncorrectFieldsArity { .. } => None, Self::IncorrectFunctionCallArity { expected, .. } => Some(Box::new(formatdoc! { @@ -1161,10 +1187,12 @@ impl Diagnostic for Error { Self::FunctionTypeInData { location } => Some(Box::new( vec![LabeledSpan::new_with_span(None, *location)].into_iter(), )), - Self::ImplicitlyDiscardedExpression { location, .. } => Some(Box::new( vec![LabeledSpan::new_with_span(None, *location)].into_iter(), )), + Self::LastExpressionIsAssignment { location, .. } => Some(Box::new( + vec![LabeledSpan::new_with_span(None, *location)].into_iter(), + )), Self::IncorrectFieldsArity { location, .. } => Some(Box::new( vec![LabeledSpan::new_with_span(None, *location)].into_iter(), )), @@ -1299,6 +1327,9 @@ impl Diagnostic for Error { Self::DuplicateTypeName { .. } => None, Self::FunctionTypeInData { .. } => None, Self::ImplicitlyDiscardedExpression { .. } => None, + Self::LastExpressionIsAssignment { .. } => Some(Box::new( + "https://aiken-lang.org/language-tour/functions#named-functions" + )), Self::IncorrectFieldsArity { .. } => Some(Box::new( "https://aiken-lang.org/language-tour/custom-types", )), diff --git a/crates/aiken-lang/src/tipo/expr.rs b/crates/aiken-lang/src/tipo/expr.rs index 30f71d43..65d17d4c 100644 --- a/crates/aiken-lang/src/tipo/expr.rs +++ b/crates/aiken-lang/src/tipo/expr.rs @@ -1,4 +1,4 @@ -use std::{collections::HashMap, sync::Arc}; +use std::{cmp::Ordering, collections::HashMap, sync::Arc}; use vec1::Vec1; @@ -191,8 +191,8 @@ impl<'a, 'b> ExprTyper<'a, 'b> { .field_map()) } - fn assert_assignment(&self, expr: &TypedExpr) -> Result<(), Error> { - if !matches!(*expr, TypedExpr::Assignment { .. }) { + fn assert_assignment(&self, expr: &UntypedExpr) -> Result<(), Error> { + if !matches!(*expr, UntypedExpr::Assignment { .. }) { return Err(Error::ImplicitlyDiscardedExpression { location: expr.location(), }); @@ -200,6 +200,16 @@ impl<'a, 'b> ExprTyper<'a, 'b> { Ok(()) } + fn assert_no_assignment(&self, expr: &UntypedExpr) -> Result<(), Error> { + match expr { + UntypedExpr::Assignment { value, .. } => Err(Error::LastExpressionIsAssignment { + location: expr.location(), + expr: *value.clone(), + }), + _ => Ok(()), + } + } + pub fn in_new_scope(&mut self, process_scope: impl FnOnce(&mut Self) -> T) -> T { // Create new scope let environment_reset_data = self.environment.open_new_scope(); @@ -1671,15 +1681,20 @@ impl<'a, 'b> ExprTyper<'a, 'b> { let mut expressions = Vec::with_capacity(count); for (i, expression) in untyped.into_iter().enumerate() { - let expression = self.infer(expression)?; + match i.cmp(&(count - 1)) { + // When the expression is the last in a sequence, we enforce it is NOT + // an assignment (kind of treat assignments like statements). + Ordering::Equal => self.assert_no_assignment(&expression)?, - // This isn't the final expression in the sequence, so it *must* - // be a let-binding; we do not allow anything else. - if i < count - 1 { - self.assert_assignment(&expression)?; + // This isn't the final expression in the sequence, so it *must* + // be a let-binding; we do not allow anything else. + Ordering::Less => self.assert_assignment(&expression)?, + + // Can't actually happen + Ordering::Greater => (), } - expressions.push(expression); + expressions.push(self.infer(expression)?); } Ok(TypedExpr::Sequence {