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

Allow calling interrupt::free with zero-arity closures. (0.7.x) #436

Closed

Conversation

reitermarkus
Copy link
Member

0.8 will only accept zero-arity closures, this will make the transition a bit easier by being able to update calls before it is released.

@reitermarkus reitermarkus requested a review from a team as a code owner May 4, 2022 14:58
@reitermarkus reitermarkus force-pushed the interrupt-free-arity-0 branch 6 times, most recently from 6ba7d2d to 46916ed Compare May 4, 2022 18:13
@reitermarkus reitermarkus force-pushed the interrupt-free-arity-0 branch 4 times, most recently from 145feb5 to bd2d89c Compare May 4, 2022 18:38
src/interrupt.rs Outdated Show resolved Hide resolved
Co-authored-by: Alex Martens <alex@thinglab.org>
@Dirbaio
Copy link
Member

Dirbaio commented May 4, 2022

This is unsound, it allows any value for the 'cs lifetime. This code should be rejected but it's not.

    interrupt::free(|cs| {
        let cs: &'static CriticalSection = cs;
    });

@Dirbaio
Copy link
Member

Dirbaio commented May 4, 2022

The sound way of writiing it would be this:

impl<F, R> InterruptFreeFn<CriticalSection, R> for F
where
    F: for<'cs> FnOnce(&'cs CriticalSection) -> R,
{
    #[inline]
    unsafe fn call(self) -> R {
        self(&CriticalSection::new())
    }
}

However this fails to correctly infer the closure type (playground):

interrupt::free(|cs| {})

Interestingly, these work.

interrupt::free(|cs: &CriticalSection| {})
interrupt::free(|cs: &_| {})

Closure type inference through "wrapper" traits like these is known to be flaky, see this issue for example rust-lang/rust#70263 . I'm not sure if what you're trying to do is possible in today's stable Rust.

@reitermarkus
Copy link
Member Author

This is unsound

Thanks, good catch.

I'm not sure if what you're trying to do is possible in today's stable Rust.

Yeah, seems like it.

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.

3 participants