Merge pull request #826 from rtfeldman/bughunt

Throw on overflow in Num.neg/Num.abs & fix mono bug

cli/tests/repl_eval.rs

@@ -327,13 +327,16 @@ mod repl_eval {
         expect_success("List.sum [ 1.1, 2.2, 3.3 ]", "6.6 : F64");
     }
 
-    // TODO add test cases for empty lists once error messages in the repl are correct
     #[test]
     fn list_first() {
         expect_success(
             "List.first [ 12, 9, 6, 3 ]",
             "Ok 12 : Result (Num *) [ ListWasEmpty ]*",
         );
+        expect_success(
+            "List.first []",
+            "Err (ListWasEmpty) : Result * [ ListWasEmpty ]*",
+        );
     }
 
     #[test]
@@ -342,6 +345,11 @@ mod repl_eval {
             "List.last [ 12, 9, 6, 3 ]",
             "Ok 3 : Result (Num *) [ ListWasEmpty ]*",
         );
+
+        expect_success(
+            "List.last []",
+            "Err (ListWasEmpty) : Result * [ ListWasEmpty ]*",
+        );
     }
 
     #[test]

compiler/gen/src/llvm/build.rs

@@ -9,7 +9,8 @@ use crate::llvm::build_str::{
 };
 use crate::llvm::compare::{build_eq, build_neq};
 use crate::llvm::convert::{
-    basic_type_from_layout, block_of_memory, collection, get_fn_type, get_ptr_type, ptr_int,
+    basic_type_from_builtin, basic_type_from_layout, block_of_memory, collection, get_fn_type,
+    get_ptr_type, ptr_int,
 };
 use crate::llvm::refcounting::{
     decrement_refcount_layout, increment_refcount_layout, refcount_is_one_comparison,
@@ -2909,7 +2910,7 @@ fn run_low_level<'a, 'ctx, 'env>(
 
                     match arg_builtin {
                         Int128 | Int64 | Int32 | Int16 | Int8 => {
-                            build_int_unary_op(env, arg.into_int_value(), arg_layout, op)
+                            build_int_unary_op(env, arg.into_int_value(), arg_builtin, op)
                         }
                         Float128 | Float64 | Float32 | Float16 => {
                             build_float_unary_op(env, arg.into_float_value(), op)
@@ -3574,10 +3575,37 @@ fn build_float_binop<'a, 'ctx, 'env>(
     }
 }
 
+fn int_type_signed_min(int_type: IntType) -> IntValue {
+    let width = int_type.get_bit_width();
+
+    debug_assert!(width <= 128);
+    let shift = 128 - width as usize;
+
+    if shift < 64 {
+        let min = i128::MIN >> shift;
+        let a = min as u64;
+        let b = (min >> 64) as u64;
+
+        int_type.const_int_arbitrary_precision(&[b, a])
+    } else {
+        int_type.const_int((i128::MIN >> shift) as u64, false)
+    }
+}
+
+fn builtin_to_int_type<'a, 'ctx, 'env>(
+    env: &Env<'a, 'ctx, 'env>,
+    builtin: &Builtin<'a>,
+) -> IntType<'ctx> {
+    let result = basic_type_from_builtin(env.arena, env.context, builtin, env.ptr_bytes);
+    debug_assert!(result.is_int_type());
+
+    result.into_int_type()
+}
+
 fn build_int_unary_op<'a, 'ctx, 'env>(
     env: &Env<'a, 'ctx, 'env>,
     arg: IntValue<'ctx>,
-    arg_layout: &Layout<'a>,
+    arg_layout: &Builtin<'a>,
     op: LowLevel,
 ) -> BasicValueEnum<'ctx> {
     use roc_module::low_level::LowLevel::*;
