From 9aa9070f562c8f9f946c9f981ae2f4b9ccee56fb Mon Sep 17 00:00:00 2001 From: KtorZ Date: Wed, 21 Aug 2024 14:31:57 +0200 Subject: [PATCH] Revise desugaring following feedback - We now consistently desugar an expect in the last position as `Void`. Regardless of the pattern. Desugaring to a boolean value is deemed too confusing. - This commit also removes the desugaring for let-binding. It's only ever allowed for _expect_ which then behaves like a side effect. - We also now allow tests to return either `Bool` or `Void`. A test that returns `Void` is treated the same as a test returning `True`. --- crates/aiken-lang/src/ast.rs | 20 --- crates/aiken-lang/src/expr.rs | 21 +-- .../snapshots/def_invalid_property_test.snap | 9 +- .../snapshots/def_property_test.snap | 9 +- .../def_property_test_annotated_fuzzer.snap | 9 +- .../parser/definition/snapshots/def_test.snap | 9 +- .../definition/snapshots/def_test_fail.snap | 9 +- .../aiken-lang/src/parser/definition/test.rs | 2 +- crates/aiken-lang/src/tests/check.rs | 160 +++++++++++++++--- crates/aiken-lang/src/tipo/error.rs | 18 +- crates/aiken-lang/src/tipo/expr.rs | 35 ++-- crates/aiken-lang/src/tipo/infer.rs | 17 +- crates/uplc/src/machine/eval_result.rs | 6 +- 13 files changed, 196 insertions(+), 128 deletions(-) diff --git a/crates/aiken-lang/src/ast.rs b/crates/aiken-lang/src/ast.rs index 2a14012f..f3b0782d 100644 --- a/crates/aiken-lang/src/ast.rs +++ b/crates/aiken-lang/src/ast.rs @@ -1423,26 +1423,6 @@ impl UntypedPattern { is_record: false, } } - - /// Returns Some() if the pattern is a [`Boolean`] literal, - /// holding the target value. None if it isn't a bool pattern. - pub fn get_bool(&self) -> Option { - match self { - Self::Constructor { - module: None, - name, - constructor: (), - .. - } if name == "True" => Some(true), - Self::Constructor { - module: None, - name, - constructor: (), - .. - } if name == "False" => Some(false), - _ => None, - } - } } impl TypedPattern { diff --git a/crates/aiken-lang/src/expr.rs b/crates/aiken-lang/src/expr.rs index 2749d9bf..2af362ea 100644 --- a/crates/aiken-lang/src/expr.rs +++ b/crates/aiken-lang/src/expr.rs @@ -8,7 +8,7 @@ pub(crate) use crate::{ TypedDataType, TypedIfBranch, TypedRecordUpdateArg, UnOp, UntypedArg, UntypedAssignmentKind, UntypedClause, UntypedIfBranch, UntypedRecordUpdateArg, }, - builtins::{bool, void}, + builtins::void, parser::token::Base, tipo::{ check_replaceable_opaque_type, convert_opaque_type, lookup_data_type_by_tipo, @@ -492,25 +492,6 @@ impl TypedExpr { location, } } - - pub fn bool(value: bool, location: Span) -> Self { - TypedExpr::Var { - name: "Bool".to_string(), - constructor: ValueConstructor { - public: true, - variant: ValueConstructorVariant::Record { - name: if value { "True" } else { "False" }.to_string(), - arity: 0, - field_map: None, - location: Span::empty(), - module: String::new(), - constructors_count: 2, - }, - tipo: bool(), - }, - location, - } - } } // Represent how a function was written so that we can format it back. diff --git a/crates/aiken-lang/src/parser/definition/snapshots/def_invalid_property_test.snap b/crates/aiken-lang/src/parser/definition/snapshots/def_invalid_property_test.snap index b7b65666..8d46100a 100644 --- a/crates/aiken-lang/src/parser/definition/snapshots/def_invalid_property_test.snap +++ b/crates/aiken-lang/src/parser/definition/snapshots/def_invalid_property_test.snap @@ -52,14 +52,7 @@ Test( location: 0..26, name: "foo", public: false, - return_annotation: Some( - Constructor { - location: 0..39, - module: None, - name: "Bool", - arguments: [], - }, - ), + return_annotation: None, return_type: (), end_position: 38, on_test_failure: FailImmediately, diff --git a/crates/aiken-lang/src/parser/definition/snapshots/def_property_test.snap b/crates/aiken-lang/src/parser/definition/snapshots/def_property_test.snap index f2ba0716..3a806be9 100644 --- a/crates/aiken-lang/src/parser/definition/snapshots/def_property_test.snap +++ b/crates/aiken-lang/src/parser/definition/snapshots/def_property_test.snap @@ -37,14 +37,7 @@ Test( location: 0..28, name: "foo", public: false, - return_annotation: Some( - Constructor { - location: 0..41, - module: None, - name: "Bool", - arguments: [], - }, - ), + return_annotation: None, return_type: (), end_position: 40, on_test_failure: FailImmediately, diff --git a/crates/aiken-lang/src/parser/definition/snapshots/def_property_test_annotated_fuzzer.snap b/crates/aiken-lang/src/parser/definition/snapshots/def_property_test_annotated_fuzzer.snap index 721edd75..31b1a283 100644 --- a/crates/aiken-lang/src/parser/definition/snapshots/def_property_test_annotated_fuzzer.snap +++ b/crates/aiken-lang/src/parser/definition/snapshots/def_property_test_annotated_fuzzer.snap @@ -44,14 +44,7 @@ Test( location: 0..26, name: "foo", public: false, - return_annotation: Some( - Constructor { - location: 0..39, - module: None, - name: "Bool", - arguments: [], - }, - ), + return_annotation: None, return_type: (), end_position: 38, on_test_failure: FailImmediately, diff --git a/crates/aiken-lang/src/parser/definition/snapshots/def_test.snap b/crates/aiken-lang/src/parser/definition/snapshots/def_test.snap index daddcb5f..42561931 100644 --- a/crates/aiken-lang/src/parser/definition/snapshots/def_test.snap +++ b/crates/aiken-lang/src/parser/definition/snapshots/def_test.snap @@ -13,14 +13,7 @@ Test( location: 0..10, name: "foo", public: false, - return_annotation: Some( - Constructor { - location: 0..23, - module: None, - name: "Bool", - arguments: [], - }, - ), + return_annotation: None, return_type: (), end_position: 22, on_test_failure: FailImmediately, diff --git a/crates/aiken-lang/src/parser/definition/snapshots/def_test_fail.snap b/crates/aiken-lang/src/parser/definition/snapshots/def_test_fail.snap index b8916bcf..4b0d4cb4 100644 --- a/crates/aiken-lang/src/parser/definition/snapshots/def_test_fail.snap +++ b/crates/aiken-lang/src/parser/definition/snapshots/def_test_fail.snap @@ -44,14 +44,7 @@ Test( location: 0..26, name: "invalid_inputs", public: false, - return_annotation: Some( - Constructor { - location: 0..61, - module: None, - name: "Bool", - arguments: [], - }, - ), + return_annotation: None, return_type: (), end_position: 60, on_test_failure: SucceedEventually, diff --git a/crates/aiken-lang/src/parser/definition/test.rs b/crates/aiken-lang/src/parser/definition/test.rs index 0931c30d..6e4cada8 100644 --- a/crates/aiken-lang/src/parser/definition/test.rs +++ b/crates/aiken-lang/src/parser/definition/test.rs @@ -45,7 +45,7 @@ pub fn parser() -> impl Parser { + let wow = 1 + } + None -> { + 2 + } + } + } + "#; + + assert!(matches!( + check(parse(source_code)), + Err((_, Error::LastExpressionIsAssignment { .. })) + )) +} + +#[test] +fn assignement_last_expr_if_first_branch() { + let source_code = r#" + pub fn foo() { + if True { + let thing = 1 + } else { + 1 + } + } + "#; + + assert!(matches!( + check(parse(source_code)), + Err((_, Error::LastExpressionIsAssignment { .. })) + )) +} + +#[test] +fn assignement_last_expr_if_branches() { + let source_code = r#" + pub fn foo() { + if True { + 2 + } else if False { + let thing = 1 + } else { + 1 + } + } + "#; + + assert!(matches!( + check(parse(source_code)), + Err((_, Error::LastExpressionIsAssignment { .. })) + )) +} + +#[test] +fn assignement_last_expr_if_final_else() { + let source_code = r#" + pub fn foo() { + if True { + 1 + } else { + let thing = 1 + } + } + "#; + + assert!(matches!( + check(parse(source_code)), + Err((_, Error::LastExpressionIsAssignment { .. })) + )) +} + +#[test] +fn assignment_last_expr_logical_chain() { + let source_code = r#" + pub fn foo() -> Bool { + and { + expect 1 + 1 == 2, + True, + 2 > 0, + or { + expect True, + False, + } + } + } + "#; + + assert!(matches!( + check(parse(source_code)), + Err((_, Error::LastExpressionIsAssignment { .. })) + )) +} + #[test] fn if_scoping() { let source_code = r#" @@ -1668,8 +1769,7 @@ fn pipe_wrong_arity_fully_saturated_return_fn() { fn fuzzer_ok_basic() { let source_code = r#" fn int() -> Fuzzer { todo } - - test prop(n via int()) { todo } + test prop(n via int()) { True } "#; assert!(check(parse(source_code)).is_ok()); @@ -1679,8 +1779,7 @@ fn fuzzer_ok_basic() { fn fuzzer_ok_explicit() { let source_code = r#" fn int(prng: PRNG) -> Option<(PRNG, Int)> { todo } - - test prop(n via int) { todo } + test prop(n via int) { Void } "#; assert!(check(parse(source_code)).is_ok()); @@ -1692,7 +1791,7 @@ fn fuzzer_ok_list() { fn int() -> Fuzzer { todo } fn list(a: Fuzzer) -> Fuzzer> { todo } - test prop(xs via list(int())) { todo } + test prop(xs via list(int())) { True } "#; assert!(check(parse(source_code)).is_ok()); @@ -2906,24 +3005,19 @@ fn recover_no_assignment_when_clause() { let source_code = r#" pub fn main(foo) { when foo is { - [] -> let bar = foo + [] -> Void [x, ..] -> expect _: Int = x } } "#; - let (warnings, _) = check(parse(source_code)).unwrap(); - - assert!(matches!( - &warnings[..], - [Warning::UnusedVariable { name, .. }] if name == "bar", - )) + assert!(check(parse(source_code)).is_ok()); } #[test] fn recover_no_assignment_fn_if_then_else() { let source_code = r#" - pub fn foo(weird_maths) -> Bool { + pub fn foo(weird_maths) -> Void { if weird_maths { expect 1 == 2 } else { @@ -2936,20 +3030,38 @@ fn recover_no_assignment_fn_if_then_else() { } #[test] -fn recover_no_assignment_logical_chain_op() { +fn test_return_explicit_void() { let source_code = r#" - pub fn foo() -> Bool { - and { - expect 1 + 1 == 2, - True, - 2 > 0, - or { - expect True, - False, - } - } + test foo() { + Void } "#; assert!(check(parse(source_code)).is_ok()); } + +#[test] +fn test_return_implicit_void() { + let source_code = r#" + test foo() { + let data: Data = 42 + expect _: Int = data + } + "#; + + assert!(check(parse(source_code)).is_ok()); +} + +#[test] +fn test_return_illegal() { + let source_code = r#" + test foo() { + 42 + } + "#; + + assert!(matches!( + check(parse(source_code)), + Err((_, Error::IllegalTestType { .. })) + )) +} diff --git a/crates/aiken-lang/src/tipo/error.rs b/crates/aiken-lang/src/tipo/error.rs index 0fe9293d..8f4fa449 100644 --- a/crates/aiken-lang/src/tipo/error.rs +++ b/crates/aiken-lang/src/tipo/error.rs @@ -2,7 +2,7 @@ use super::Type; use crate::{ ast::{Annotation, BinOp, CallArg, LogicalOpChainKind, Span, UntypedFunction, UntypedPattern}, error::ExtraData, - expr::{self, AssignmentPattern, UntypedExpr}, + expr::{self, AssignmentPattern, UntypedAssignmentKind, UntypedExpr}, format::Formatter, levenshtein, pretty::Documentable, @@ -472,6 +472,7 @@ If you really meant to return that last expression, try to replace it with the f location: Span, expr: expr::UntypedExpr, patterns: Vec1, + kind: UntypedAssignmentKind, }, #[error( @@ -1025,7 +1026,7 @@ The best thing to do from here is to remove it."#))] }, #[error("I caught a test with too many arguments.\n")] - #[diagnostic(code("illegal::test_arity"))] + #[diagnostic(code("illegal::test::arity"))] #[diagnostic(help( "Tests are allowed to have 0 or 1 argument, but no more. Here I've found a test definition with {count} arguments. If you need to provide multiple values to a test, use a Record or a Tuple.", ))] @@ -1035,6 +1036,18 @@ The best thing to do from here is to remove it."#))] location: Span, }, + #[error("I caught a test with an illegal return type.\n")] + #[diagnostic(code("illegal::test::return"))] + #[diagnostic(help( + "Tests must return either {Bool} or {Void}. Note that `expect` assignment are implicitly typed {Void} (and thus, may be the last expression of a test).", + Bool = "Bool".if_supports_color(Stderr, |s| s.cyan()), + Void = "Void".if_supports_color(Stderr, |s| s.cyan()), + ))] + IllegalTestType { + #[label("expected Bool or Void")] + location: Span, + }, + #[error("I choked on a generic type left in an outward-facing interface.\n")] #[diagnostic(code("illegal::generic_in_abi"))] #[diagnostic(help( @@ -1104,6 +1117,7 @@ impl ExtraData for Error { | Error::UpdateMultiConstructorType { .. } | Error::ValidatorImported { .. } | Error::IncorrectTestArity { .. } + | Error::IllegalTestType { .. } | Error::GenericLeftAtBoundary { .. } | Error::UnexpectedMultiPatternAssignment { .. } | Error::ExpectOnOpaqueType { .. } diff --git a/crates/aiken-lang/src/tipo/expr.rs b/crates/aiken-lang/src/tipo/expr.rs index 7c0041fc..1df80e83 100644 --- a/crates/aiken-lang/src/tipo/expr.rs +++ b/crates/aiken-lang/src/tipo/expr.rs @@ -1803,17 +1803,8 @@ impl<'a, 'b> ExprTyper<'a, 'b> { let mut typed_expressions = vec![]; for expression in expressions { - let typed_expression = if let Some(filler) = recover_from_no_assignment( - assert_no_assignment(&expression), - expression.location(), - )? { - TypedExpr::Sequence { - location: expression.location(), - expressions: vec![self.infer(expression)?, filler], - } - } else { - self.infer(expression)? - }; + assert_no_assignment(&expression)?; + let typed_expression = self.infer(expression)?; self.unify( bool(), @@ -2545,24 +2536,32 @@ fn recover_from_no_assignment( result: Result<(), Error>, span: Span, ) -> Result, Error> { - if let Err(Error::LastExpressionIsAssignment { patterns, .. }) = result { - match patterns.first().pattern.get_bool() { - Some(expected) if patterns.len() == 1 => Ok(Some(TypedExpr::bool(expected, span))), - _ => Ok(Some(TypedExpr::void(span))), + if let Err(Error::LastExpressionIsAssignment { + ref patterns, + ref kind, + .. + }) = result + { + if matches!(kind, AssignmentKind::Expect { ..} if patterns.len() == 1) { + return Ok(Some(TypedExpr::void(span))); } - } else { - result.map(|()| None) } + + result.map(|()| None) } fn assert_no_assignment(expr: &UntypedExpr) -> Result<(), Error> { match expr { UntypedExpr::Assignment { - value, patterns, .. + value, + patterns, + kind, + .. } => Err(Error::LastExpressionIsAssignment { location: expr.location(), expr: *value.clone(), patterns: patterns.clone(), + kind: *kind, }), UntypedExpr::Trace { then, .. } => assert_no_assignment(then), UntypedExpr::Fn { .. } diff --git a/crates/aiken-lang/src/tipo/infer.rs b/crates/aiken-lang/src/tipo/infer.rs index 8f9a37a6..a3e0ef6b 100644 --- a/crates/aiken-lang/src/tipo/infer.rs +++ b/crates/aiken-lang/src/tipo/infer.rs @@ -392,12 +392,25 @@ fn infer_definition( let typed_f = infer_function(&f.into(), module_name, hydrators, environment, tracing)?; - environment.unify( + let is_bool = environment.unify( typed_f.return_type.clone(), builtins::bool(), typed_f.location, false, - )?; + ); + + let is_void = environment.unify( + typed_f.return_type.clone(), + builtins::void(), + typed_f.location, + false, + ); + + if is_bool.or(is_void).is_err() { + return Err(Error::IllegalTestType { + location: typed_f.location, + }); + } Ok(Definition::Test(Function { doc: typed_f.doc, diff --git a/crates/uplc/src/machine/eval_result.rs b/crates/uplc/src/machine/eval_result.rs index 572d2276..364d4869 100644 --- a/crates/uplc/src/machine/eval_result.rs +++ b/crates/uplc/src/machine/eval_result.rs @@ -39,7 +39,11 @@ impl EvalResult { } else { self.result.is_err() || matches!(self.result, Ok(Term::Error)) - || !matches!(self.result, Ok(Term::Constant(ref con)) if matches!(con.as_ref(), Constant::Bool(true))) + || !matches!( + self.result, + Ok(Term::Constant(ref con)) + if matches!(con.as_ref(), Constant::Bool(true)) || matches!(con.as_ref(), Constant::Unit) + ) } }