From ed4fe8216eb0dac5597d9e4a028488cea4dfd49e 2021-05-27 16:48:06 From: MH Date: 2021-05-27 16:48:06 Subject: [PATCH] Fix binding- and assignment-expression related typing issues. Simpler solutions are better, so the typechecker is back to normal. Instead we simply make sure that assignment expression is never nested under another expression, and binding expressions may only be nested under LogicalAnd-expressions. If only I knew why I thought type shenanigans were a good idea in the first place... --- diff --git a/src/protocol/ast.rs b/src/protocol/ast.rs index 1c85a558741dd335dee6f3e89d60059b29e3ae9f..fb2a3c6cd3524b61af5ba30a3e9b10d9b3aa6950 100644 --- a/src/protocol/ast.rs +++ b/src/protocol/ast.rs @@ -1409,6 +1409,13 @@ impl ExpressionParent { _ => false, } } + + pub fn as_expression(&self) -> ExpressionId { + match self { + ExpressionParent::Expression(id, _) => *id, + _ => panic!("called as_expression() on {:?}", self), + } + } } #[derive(Debug, Clone)] diff --git a/src/protocol/parser/pass_typing.rs b/src/protocol/parser/pass_typing.rs index 26a9543b7881db7df3230332507a112d7d9f3ac0..c490a2a446035402fd5845e6b00d04daeba2e2c7 100644 --- a/src/protocol/parser/pass_typing.rs +++ b/src/protocol/parser/pass_typing.rs @@ -66,8 +66,6 @@ use super::visitor::{ const VOID_TEMPLATE: [InferenceTypePart; 1] = [ InferenceTypePart::Void ]; const MESSAGE_TEMPLATE: [InferenceTypePart; 2] = [ InferenceTypePart::Message, InferenceTypePart::UInt8 ]; const BOOL_TEMPLATE: [InferenceTypePart; 1] = [ InferenceTypePart::Bool ]; -const BOOLLIKE_TEMPLATE: [InferenceTypePart; 1] = [ InferenceTypePart::BoolLike ]; -const BINDING_BOOL_TEMPLATE: [InferenceTypePart; 1] = [ InferenceTypePart::BindingBool ]; const CHARACTER_TEMPLATE: [InferenceTypePart; 1] = [ InferenceTypePart::Character ]; const STRING_TEMPLATE: [InferenceTypePart; 1] = [ InferenceTypePart::String ]; const NUMBERLIKE_TEMPLATE: [InferenceTypePart; 1] = [ InferenceTypePart::NumberLike ]; @@ -92,7 +90,6 @@ pub(crate) enum InferenceTypePart { // Partially known type, may be inferred to to be the appropriate related // type. // IndexLike, // index into array/slice - BoolLike, // boolean or binding boolean NumberLike, // any kind of integer/float IntegerLike, // any kind of integer ArrayLike, // array or slice. Note that this must have a subtype @@ -100,7 +97,6 @@ pub(crate) enum InferenceTypePart { // Special types that cannot be instantiated by the user Void, // For builtin functions that do not return anything // Concrete types without subtypes - BindingBool, // boolean result from a binding expression Bool, UInt8, UInt16, @@ -135,7 +131,7 @@ impl InferenceTypePart { fn is_concrete(&self) -> bool { use InferenceTypePart as ITP; match self { - ITP::Unknown | ITP::BoolLike | ITP::NumberLike | + ITP::Unknown | ITP::NumberLike | ITP::IntegerLike | ITP::ArrayLike | ITP::PortLike => false, _ => true } @@ -184,9 +180,7 @@ impl InferenceTypePart { (*self == ITP::IntegerLike && arg.is_concrete_integer()) || (*self == ITP::NumberLike && (arg.is_concrete_number() || *arg == ITP::IntegerLike)) || (*self == ITP::ArrayLike && arg.is_concrete_msg_array_or_slice()) || - (*self == ITP::PortLike && arg.is_concrete_port()) || - (*self == ITP::BoolLike && (*arg == ITP::Bool || *arg == ITP::BindingBool)) || - (*self == ITP::Bool && *arg == ITP::BindingBool) + (*self == ITP::PortLike && arg.is_concrete_port()) } /// Checks if a part is more specific @@ -198,7 +192,7 @@ impl InferenceTypePart { use InferenceTypePart as ITP; match &self { ITP::Unknown | ITP::NumberLike | ITP::IntegerLike | - ITP::Void | ITP::BoolLike | ITP::Bool | ITP::BindingBool | + ITP::Void | ITP::Bool | ITP::UInt8 | ITP::UInt16 | ITP::UInt32 | ITP::UInt64 | ITP::SInt8 | ITP::SInt16 | ITP::SInt32 | ITP::SInt64 | ITP::Character | ITP::String => { @@ -491,8 +485,7 @@ impl InferenceType { /// is false, then as long as the `to_infer` and `template` types are /// compatible the inference will succeed. If `forced_template` is true, /// then `to_infer` MUST be less specific than `template` (e.g. - /// `IntegerLike` is less specific than `UInt32`. Likewise `Bool` is less - /// specific than `BindingBool`) + /// `IntegerLike` is less specific than `UInt32`) fn infer_subtree_for_single_type( to_infer: &mut InferenceType, mut to_infer_idx: usize, template: &[InferenceTypePart], mut template_idx: usize, @@ -613,7 +606,7 @@ impl InferenceType { idx += 1; continue; }, - ITP::Unknown | ITP::BoolLike | ITP::NumberLike | + ITP::Unknown | ITP::NumberLike | ITP::IntegerLike | ITP::ArrayLike | ITP::PortLike => { // Should not happen if type inferencing works correctly: we // should have returned a programmer-readable error or have @@ -622,7 +615,6 @@ impl InferenceType { }, ITP::Void => CTP::Void, ITP::Message => CTP::Message, - ITP::BindingBool => CTP::Bool, ITP::Bool => CTP::Bool, ITP::UInt8 => CTP::UInt8, ITP::UInt16 => CTP::UInt16, @@ -661,7 +653,6 @@ impl InferenceType { idx = Self::write_display_name(buffer, heap, parts, idx + 1); }, ITP::Unknown => buffer.push_str("?"), - ITP::BoolLike => buffer.push_str("boollike"), ITP::NumberLike => buffer.push_str("numberlike"), ITP::IntegerLike => buffer.push_str("integerlike"), ITP::ArrayLike => { @@ -674,7 +665,6 @@ impl InferenceType { buffer.push('>'); } ITP::Void => buffer.push_str("void"), - ITP::BindingBool => buffer.push_str("binding result"), ITP::Bool => buffer.push_str(KW_TYPE_BOOL_STR), ITP::UInt8 => buffer.push_str(KW_TYPE_UINT8_STR), ITP::UInt16 => buffer.push_str(KW_TYPE_UINT16_STR), @@ -1671,9 +1661,9 @@ impl PassTyping { let bound_from_id = binding_expr.bound_from; let bound_to_id = binding_expr.bound_to; - // Output of a binding expression is a special kind of boolean that can - // only be used in binary-and expressions - let progress_expr = self.apply_forced_constraint(ctx, upcast_id, &BINDING_BOOL_TEMPLATE)?; + // Output is always a boolean. The two arguments should be of equal + // type. + let progress_expr = self.apply_forced_constraint(ctx, upcast_id, &BOOL_TEMPLATE)?; let (progress_from, progress_to) = self.apply_equal2_constraint(ctx, upcast_id, bound_from_id, 0, bound_to_id, 0)?; if progress_expr { self.queue_expr_parent(ctx, upcast_id); } @@ -1696,6 +1686,10 @@ impl PassTyping { debug_log!(" - Arg2 type: {}", self.temp_get_display_name(ctx, arg2_expr_id)); debug_log!(" - Expr type: {}", self.temp_get_display_name(ctx, upcast_id)); + // I keep confusing myself: this applies equality of types between the + // condition branches' types, and the result from the conditional + // expression, because the result from the conditional is one of the + // branches. let (progress_expr, progress_arg1, progress_arg2) = self.apply_equal3_constraint( ctx, upcast_id, arg1_expr_id, arg2_expr_id, 0 )?; @@ -1742,16 +1736,12 @@ impl PassTyping { (progress_expr || subtype_expr, progress_arg1 || subtype_arg1, progress_arg2 || subtype_arg2) }, BO::LogicalAnd => { - // Logical AND may operate both on normal booleans and on - // booleans that are the result of a binding expression. So we - // force the expression to bool-like, then apply an equal_3 - // constraint. Any BindingBool will promote all the other Bool - // types. - let base_expr = self.apply_template_constraint(ctx, upcast_id, &BOOLLIKE_TEMPLATE)?; - let (progress_expr, progress_arg1, progress_arg2) = - self.apply_equal3_constraint(ctx, upcast_id, arg1_id, arg2_id, 0)?; + // Forced boolean on all + let progress_expr = self.apply_forced_constraint(ctx, upcast_id, &BOOL_TEMPLATE)?; + let progress_arg1 = self.apply_forced_constraint(ctx, upcast_id, &BOOL_TEMPLATE)?; + let progress_arg2 = self.apply_forced_constraint(ctx, upcast_id, &BOOL_TEMPLATE)?; - (base_expr || progress_expr, progress_arg1, progress_arg2) + (progress_expr, progress_arg1, progress_arg2) }, BO::LogicalOr => { // Forced boolean on all @@ -1771,7 +1761,7 @@ impl PassTyping { }, BO::Equality | BO::Inequality => { // Equal2 on args, forced boolean output - let progress_expr = self.apply_template_constraint(ctx, upcast_id, &BOOLLIKE_TEMPLATE)?; + let progress_expr = self.apply_forced_constraint(ctx, upcast_id, &BOOL_TEMPLATE)?; let (progress_arg1, progress_arg2) = self.apply_equal2_constraint(ctx, upcast_id, arg1_id, 0, arg2_id, 0)?; @@ -1779,7 +1769,7 @@ impl PassTyping { }, BO::LessThan | BO::GreaterThan | BO::LessThanEqual | BO::GreaterThanEqual => { // Equal2 on args with numberlike type, forced boolean output - let progress_expr = self.apply_template_constraint(ctx, upcast_id, &BOOLLIKE_TEMPLATE)?; + let progress_expr = self.apply_forced_constraint(ctx, upcast_id, &BOOL_TEMPLATE)?; let progress_arg_base = self.apply_template_constraint(ctx, arg1_id, &NUMBERLIKE_TEMPLATE)?; let (progress_arg1, progress_arg2) = self.apply_equal2_constraint(ctx, upcast_id, arg1_id, 0, arg2_id, 0)?; @@ -1838,7 +1828,7 @@ impl PassTyping { (progress_base || progress_expr, progress_base || progress_arg) }, UO::LogicalNot => { - // Both booleans + // Both bools let progress_expr = self.apply_forced_constraint(ctx, upcast_id, &BOOL_TEMPLATE)?; let progress_arg = self.apply_forced_constraint(ctx, upcast_id, &BOOL_TEMPLATE)?; (progress_expr, progress_arg) diff --git a/src/protocol/parser/pass_validation_linking.rs b/src/protocol/parser/pass_validation_linking.rs index cfd28c4f3ab906c4d244ccb119ba7adc77f74ec5..99a7ea702e46371d4a3e8ce1834deeb2f53bb9c4 100644 --- a/src/protocol/parser/pass_validation_linking.rs +++ b/src/protocol/parser/pass_validation_linking.rs @@ -406,6 +406,16 @@ impl Visitor2 for PassValidationLinking { let assignment_expr = &mut ctx.heap[id]; + // Although we call assignment an expression to simplify the compiler's + // code (mainly typechecking), we disallow nested use in expressions + match self.expr_parent { + ExpressionParent::ExpressionStmt(_) => {}, + _ => return Err(ParseError::new_error_str_at_span( + &ctx.module.source, assignment_expr.span, + "assignments may only appear at the statement level" + )), + } + let left_expr_id = assignment_expr.left; let right_expr_id = assignment_expr.right; let old_expr_parent = self.expr_parent; @@ -425,7 +435,6 @@ impl Visitor2 for PassValidationLinking { fn visit_binding_expr(&mut self, ctx: &mut Ctx, id: BindingExpressionId) -> VisitorResult { let upcast_id = id.upcast(); - let binding_expr = &mut ctx.heap[id]; // Check for valid context of binding expression if let Some(span) = self.must_be_assignable { @@ -435,6 +444,7 @@ impl Visitor2 for PassValidationLinking { } if self.in_test_expr.is_invalid() { + let binding_expr = &ctx.heap[id]; return Err(ParseError::new_error_str_at_span( &ctx.module.source, binding_expr.span, "binding expressions can only be used inside the testing expression of 'if' and 'while' statements" @@ -453,6 +463,51 @@ impl Visitor2 for PassValidationLinking { )); } + let mut seeking_parent = self.expr_parent; + loop { + // Perform upward search to make sure only LogicalAnd is applied to + // the binding expression + let valid = match seeking_parent { + ExpressionParent::If(_) | ExpressionParent::While(_) => { + // Every parent expression (if any) were LogicalAnd. + break; + } + ExpressionParent::Expression(parent_id, _) => { + let parent_expr = &ctx.heap[parent_id]; + match parent_expr { + Expression::Binary(parent_expr) => { + // Set new parent to continue the search. Otherwise + // halt and provide an error using the current + // parent. + if parent_expr.operation == BinaryOperator::LogicalAnd { + seeking_parent = parent_expr.parent; + true + } else { + false + } + }, + _ => false, + } + }, + _ => unreachable!(), // nested under if/while, so always expressions as parents + }; + + if !valid { + let binding_expr = &ctx.heap[id]; + let parent_expr = &ctx.heap[seeking_parent.as_expression()]; + return Err(ParseError::new_error_str_at_span( + &ctx.module.source, binding_expr.span, + "only the logical-and operator (&&) may be applied to binding expressions" + ).with_info_str_at_span( + &ctx.module.source, parent_expr.span(), + "this was the disallowed operation applied to the result from a binding expression" + )); + } + } + + // Perform all of the index/parent assignment magic + let binding_expr = &mut ctx.heap[id]; + let old_expr_parent = self.expr_parent; binding_expr.parent = old_expr_parent; binding_expr.unique_id_in_definition = self.next_expr_index; diff --git a/src/protocol/tests/parser_binding.rs b/src/protocol/tests/parser_binding.rs index 6fcab16a2354fa7b50c2021042976446f72051ff..4aa276e9371a3b41772ecf8e7feb2b99b8c4b935 100644 --- a/src/protocol/tests/parser_binding.rs +++ b/src/protocol/tests/parser_binding.rs @@ -109,9 +109,11 @@ fn test_boolean_ops_on_binding() { return 0; } ").error(|e| { e - .assert_num(1) - .assert_occurs_at(0, "let") - .assert_msg_has(0, "expected a 'bool'"); + .assert_num(2) + .assert_occurs_at(0, "let a") + .assert_msg_has(0, "only the logical-and operator") + .assert_occurs_at(1, "||") + .assert_msg_has(1, "disallowed operation"); }); Tester::new_single_source_expect_err("apply || after binding", " @@ -123,9 +125,11 @@ fn test_boolean_ops_on_binding() { return 0; } ").error(|e| { e - .assert_num(1) - .assert_occurs_at(0, "let") - .assert_msg_has(0, "expected a 'bool'"); + .assert_num(2) + .assert_occurs_at(0, "let b") + .assert_msg_has(0, "only the logical-and operator") + .assert_occurs_at(1, "||") + .assert_msg_has(1, "disallowed operation"); }); Tester::new_single_source_expect_err("apply || before and after binding", " @@ -137,7 +141,10 @@ fn test_boolean_ops_on_binding() { return 0; } ").error(|e| { e - .assert_num(1) - .assert_msg_has(0, "expected a 'bool'"); + .assert_num(2) + .assert_occurs_at(0, "let a") + .assert_msg_has(0, "only the logical-and operator") + .assert_occurs_at(1, "|| (let a") + .assert_msg_has(1, "disallowed operation"); }); } \ No newline at end of file