Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: deriving Destruct leads to 'Internal error, cycle detected:` panic #3943

Closed
enitrat opened this issue Aug 20, 2023 · 3 comments · Fixed by #5335
Closed

bug: deriving Destruct leads to 'Internal error, cycle detected:` panic #3943

enitrat opened this issue Aug 20, 2023 · 3 comments · Fixed by #5335
Labels
bug Something isn't working

Comments

@enitrat
Copy link
Contributor

enitrat commented Aug 20, 2023

Bug Report

Cairo version:
1f83b95

Current behavior:
Compiling the code below leads to the following error:

thread 'main' panicked at 'Internal error, cycle detected:

DatabaseKeyIndex { group_index: 5, query_index: 13, key_index: 0 }
DatabaseKeyIndex { group_index: 5, query_index: 12, key_index: 1 }
DatabaseKeyIndex { group_index: 5, query_index: 11, key_index: 1 }
', /Users/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/salsa-0.16.1/src/lib.rs:490:48

Expected behavior:
The code compiles successfully

Steps to reproduce:
Compile the following code

#[derive(Destruct)]
struct MinimalStruct {}

fn execute() {
    let mut ctx = MinimalStruct {};
}

Other information:
The bug seems to be related to the Destruct derivation. Removing it and replacing it by Drop works fine. However, this is just a minimal reproducible example I've been able to build. In my real use case, I have struct containing a Dict, and thus I cannot derive Drop.

@enitrat enitrat added the bug Something isn't working label Aug 20, 2023
@enitrat
Copy link
Contributor Author

enitrat commented Aug 20, 2023

It also looks like there might be a problem when we create a type T that implements destruct and put it in a box (which is what I'm trying to do originally in my codebase)
e.g

the following code will not compile because Destruct<Box> is not implemented

#[derive(Destruct)]
struct InnerStruct {// dict: Felt252Dict<u128>
}

#[derive(Destruct)]
struct OuterStruct {
    dict: Felt252Dict<u128>,
    inner: Box<InnerStruct>
}

fn execute() {
    let mut ctx = OuterStruct { dict: Default::default(), inner: BoxTrait::new(InnerStruct {}) };
}

so, I add an implementation of destruct for it following @orizi's advice in #3854

struct InnerStruct {}

impl DestructBoxInnerStruct of Destruct<Box<InnerStruct>> {
    fn destruct(self: Box<InnerStruct>) nopanic {}
}

#[derive(Destruct)]
struct OuterStruct {
    dict: Felt252Dict<u128>,
    inner: Box<InnerStruct>
}

fn execute() {
    let mut ctx = OuterStruct { dict: Default::default(), inner: BoxTrait::new(InnerStruct {}) };
}

And same error:

thread 'main' panicked at 'Internal error, cycle detected:

DatabaseKeyIndex { group_index: 5, query_index: 16, key_index: 1 }
DatabaseKeyIndex { group_index: 5, query_index: 15, key_index: 2 }
DatabaseKeyIndex { group_index: 5, query_index: 14, key_index: 2 }
', /Users/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/salsa-0.16.1/src/lib.rs:490:48
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@orizi
Copy link
Collaborator

orizi commented Aug 21, 2023

do note that the destruct implementation is wrong - but the compiler panic is indeed bad:

impl DestructBoxInnerStruct of Destruct<Box<InnerStruct>> {
    fn destruct(self: Box<InnerStruct>) nopanic {
        self.unbox().destruct()
    }
}

@enitrat
Copy link
Contributor Author

enitrat commented Aug 21, 2023

Thanks for the correction! The compiler still panics if the struct is empty. But with your right destruct implementation, it fixed my original problem 😉

use box::BoxTrait;
use traits::Destruct;

#[derive(Destruct)]
struct InnerStruct {}

impl DestructBoxInnerStruct of Destruct<Box<InnerStruct>> {
    fn destruct(self: Box<InnerStruct>) nopanic {
        self.unbox().destruct()
    }
}

#[derive(Destruct)]
struct OuterStruct {
    dict: Felt252Dict<u128>,
    inner: Box<InnerStruct>
}

fn execute() {
    let mut ctx = OuterStruct { dict: Default::default(), inner: BoxTrait::new(InnerStruct {}) };
}

@orizi orizi linked a pull request Mar 27, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants