cairo_native icon indicating copy to clipboard operation
cairo_native copied to clipboard

Implement CoreTypeConcrete::RangeCheck return type

Open pefontana opened this issue 1 year ago • 1 comments

Implement the parse result for CoreTypeConcrete::RangeCheck https://github.com/lambdaclass/cairo_native/blob/ddfd28217c10f7202a03f10234f0a19113c46904/src/executor.rs#L795

We should be able to run the primitive_types2.cairo program https://github.com/lambdaclass/cairo_native/blob/e89d38ad2f01bbd85c9f9de0b37841ac98661b50/tests/tests/cases.rs#L133

pefontana avatar May 15 '24 22:05 pefontana

hey @pefontana are you working on this or can I give it a try?

greged93 avatar May 22 '24 11:05 greged93

Hi @greged93 You can tackle it!

pefontana avatar May 23 '24 23:05 pefontana

Hello, I think there's been a misunderstanding in the issue.

The RangeCheck is a builtin. All builtins are parsed before parse_result is invoked, therefore making parse_result parse RangeCheck makes no sense. I'm thinking that this issue was caused when a program that returns nothing but uses RangeCheck is executed since the compiler would trigger the panic in parse_result for that specific builtin.

The correct fix would be to check whether the last return value is a builtin. If it's not a builtin then invoke parse_result. Here's the code that should be modified instead:

    // Parse return values.
    let return_value = function_signature
        .ret_types
        .last()
        .map(|ret_type| {
            parse_result(
                context,
                ret_type,
                registry,
                module,
                metadata,
                return_ptr,
                ret_registers,
                // TODO: Consider returning an Option<JitValue> as return_value instead
                // As cairo functions can not have a return value
            )
        })
        .unwrap_or_else(|| JitValue::Struct {
            fields: vec![],
            debug_name: None,
        });

Since you already have one, would you mind fixing that in your PR @greged93 ? The changes in src/executor.rs and tests/common.rs should be reverted, of course.

azteca1998 avatar May 29 '24 16:05 azteca1998

Makes sense, I was actually wondering why we needed to convert the builtin since builtins were already handled as you said. Will quickly update tmr!

greged93 avatar May 29 '24 16:05 greged93

Fixed by #626

igaray avatar Jun 04 '24 19:06 igaray