@@ -3585,43 +3613,13 @@ fn build_int_unary_op<'a, 'ctx, 'env>(
     let bd = env.builder;
 
     match op {
-        NumNeg => bd.build_int_neg(arg, "negate_int").into(),
+        NumNeg => {
+            // integer abs overflows when applied to the minimum value of a signed type
+            int_neg_raise_on_overflow(env, arg, arg_layout)
+        }
         NumAbs => {
-            // This is how libc's abs() is implemented - it uses no branching!
+            // integer abs overflows when applied to the minimum value of a signed type
-            //
+            int_abs_raise_on_overflow(env, arg, arg_layout)
-            //     abs = \arg ->
-            //         shifted = arg >>> 63
-            //
-            //         (xor arg shifted) - shifted
-
-            let ctx = env.context;
-            let shifted_name = "abs_shift_right";
-            let shifted_alloca = {
-                let bits_to_shift = ((arg_layout.stack_size(env.ptr_bytes) as u64) * 8) - 1;
-                let shift_val = ctx.i64_type().const_int(bits_to_shift, false);
-                let shifted = bd.build_right_shift(arg, shift_val, true, shifted_name);
-                let alloca = bd.build_alloca(
-                    basic_type_from_layout(env.arena, ctx, arg_layout, env.ptr_bytes),
-                    "#int_abs_help",
-                );
-
-                // shifted = arg >>> 63
-                bd.build_store(alloca, shifted);
-
-                alloca
-            };
-
-            let xored_arg = bd.build_xor(
-                arg,
-                bd.build_load(shifted_alloca, shifted_name).into_int_value(),
-                "xor_arg_shifted",
-            );
-
-            BasicValueEnum::IntValue(bd.build_int_sub(
-                xored_arg,
-                bd.build_load(shifted_alloca, shifted_name).into_int_value(),
-                "sub_xored_shifted",
-            ))
         }
         NumToFloat => {
             // This is an Int, so we need to convert it.
@@ -3638,6 +3636,109 @@ fn build_int_unary_op<'a, 'ctx, 'env>(
     }
 }
 
+fn int_neg_raise_on_overflow<'a, 'ctx, 'env>(
+    env: &Env<'a, 'ctx, 'env>,
+    arg: IntValue<'ctx>,
+    builtin: &Builtin<'a>,
+) -> BasicValueEnum<'ctx> {
+    let builder = env.builder;
+
+    let min_val = int_type_signed_min(builtin_to_int_type(env, builtin));
+    let condition = builder.build_int_compare(IntPredicate::EQ, arg, min_val, "is_min_val");
+
+    let block = env.builder.get_insert_block().expect("to be in a function");
+    let parent = block.get_parent().expect("to be in a function");
+    let then_block = env.context.append_basic_block(parent, "then");
+    let else_block = env.context.append_basic_block(parent, "else");
+
+    env.builder
+        .build_conditional_branch(condition, then_block, else_block);
+
+    builder.position_at_end(then_block);
+
+    throw_exception(
+        env,
+        "integer negation overflowed because its argument is the minimum value",
+    );
+
+    builder.position_at_end(else_block);
+
+    builder.build_int_neg(arg, "negate_int").into()
+}
+
+fn int_abs_raise_on_overflow<'a, 'ctx, 'env>(
+    env: &Env<'a, 'ctx, 'env>,
+    arg: IntValue<'ctx>,
+    builtin: &Builtin<'a>,
+) -> BasicValueEnum<'ctx> {
+    let builder = env.builder;
+
+    let min_val = int_type_signed_min(builtin_to_int_type(env, builtin));
+    let condition = builder.build_int_compare(IntPredicate::EQ, arg, min_val, "is_min_val");
+
+    let block = env.builder.get_insert_block().expect("to be in a function");
+    let parent = block.get_parent().expect("to be in a function");
+    let then_block = env.context.append_basic_block(parent, "then");
+    let else_block = env.context.append_basic_block(parent, "else");
+
+    env.builder
+        .build_conditional_branch(condition, then_block, else_block);
+
+    builder.position_at_end(then_block);
+
+    throw_exception(
+        env,
+        "integer absolute overflowed because its argument is the minimum value",
+    );
+
+    builder.position_at_end(else_block);
+
+    int_abs_with_overflow(env, arg, builtin)
+}
+
+fn int_abs_with_overflow<'a, 'ctx, 'env>(
+    env: &Env<'a, 'ctx, 'env>,
+    arg: IntValue<'ctx>,
+    arg_layout: &Builtin<'a>,
+) -> BasicValueEnum<'ctx> {
+    // This is how libc's abs() is implemented - it uses no branching!
+    //
+    //     abs = \arg ->
+    //         shifted = arg >>> 63
+    //
+    //         (xor arg shifted) - shifted
+
+    let bd = env.builder;
+    let ctx = env.context;
+    let shifted_name = "abs_shift_right";
+    let shifted_alloca = {
+        let bits_to_shift = ((arg_layout.stack_size(env.ptr_bytes) as u64) * 8) - 1;
+        let shift_val = ctx.i64_type().const_int(bits_to_shift, false);
+        let shifted = bd.build_right_shift(arg, shift_val, true, shifted_name);
+        let alloca = bd.build_alloca(
+            basic_type_from_builtin(env.arena, ctx, arg_layout, env.ptr_bytes),
+            "#int_abs_help",
+        );
+
+        // shifted = arg >>> 63
+        bd.build_store(alloca, shifted);
+
+        alloca
+    };
+
+    let xored_arg = bd.build_xor(
+        arg,
+        bd.build_load(shifted_alloca, shifted_name).into_int_value(),
+        "xor_arg_shifted",
+    );
+
+    BasicValueEnum::IntValue(bd.build_int_sub(
+        xored_arg,
+        bd.build_load(shifted_alloca, shifted_name).into_int_value(),
+        "sub_xored_shifted",
+    ))
+}
+
 fn build_float_unary_op<'a, 'ctx, 'env>(
     env: &Env<'a, 'ctx, 'env>,
     arg: FloatValue<'ctx>,

