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

Confusing error when checking trait impl against trait #890

Closed
enjhnsn2 opened this issue Nov 16, 2024 · 5 comments · Fixed by #891
Closed

Confusing error when checking trait impl against trait #890

enjhnsn2 opened this issue Nov 16, 2024 · 5 comments · Fixed by #891
Assignees

Comments

@enjhnsn2
Copy link
Collaborator

enjhnsn2 commented Nov 16, 2024

When I try to check the following trait implementation (here):

    #[flux_rs::sig(
        fn(self: &strg Self, _) -> Option<_> ensures self: Self
    )]
    fn remove_first_matching<F>(&mut self, f: F) -> Option<T>
    where
        F: Fn(&T) -> bool,
    {...}

against the trait spec (here):

    #[flux_rs::sig(
        fn(self: &strg Self, _) -> Option<_> ensures self: Self
    )]
    fn remove_first_matching<F>(&mut self, f: F) -> Option<T>
    where
        F: Fn(&T) -> bool;

it fails with the error:

error[E0999]: internal flux error: crates/flux-infer/src/infer.rs:533:22
   --> kernel/src/collections/ring_buffer.rs:181:5
    |
181 | /     fn remove_first_matching<F>(&mut self, f: F) -> Option<T>
182 | |     where
183 | |         F: Fn(&T) -> bool,
    | |__________________________^
    |
    = note: incompatible types: `&'_ strg <self: RingBuffer<'_, λb0. T[b0]>[RingBuffer { a0, a1, a2 }]>` - `&'_ strg <?0e#8242: ∃b0. RingBuffer<'_, λb1. T[b1]>[b0]>`

I'm a little confused because the spec is token-for-token identical in both the trait and implementation, and only asserts that we need a &strg Self.

I also get a similar error when there is no spec on the trait at all. I don't fully understand the implementation for trait_impl checking, so I'm not sure if this is expected or not.

Any guidance?

@nilehmann
Copy link
Member

nilehmann commented Nov 16, 2024

The check is not implemented at all when strong references are involved so it just crashes.

Do you need the &strg? isn't the mut ref unfolding we implemented enough for this use case? I would like to know why not.

In any case, @ranjitjhala maybe you can take care of finishing the work on trait impl checking to consider strong references.

@enjhnsn2 in the mean time you can use #[flux::trusted_impl]

@enjhnsn2
Copy link
Collaborator Author

As requested, another implementation

    #[flux_rs::sig(
        fn(self: &strg RingBuffer<T>[@old]) ensures self: RingBuffer<T>[old.ring_len, 0, 0]
    )]
    fn empty(&mut self) {
        self.head = 0;
        self.tail = 0;
    }

and trait method

    /// Remove all elements from the ring buffer.
    fn empty(&mut self);

where the implementation has a non-trivial ensures clause.

@nilehmann
Copy link
Member

@ranjitjhala your PR doesn't consider this case. We need the unfold_local_ptrs hack to handle this. I think it shouldn't be too hard to reuse it, right?

Also, I always thought that if you have an &mut in the trait definition you would never be able to make it an &strg in the impl, but now that we've implemented the check I see that in certain situations you can. In particular, if the trait definition is completely unrefined you can turn an &mut into an &strg as long as you don't put extra constraints in the input at the implementation. I'm quite happy about that.

@ranjitjhala
Copy link
Contributor

on it!

@ranjitjhala
Copy link
Contributor

I updated the PR to handle the above (I think!) can you lmk if it works now @enjhnsn2 (and of course reopen if not!)

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 a pull request may close this issue.

3 participants