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

Rudimentary support for dynamic trait objects #664

Merged
merged 37 commits into from
Jul 29, 2024
Merged

Rudimentary support for dynamic trait objects #664

merged 37 commits into from
Jul 29, 2024

Conversation

ranjitjhala
Copy link
Contributor

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
Copy link
Collaborator

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:

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
Copy link
Member

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

@enjhnsn2
Copy link
Collaborator

enjhnsn2 commented Jul 23, 2024

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:

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

@ranjitjhala
Copy link
Contributor Author

@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
Copy link
Contributor Author

@enjhnsn2
Copy link
Collaborator

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
Copy link
Collaborator

enjhnsn2 commented Jul 24, 2024

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
Copy link
Contributor Author

ranjitjhala commented Jul 24, 2024 via email

@nilehmann
Copy link
Member

nilehmann commented Jul 24, 2024

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.

Copy link
Member

@nilehmann nilehmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

crates/flux-middle/src/fhir.rs Show resolved Hide resolved
crates/flux-middle/src/fhir/lift.rs Show resolved Hide resolved
crates/flux-middle/src/fhir/lift.rs Outdated Show resolved Hide resolved
crates/flux-middle/src/rustc/ty.rs Outdated Show resolved Hide resolved
crates/flux-middle/src/rty/mod.rs Outdated Show resolved Hide resolved
@ranjitjhala
Copy link
Contributor Author

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

@ranjitjhala
Copy link
Contributor Author

ranjitjhala commented Jul 25, 2024

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...

crates/flux-fhir-analysis/src/conv/mod.rs Outdated Show resolved Hide resolved
crates/flux-fhir-analysis/src/conv/mod.rs Outdated Show resolved Hide resolved
crates/flux-middle/src/rustc/lowering.rs Outdated Show resolved Hide resolved
@@ -1099,6 1133,7 @@ impl BaseTy {
| BaseTy::Array(_, _)
| BaseTy::Closure(_, _)
| BaseTy::Coroutine(..)
| BaseTy::Dynamic(_, _)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

crates/flux-refineck/src/constraint_gen.rs Outdated Show resolved Hide resolved
@ranjitjhala
Copy link
Contributor Author

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...)

Copy link
Member

@nilehmann nilehmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ranjitjhala ranjitjhala merged commit aed06a7 into main Jul 29, 2024
5 checks passed
@ranjitjhala ranjitjhala deleted the dyn branch July 29, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supporting dynamic trait objects
3 participants