Merge pull request #299 from aiken-lang/forbid-assignment-as-last-expression

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 {

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,
@@ -51,7 +52,13 @@ 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 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 {
@@ -375,9 +382,10 @@ 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::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")),
@@ -494,7 +502,30 @@ 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::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 {keyword_let} keyword. If you really wish to discard an expression that is unused, you can assign it to '{discard}'.
+                "#
+                , 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::<Vec<_>>()
+                    .join("\n")
+            })),
+
             Self::IncorrectFieldsArity { .. } => None,
 
             Self::IncorrectFunctionCallArity { expected, .. } => Some(Box::new(formatdoc! {
@@ -1156,8 +1187,10 @@ 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(
-            Self::ImplicityDiscardedExpression { 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(
@@ -1293,7 +1326,10 @@ impl Diagnostic for Error {
             Self::DuplicateName { .. } => None,
             Self::DuplicateTypeName { .. } => None,
             Self::FunctionTypeInData { .. } => None,
-            Self::ImplicityDiscardedExpression { .. } => 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",
             )),

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;
 
@@ -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,25 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
             .field_map())
     }
 
+    fn assert_assignment(&self, expr: &UntypedExpr) -> Result<(), Error> {
+        if !matches!(*expr, UntypedExpr::Assignment { .. }) {
+            return Err(Error::ImplicitlyDiscardedExpression {
+                location: expr.location(),
+            });
+        }
+        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<T>(&mut self, process_scope: impl FnOnce(&mut Self) -> T) -> T {
         // Create new scope
         let environment_reset_data = self.environment.open_new_scope();
@@ -1681,22 +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)) {
-            // This isn't the final expression in the sequence, so call the
+                // When the expression is the last in a sequence, we enforce it is NOT
-            // `expression_discarded` function to see if anything is being
+                // an assignment (kind of treat assignments like statements).
-            // discarded that we think shouldn't be. We also want to make sure
+                Ordering::Equal => self.assert_no_assignment(&expression)?,
-            // that there are no implicitly discarded expressions
-            if i < count - 1 {
-                // self.expression_discarded(&expression);
 
-                if !matches!(expression, TypedExpr::Assignment { .. }) {
+                // This isn't the final expression in the sequence, so it *must*
-                    return Err(Error::ImplicityDiscardedExpression {
+                // be a let-binding; we do not allow anything else.
-                        location: expression.location(),
+                Ordering::Less => self.assert_assignment(&expression)?,
-                    });
+
-                }
+                // Can't actually happen
+                Ordering::Greater => (),
             }
 
-            expressions.push(expression);
+            expressions.push(self.infer(expression)?);
         }
 
         Ok(TypedExpr::Sequence {