compiler/gen/src/llvm/convert.rs

@@ -4,7 +4,7 @@ use inkwell::context::Context;
 use inkwell::types::BasicTypeEnum::{self, *};
 use inkwell::types::{ArrayType, BasicType, FunctionType, IntType, PointerType, StructType};
 use inkwell::AddressSpace;
-use roc_mono::layout::Layout;
+use roc_mono::layout::{Builtin, Layout};
 
 /// TODO could this be added to Inkwell itself as a method on BasicValueEnum?
 pub fn get_ptr_type<'ctx>(
@@ -103,8 +103,7 @@ pub fn basic_type_from_layout<'ctx>(
     layout: &Layout<'_>,
     ptr_bytes: u32,
 ) -> BasicTypeEnum<'ctx> {
-    use roc_mono::layout::Builtin::*;
+    use Layout::*;
-    use roc_mono::layout::Layout::*;
 
     match layout {
         FunctionPointer(args, ret_layout) => {
@@ -147,22 +146,33 @@ pub fn basic_type_from_layout<'ctx>(
                 .as_basic_type_enum()
         }
 
-        Builtin(builtin) => match builtin {
+        Builtin(builtin) => basic_type_from_builtin(arena, context, builtin, ptr_bytes),
-            Int128 => context.i128_type().as_basic_type_enum(),
+    }
-            Int64 => context.i64_type().as_basic_type_enum(),
+}
-            Int32 => context.i32_type().as_basic_type_enum(),
+
-            Int16 => context.i16_type().as_basic_type_enum(),
+pub fn basic_type_from_builtin<'ctx>(
-            Int8 => context.i8_type().as_basic_type_enum(),
+    _arena: &Bump,
-            Int1 => context.bool_type().as_basic_type_enum(),
+    context: &'ctx Context,
-            Float128 => context.f128_type().as_basic_type_enum(),
+    builtin: &Builtin<'_>,
-            Float64 => context.f64_type().as_basic_type_enum(),
+    ptr_bytes: u32,
-            Float32 => context.f32_type().as_basic_type_enum(),
+) -> BasicTypeEnum<'ctx> {
-            Float16 => context.f16_type().as_basic_type_enum(),
+    use Builtin::*;
-            Dict(_, _) | EmptyDict => panic!("TODO layout_to_basic_type for Builtin::Dict"),
+
-            Set(_) | EmptySet => panic!("TODO layout_to_basic_type for Builtin::Set"),
+    match builtin {
-            List(_, _) | Str | EmptyStr => collection(context, ptr_bytes).into(),
+        Int128 => context.i128_type().as_basic_type_enum(),
-            EmptyList => BasicTypeEnum::StructType(collection(context, ptr_bytes)),
+        Int64 => context.i64_type().as_basic_type_enum(),
-        },
+        Int32 => context.i32_type().as_basic_type_enum(),
+        Int16 => context.i16_type().as_basic_type_enum(),
+        Int8 => context.i8_type().as_basic_type_enum(),
+        Int1 => context.bool_type().as_basic_type_enum(),
+        Float128 => context.f128_type().as_basic_type_enum(),
+        Float64 => context.f64_type().as_basic_type_enum(),
+        Float32 => context.f32_type().as_basic_type_enum(),
+        Float16 => context.f16_type().as_basic_type_enum(),
+        Dict(_, _) | EmptyDict => panic!("TODO layout_to_basic_type for Builtin::Dict"),
+        Set(_) | EmptySet => panic!("TODO layout_to_basic_type for Builtin::Set"),
+        List(_, _) | Str | EmptyStr => collection(context, ptr_bytes).into(),
+        EmptyList => BasicTypeEnum::StructType(collection(context, ptr_bytes)),
     }
 }
 

