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

Support for mutually recursive traits and functions #159

Open
msprotz opened this issue Apr 30, 2024 · 5 comments
Open

Support for mutually recursive traits and functions #159

msprotz opened this issue Apr 30, 2024 · 5 comments
Assignees
Labels
C-bug A bug in charon C-simplification-pass Request for charon to process the output llbc

Comments

@msprotz
Copy link
Contributor

msprotz commented Apr 30, 2024

I am getting this error:

   Compiling libcrux-ml-kem v0.0.2-pre.2 (/Users/jonathan/Code/libcrux/libcrux-ml-kem)
warning: Could not find clauses for trait obligations:core::ops::function::FnMut<Fold, (Acc, core::iter::adapters::enumerate::{impl core::iter::traits::iterator::Iterator for @Adt15<I>#1}<I>[@TraitClause0]::Item)>

         Available clauses:
         [@TraitClause0]: core::iter::traits::iterator::Iterator<I>
         [@TraitClause1]: core::ops::function::FnMut<Fold, (Acc, (usize, @TraitClause0::Item))>
         [@TraitClause2]: @TraitDecl21<R>
         [Self]: core::iter::traits::iterator::Iterator<@Adt15<I>>
         - context: core::iter::adapters::enumerate::{impl#1}::try_fold
 --> /rustc/d59363ad0b6391b7fc5bbb02c9ccf9300eef3753/library/core/src/iter/adapters/enumerate.rs:73:5

warning: Could not find clauses for trait obligations:core::ops::function::FnMut<Fold, (Acc, core::iter::adapters::enumerate::{impl core::iter::traits::iterator::Iterator for @Adt15<I>#1}<I>[@TraitClause0]::Item)>

         Available clauses:
         [@TraitClause0]: core::iter::traits::iterator::Iterator<I>
         [@TraitClause1]: core::ops::function::FnMut<Fold, (Acc, (usize, @TraitClause0::Item))>
         [Self]: core::iter::traits::iterator::Iterator<@Adt15<I>>
         - context: core::iter::adapters::enumerate::{impl#1}::fold
 --> /rustc/d59363ad0b6391b7fc5bbb02c9ccf9300eef3753/library/core/src/iter/adapters/enumerate.rs:96:5

thread 'rustc' panicked at 'Invalid scc:
libcrux_ml_kem::simd::portable::{impl libcrux_ml_kem::simd::simd_trait::Operations for libcrux_ml_kem::simd::portable::PortableVector}
libcrux_ml_kem::simd::portable::ntt_multiply
libcrux_ml_kem::simd::portable::deserialize_1
libcrux_ml_kem::simd::portable::deserialize_4
libcrux_ml_kem::simd::portable::deserialize_5
libcrux_ml_kem::simd::portable::deserialize_10
libcrux_ml_kem::simd::portable::deserialize_11
libcrux_ml_kem::simd::portable::deserialize_12
libcrux_ml_kem::simd::portable::{impl libcrux_ml_kem::simd::simd_trait::Operations for libcrux_ml_kem::simd::portable::PortableVector}::ntt_multiply
libcrux_ml_kem::simd::portable::{impl libcrux_ml_kem::simd::simd_trait::Operations for libcrux_ml_kem::simd::portable::PortableVector}::deserialize_1
libcrux_ml_kem::simd::portable::{impl libcrux_ml_kem::simd::simd_trait::Operations for libcrux_ml_kem::simd::portable::PortableVector}::deserialize_4
libcrux_ml_kem::simd::portable::{impl libcrux_ml_kem::simd::simd_trait::Operations for libcrux_ml_kem::simd::portable::PortableVector}::deserialize_5
libcrux_ml_kem::simd::portable::{impl libcrux_ml_kem::simd::simd_trait::Operations for libcrux_ml_kem::simd::portable::PortableVector}::deserialize_10
libcrux_ml_kem::simd::portable::{impl libcrux_ml_kem::simd::simd_trait::Operations for libcrux_ml_kem::simd::portable::PortableVector}::deserialize_11
libcrux_ml_kem::simd::portable::{impl libcrux_ml_kem::simd::simd_trait::Operations for libcrux_ml_kem::simd::portable::PortableVector}::deserialize_12', src/reorder_decls.rs:642:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: `libcrux-ml-kem` (lib) generated 2 warnings
error: could not compile `libcrux-ml-kem` (lib); 2 warnings emitted

steps to reproduce: clone libcrux with branch and commit information below, then run charon in the libcrux-ml-kem subdirectory

jonathan@benedictine:~/Code/libcrux/libcrux-ml-kem (franziskus/simd-ml-kem-architecture) $ git rev-parse HEAD
e3ab051c847df34f245fba39117c50ac6ffaebcf
jonathan@benedictine:~/Code/libcrux/libcrux-ml-kem (franziskus/simd-ml-kem-architecture) $ charon

for now, I would love to just get support in charon so that I can handle this in Eurydice and produce C code -- Aeneas support can come later

@R1kM
Copy link
Member

R1kM commented May 1, 2024

After debugging a bit, I am not sure this is due to recursive functions or traits. Below is a minimized example leading to the same error. The problematic part is the Portable::ZERO() in the implementation of ntt_multiply, removing the Portable prefix makes the error go away.

trait Ops {
    fn ZERO() -> Self;
    fn ntt_multiply(&Self) -> Self;
}

struct Portable {}

fn ZERO() -> Portable {
    Portable {}
}

fn ntt_multiply(x: &Portable) -> Portable {
    let mut out = Portable::ZERO();
    out
}

impl Ops for Portable {
    fn ZERO() -> Self {
        ZERO()
    }

    fn ntt_multiply(x: &Self) -> Self {
        ntt_multiply(x)
    }
}

@Nadrieril Nadrieril added the C-bug A bug in charon label May 3, 2024
@Nadrieril
Copy link
Member

Nadrieril commented May 6, 2024

After discussing with @R1kM I understand the issue better: this is a limitation put in place because verification backends typically don't support mutually recursive groups that mix e.g. trait impls and functions. At the very least we should have a clear error message.

At least for this case charon could do better: given how charon encodes trait impls, inside ntt_multiply, we don't actually need to refer to a trait impl to get the function Portable::ZERO; we could name it directly as test_crate::{impl test_crate::Ops for test_crate::Portable}::ZERO and avoid the issue.

@sonmarcho
Copy link
Member

I think a first thing to do is to allow heterogeneous groups of mutually recursive definitions in Charon, and fail in Aeneas if such a group is not supported. I have several things in mind to add proper support in Aeneas: it depends on the cases we encounter. In particular, if we only encounter mutually recursive groups of functions and implementations I think it's ok.

@sonmarcho
Copy link
Member

After discussion: we will add a pass to inline calls to methods when the implementation is known (i.e., it is not a trait clause). It should take care of most situations where we have a group of mutually recursive functions and implementations. If we encounter (real-world) cases which are not eliminated through this micro-pass, we will implement a more general solution.

@Nadrieril Nadrieril added the C-simplification-pass Request for charon to process the output llbc label May 14, 2024
@Nadrieril Nadrieril self-assigned this May 14, 2024
@Nadrieril
Copy link
Member

Nadrieril commented Jun 27, 2024

Btw this should no longer be a problem in Eurydice since #265 (and AeneasVerif/eurydice#27). The proposed fix is still desirable for Aeneas but shouldn't be a blocker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug A bug in charon C-simplification-pass Request for charon to process the output llbc
Projects
None yet
Development

No branches or pull requests

4 participants