Rudimentary support for dynamic trait objects

[ranjitjhala]

Fixes #655

Totally ignores all the bound-vars stuff for TraitObject because I haven't understood it at all, but wanted to start iterating.

[enjhnsn2]

This pr already helps a ton, although there are still some dyn trait related crashes when I try to apply it to Tock. So far, I've seen one main crash that I've minimized it down to:

struct Cow {}

trait CowContainer<'a> {
    fn get_cow(&self) -> &'a Cow;
}

pub struct CowCell<'a> {
    inner: &'a dyn CowContainer<'a>,
}

When you run flux on this code, it errors with:
error: internal compiler error: crates/flux-fhir-analysis/src/conv/mod.rs:444:13: unexpected! conv_poly_dyn_trait_ref

Which points to this code:

flux/crates/flux-fhir-analysis/src/conv/mod.rs

Lines 438 to 450 in 23c7453

	fn conv_poly_trait_ref_dyn(
	poly_trait_ref: &fhir::PolyTraitRef,
	) -> QueryResult<rty::PolyTraitRef> {
	let trait_segment = poly_trait_ref.trait_ref.last_segment();
	if !poly_trait_ref.bound_generic_params.is_empty() || !trait_segment.args.is_empty() {
	bug!("unexpected! conv_poly_dyn_trait_ref");
	}
	let trait_ref =
	rty::TraitRef { def_id: poly_trait_ref.trait_def_id(), args: List::empty() };
	Ok(rty::PolyTraitRef { trait_ref, bound_generic_params: List::empty() })
	}

Comparing this code to other code that passes, it appears that that the problem is that the the trait being parameterized by the dyn trait has a lifetime parameter. Let me know if you need any other info

[nilehmann]

I'll probably have some time to look at this tomorrow

[enjhnsn2]

Found one more issue: dyn traits are not implemented as possible return values.
If you attempt to verify the code

trait Animal {
    fn noise(&self);
}

#[flux::trusted]
fn get_animal() -> &dyn Animal {
    unimplemented!()
}

It will error with:
thread 'rustc' panicked at crates/flux-middle/src/rty/mod.rs:1160:45: not yet implemented
which points to the code:

flux/crates/flux-middle/src/rty/mod.rs

Line 1160 in 23c7453

BaseTy::TraitObject(_, _, _) => todo!(),

[ranjitjhala]

@enjhnsn2 are you sure about the second one, it seems to work fine for me:

rjhala@pozole ~/r/flux (dyn)> more tests/tests/todo/dyn02.rs 
pub trait Animal {
    fn noise(&self);
}

#[flux::trusted]
pub fn get_animal() -> &'static dyn Animal {
    unimplemented!()
}
rjhala@pozole ~/r/flux (dyn)> cargo xtask run tests/tests/todo/dyn02.rs
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.02s
     Running `target/debug/xtask run tests/tests/todo/dyn02.rs`
$ export FLUX_BUILD_SYSROOT=1
$ cargo build -p flux-rs
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.02s
$ cargo build
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.06s

[ranjitjhala]

@enjhnsn2 - the above two things work now, see

• https://github.com/flux-rs/flux/blob/dyn/tests/tests/pos/surface/dyn02.rs
• https://github.com/flux-rs/flux/blob/dyn/tests/tests/pos/surface/dyn03.rs

[enjhnsn2]

pub trait Animal {
fn noise(&self);
}

#[flux::trusted]
pub fn get_animal() -> &'static dyn Animal {
unimplemented!()
}

Whoops, I think I may have messed up the minimization for that one. When I was minimizing I started with tock code that pulled in the tock kernel and I think the error was in the dependency, not the minimized code. Let me try to get a better example

[enjhnsn2]

Alright, I found a minimized example that triggers the error. Unfortunately its a little more complicated:

pub trait Animal {
    fn noise(&self);
}

fn apply_closure_to_animal<F>(closure: F, animal: &'static dyn Animal)
where
    F: FnOnce(&dyn Animal),
{
    closure(animal);
}

This bug isn't too frequent and is possible to work around, so I don't think you should block this pr on it.

[ranjitjhala]

Thanks, on it! - Ranjit.

…

On Wed, Jul 24, 2024 at 10:33 AM enjhnsn2 ***@***.***> wrote: Alright, I found a minimized example that triggers the error. Unfortunately its a little more complicated: pub trait Animal { fn noise(&self);} fn apply_closure_to_animal<F>(closure: F, animal: &'static dyn Animal)where F: FnOnce(&dyn Animal),{ closure(animal);} — Reply to this email directly, view it on GitHub <https://urldefense.com/v3/__https://github.com/flux-rs/flux/pull/664*issuecomment-2248555787__;Iw!!Mih3wA!HMp_f3DiAmkkZyGYTraImuODudwhdIKDCrIJrm2GsTIaWArQDMIXCnEPBLprCdOC47H0ZBEGXZ2Wk2KUKDXHXJnO$>, or unsubscribe <https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AAMS4OCINCS2FSTVUEW3ZTTZN7QPNAVCNFSM6AAAAABLK6BQ4GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBYGU2TKNZYG4__;!!Mih3wA!HMp_f3DiAmkkZyGYTraImuODudwhdIKDCrIJrm2GsTIaWArQDMIXCnEPBLprCdOC47H0ZBEGXZ2Wk2KUKF0HazE_$> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[nilehmann]

Alright, I found a minimized example that triggers the error. Unfortunately its a little more complicated:

pub trait Animal {
    fn noise(&self);
}

fn apply_closure_to_animal<F>(closure: F, animal: &'static dyn Animal)
where
    F: FnOnce(&dyn Animal),
{
    closure(animal);
}

This bug isn't too frequent and is possible to work around, so I don't think you should block this pr on it.

This feels it'll fail for various reasons not just dynamic objects. I'd prefer if we don't fix it in this PR.

[nilehmann]

@ranjitjhala first round of comments. I focused only on the way dynamic objects are being represented.

[ranjitjhala]

Ok I think I've fixed the TraitObject/Dynamic mismatch ...

[ranjitjhala]

This bug isn't too frequent and is possible to work around, so I don't think you should block this pr on it.

This feels it'll fail for various reasons not just dynamic objects. I'd prefer if we don't fix it in this PR.

@nilehmann -- it turns out that for @enjhnsn2 example with closure and dyn above (see issue #666) -- just filling out the todo! case for Dynamic in the to_rustc(...) fixes #666 i.e. gets this to not crash, so I just tacked it onto this PR since its very much related to the whole conversion to/from ExistentialPredicate/ExistentialTraitRef that is the bulk of this PR...

[nilehmann]

There's something I don't quite understand yet. We are putting Dynamic in BaseTy which means they can be indexed (by unit as returned by this function), however, the concrete type under the dynamic type could be indexed by another sort. I don't know if these should be somehow related or if they are completely orthogonal.

[ranjitjhala]

I think its fine, because once its Dynamic you have zero information about the underlying concrete type, everything is abstracted to whatever is know about the dyn...

[ranjitjhala]

Ok I think I addressed all the issues above -- except the

We are putting Dynamic in BaseTy which means they can be indexed (by unit as returned by this function),

But I'm pretty certain that's (for now at least) the sensible choice - as in dyn you have to forget all about the underlying concrete type; I suggest we revisit this later if we have reason to (or if we think there's some use-case for indexing Dynamic values...)

[nilehmann]

Ok, I think this is good to go. we should be able to iterate from here once we have concrete examples.

re indices: more than worried about being unsound or anything I was wondering whether it will ever make sense to have indices on dynamic objects. We will see, but for now unit sounds like a safe option.