compiler/gen/tests/gen_num.rs

@@ -40,6 +40,8 @@ mod gen_num {
     fn f64_abs() {
         assert_evals_to!("Num.abs -4.7", 4.7, f64);
         assert_evals_to!("Num.abs 5.8", 5.8, f64);
+        //assert_evals_to!("Num.abs Num.maxFloat", f64::MAX, f64);
+        //assert_evals_to!("Num.abs Num.minFloat", -f64::MIN, f64);
     }
 
     #[test]
@@ -52,6 +54,24 @@ mod gen_num {
         assert_evals_to!("Num.abs 1", 1, i64);
         assert_evals_to!("Num.abs 9_000_000_000_000", 9_000_000_000_000, i64);
         assert_evals_to!("Num.abs -9_000_000_000_000", 9_000_000_000_000, i64);
+        assert_evals_to!("Num.abs Num.maxInt", i64::MAX, i64);
+        assert_evals_to!("Num.abs (Num.minInt + 1)", -(i64::MIN + 1), i64);
+    }
+
+    #[test]
+    #[should_panic(
+        expected = r#"Roc failed with message: "integer absolute overflowed because its argument is the minimum value"#
+    )]
+    fn abs_min_int_overflow() {
+        assert_evals_to!(
+            indoc!(
+                r#"
+                Num.abs Num.minInt
+                "#
+            ),
+            0,
+            i64
+        );
     }
 
     #[test]
@@ -533,6 +553,24 @@ mod gen_num {
     #[test]
     fn int_negate() {
         assert_evals_to!("Num.neg 123", -123, i64);
+        assert_evals_to!("Num.neg Num.maxInt", -i64::MAX, i64);
+        assert_evals_to!("Num.neg (Num.minInt + 1)", i64::MAX, i64);
+    }
+
+    #[test]
+    #[should_panic(
+        expected = r#"Roc failed with message: "integer negation overflowed because its argument is the minimum value"#
+    )]
+    fn neg_min_int_overflow() {
+        assert_evals_to!(
+            indoc!(
+                r#"
+                Num.neg Num.minInt
+                "#
+            ),
+            0,
+            i64
+        );
     }
 
     #[test]

compiler/gen/tests/gen_primitives.rs

@@ -1732,4 +1732,20 @@ mod gen_primitives {
             |_| 0
         );
     }
+
+    #[test]
+    fn hof_conditional() {
+        // exposed issue with the if condition being just a symbol
+        assert_evals_to!(
+            indoc!(
+                r#"
+                    passTrue = \f -> f True
+
+                    passTrue (\trueVal -> if trueVal then False else True)
+                "#
+            ),
+            0,
+            u8
+        );
+    }
 }

compiler/mono/src/ir.rs

@@ -2584,13 +2584,23 @@ pub fn with_hole<'a>(
 
                     let mut field_symbols_temp = Vec::with_capacity_in(args.len(), env.arena);
 
