Manual block encodings

[PaulDance]

Hey @madsmtm!

This is an attempt to contribute to #442, by adding the ability to manually provide a block encoding to its constructor so that it sets BLOCK_HAS_SIGNATURE and saves the string properly.

Included:

• New StackBlock::with_encoding.
• New RcBlock::with_encoding.
• block2::traits::ManualBlockEncoding is the smuggling trait.
• String check on cfg(debug_assertions): a bit too strict for now, but can be relaxed later on by implementing a dedicated parser and equivalence checking.
• Updated and new tests.

Cheers,
Paul.

[madsmtm]

dangling pointers

Right, &Self::DESCRIPTOR_WITH_CLONE works because it is promoted to a static, but that can't happen if we construct the descriptor on the stack. I've documented this somewhat in 893d044.

I think my preferred solution (for now) would be something like the following instead (similar to your proposal in #442 (comment), just with an extra trait):

unsafe trait BlockEncodingHelper { // Bikeshed name
    type Arguments: EncodingArguments;
    type Return: EncodingReturn;
    const ENCODING_CSTR: &'static CStr;
}

impl Block {
    pub fn with_encoding<'f, A, R, Closure, Helper>(
        _helper: Helper,
        closure: Closure,
    ) -> Self
    where
        Helper: BlockEncodingHelper<Arguments = A, Return = R>,
        // ...
    { ... }
}

// Usage
struct Helper;

impl BlockEncodingHelper for Helper {
    type Arguments = (i32, Bool);
    type Return = u8;
    const ENCODING_CSTR: &'static CStr = c"whatever_the_encoding_is_I_have_not_checked_it";
}

let block = RcBlock::with_encoding(Helper, |arg1, arg2| {
    if arg2.as_bool() {
        arg1 as u8
    } else {
        0
    }
});

[PaulDance]

Right, &Self::DESCRIPTOR_WITH_CLONE works because it is promoted to a static

Ah yes, I was suspecting something of the sort was happening behind the scenes, but did not know the exact underlying mechanism. Thanks for the link.

I think my preferred solution (for now) would be something like the following instead

That seems like it could work, yes thanks. I'll try it soon and see if the promotion works in this case too.

[madsmtm]

I haven't reviewed all of it yet (use GitHub's UI to request a review when you want me to do so), only saw that you're bumping MSRV, please don't do that.

I think all the examples and tests can use c"string" without affecting MSRV, but in the places where the actual code is, you'll have to use unsafe { CStr::from_bytes_with_nul_unchecked(b"string\0") } instead.

[PaulDance]

I haven't reviewed all of it yet (use GitHub's UI to request a review when you want me to do so)

Yes, sorry for the notifications, I just wanted to make things work and then check that I haven't missed anything, such as adding new tests, hence why I left is as a draft.

saw that you're bumping MSRV, please don't do that

That was also part of the things I wanted to do before formally asking for a review, sorry. 😅

CStr was stabilized in 1.64 and the current MSRV is 1.60, so in any case it has to be bumped, right? Regarding why 1.79 specifically, which I understand constitutes a bit of an aggressive jump, it is for inline consts that I used to guarantee the const fn calls I made were indeed promoted, especially considering things like rust-lang/rust#121557. If you see any other way to do this without loosing these guarantees, then yes, I'll lower it as much as possible.

[madsmtm]

CStr was stabilized in 1.64

Preeetty sure that was only CStr in core, the docs are just bad at visualizing this. CStr has been available since basically forever.

guarantee the const fn calls I made were indeed promoted

Perhaps we can just avoid using the const fns here? From my reading of the code, you seem to be using it mostly to have the definition of e.g. BlockDescriptorCopyDisposeSignature next to BlockDescriptorCopyDispose (which is commendable, but also perhaps a bit unnecessary here).

If not, then there are ways to do the equivalent of inline const with traits and associated constants.

[PaulDance]

Preeetty sure that was only rust-lang/rust#98314, the docs are just bad at visualizing this. CStr has been available since basically forever.

Woops, sorry. That seems right, yes. Looking at the methods I use, I should be able to avoid any bump here, hopefully.

Perhaps we can just avoid using the const fns here?

Probably, yes. I just wanted to avoid the "let's hope promotion works similarly in previous and future compiler versions" kind of stance, although it should be acceptable 🤞

If not, then there are ways to do the equivalent of inline const with traits and associated constants.

I'll try the above first and then this if it fails somehow. Thanks for the suggestion.

[PaulDance]

In the end, the MSRV bump was indeed not necessary:

• the C string literals and too recent CStr methods were able to be circumvented;
• static promotion was successfully made functional without const fns and inline const expressions and with the suggested trait smuggling, as simply inlining the value made it fail to promote in practice;

Last thing I need to do before declaring this ready to review: try to add the cfg(debug_assertions)-gated encoding string safety checks.

[PaulDance]

Alright @madsmtm, this should now be ready for a proper first review cycle! Sorry for the many CI runs, though, I just struggled endlessly with minor details.

[madsmtm]

Sorry for the delay, I've been away on holidays. I like the overall approach of this, and I really appreciate the thorough documentation you've put into it!

Sorry for the many CI runs, though, I just struggled endlessly with minor details.

Don't worry, I don't pay for any of it - and it's mostly my own fault, I haven't really provided an easy way to test everything locally!

[madsmtm]

Raising this as a comment on the PR for visibility, I'm not sure what to do about alignment myself.

[madsmtm]

Hmm, I think perhaps a way to get this PR merged quicker would be to rip out the debug_assertions check for now (it's not robust enough for me to want to prevent users from using the API without it, and there's a bunch of issues with MSRV and such).

[PaulDance]

@madsmtm All the items should be addressed now. The only remaining issue is the MSRV bump requirement.

[madsmtm]

Thanks! I'll get back to this, and will fix any nits I have myself, after I've done the MSRV bump.

In the odd case that this isn't merged in a week, ping me again ;)

[PaulDance]

Thanks for the MSRV bump. It seems to work without my having to change anything, except for the tests where I use the c"" C string literal syntax. If I remember correctly, it previously worked. Was an --all-targets or --all-features flag of sorts added to the cargo check used in the MSRV CI step? If so, was it intentional, i.e. should I update my code?

[madsmtm]

Thanks for the MSRV bump. It seems to work without my having to change anything, except for the tests where I use the c"" C string literal syntax. If I remember correctly, it previously worked. Was an --all-targets or --all-features flag of sorts added to the cargo check used in the MSRV CI step? If so, was it intentional, i.e. should I update my code?

I think it's because even though it's guarded behind #[cfg(test)], Rust still needs to know that it's valid syntax.

You could put it in a separate file and import it with #[cfg(test)] mod tests; instead if really necessary, though I'd prefer to just not use c"...", and use "..." + a later cstr.to_str().unwrap() where possible, and from_bytes_with_nul(b"...\0").unwrap() where not.

[PaulDance]

Done 🙂

[madsmtm]

I went through the whole thing now, thanks for the nice commit sequence! I ended up making with_encoding safe, since we can more the safety requirement to the ManualBlockEncoding trait.

And thanks for the excellent documentation and list of examples in ManualBlockEncoding, could not have written that better!

[PaulDance]

Cool, thanks for the great collaboration!