-                    for (var, arg) in args.drain(..) {
+                    for (var, mut arg) in args.drain(..) {
                         // Layout will unpack this unwrapped tack if it only has one (non-zero-sized) field
-                        let layout = layout_cache
+                        let layout = match layout_cache.from_var(env.arena, var, env.subs) {
-                            .from_var(env.arena, var, env.subs)
+                            Ok(cached) => cached,
-                            .unwrap_or_else(|err| {
+                            Err(LayoutProblem::UnresolvedTypeVar(_)) => {
-                                panic!("TODO turn fn_var into a RuntimeError {:?}", err)
+                                // this argument has type `forall a. a`, which is isomorphic to
-                            });
+                                // the empty type (Void, Never, the empty tag union `[]`)
+                                use roc_can::expr::Expr;
+                                use roc_problem::can::RuntimeError;
+                                arg.value = Expr::RuntimeError(RuntimeError::VoidValue);
+                                Layout::Struct(&[])
+                            }
+                            Err(LayoutProblem::Erroneous) => {
+                                // something went very wrong
+                                panic!("TODO turn fn_var into a RuntimeError")
+                            }
+                        };
 
                         let alignment = layout.alignment_bytes(8);
 
@@ -3574,9 +3584,23 @@ pub fn with_hole<'a>(
             let arg_symbols = arg_symbols.into_bump_slice();
 
             // layout of the return type
-            let layout = layout_cache
+            let layout = match layout_cache.from_var(env.arena, ret_var, env.subs) {
-                .from_var(env.arena, ret_var, env.subs)
+                Ok(cached) => cached,
-                .unwrap_or_else(|err| todo!("TODO turn fn_var into a RuntimeError {:?}", err));
+                Err(LayoutProblem::UnresolvedTypeVar(_)) => {
+                    return Stmt::RuntimeError(env.arena.alloc(format!(
+                        "UnresolvedTypeVar {} line {}",
+                        file!(),
+                        line!()
+                    )));
+                }
+                Err(LayoutProblem::Erroneous) => {
+                    return Stmt::RuntimeError(env.arena.alloc(format!(
+                        "Erroneous {} line {}",
+                        file!(),
+                        line!()
+                    )));
+                }
+            };
 
             let result = Stmt::Let(assigned, Expr::RunLowLevel(op, arg_symbols), layout, hole);
 
@@ -3653,7 +3677,7 @@ pub fn from_can<'a>(
             let mut stmt = from_can(env, branch_var, final_else.value, procs, layout_cache);
 
             for (loc_cond, loc_then) in branches.into_iter().rev() {
-                let branching_symbol = env.unique_symbol();
+                let branching_symbol = possible_reuse_symbol(env, procs, &loc_cond.value);
                 let then = from_can(env, branch_var, loc_then.value, procs, layout_cache);
 
                 stmt = Stmt::Cond {
@@ -3666,15 +3690,14 @@ pub fn from_can<'a>(
                     ret_layout: ret_layout.clone(),
                 };
 
-                // add condition
+                stmt = assign_to_symbol(
-                stmt = with_hole(
                     env,
-                    loc_cond.value,
-                    cond_var,
                     procs,
                     layout_cache,
+                    cond_var,
+                    loc_cond,
                     branching_symbol,
-                    env.arena.alloc(stmt),
+                    stmt,
                 );
             }
 

compiler/problem/src/can.rs

@@ -147,6 +147,9 @@ pub enum RuntimeError {
     /// When the author specifies a type annotation but no implementation
     NoImplementation,
 
+    /// cases where the `[]` value (or equivalently, `forall a. a`) pops up
+    VoidValue,
+
     ExposedButNotDefined(Symbol),
 }
 

compiler/reporting/src/error/canonicalize.rs

@@ -338,6 +338,12 @@ fn pretty_runtime_error<'b>(
     runtime_error: RuntimeError,
 ) -> RocDocBuilder<'b> {
     match runtime_error {
+        RuntimeError::VoidValue => {
+            // is used to communicate to the compiler that
+            // a branch is unreachable; this should never reach a user
+            unreachable!("")
+        }
+
         RuntimeError::Shadowing {
             original_region,
             shadow,