Skip to content

tls_codec: use fully qualified name in derive macro#1103

Closed
imor wants to merge 1 commit into
RustCrypto:masterfrom
imor:fix_tls_serialize_macro
Closed

tls_codec: use fully qualified name in derive macro#1103
imor wants to merge 1 commit into
RustCrypto:masterfrom
imor:fix_tls_serialize_macro

Conversation

@imor

@imor imor commented Jun 12, 2023

Copy link
Copy Markdown
Contributor

Using #[derive(TlsSerialize)] on a struct forced the user to include std::format.

@franziskuskiefer franziskuskiefer self-requested a review June 12, 2023 18:17

@franziskuskiefer franziskuskiefer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you have a minimal example where this makes trouble? Generally this shouldn't be necessary.

@imor

imor commented Jun 12, 2023

Copy link
Copy Markdown
Contributor Author

I was working in adding support for the Signed Certificate Timestamp extension in x509-cert in my branch and when I comment out this line the code doesn't compile. Specifically this #[derive(TlsSerialize)] fails to compile. If I apply the patch in this PR it works.

I also tried the same code in a fresh project where the #[derive(TlsSerialize)] does work without the explicit use statement. Not really sure what's going on.

@imor

imor commented Jun 12, 2023

Copy link
Copy Markdown
Contributor Author

Just checked the output of cargo expand and it expands the format to ::alloc::fmt::format:

                  Err(
                        tls_codec::Error::EncodingError({
                            let res = ::alloc::fmt::format(
                                format_args!(
                                    "Expected to serialize {0} bytes but only {1} were generated.",
                                    expected_written, written
                                ),
                            );
                            res
                        }),
                    )

Not sure why that shouldn't be sufficient to compile the code.

@imor

imor commented Jun 12, 2023

Copy link
Copy Markdown
Contributor Author

I think I got it. The crate x509-cert is marked #![no_std]. Which means there is no prelude and hence no format!. Looks like the tls_codec_derive crate always assumes that std will be available. It needs to be updated to support no_std as well.

@tarcieri

Copy link
Copy Markdown
Member

@imor FWIW all of the X.509-related crates we have in this repo have a hard dependency on liballoc, so using alloc::fmt::format should be fine

@imor

imor commented Jun 13, 2023

Copy link
Copy Markdown
Contributor Author

@tarcieri thanks that makes sense, I'll add a use alloc::format in my code.

The question about whether tls_codec_derive should refer to items in stdlib still remains. IMHO it should not unconditionally depend upon stdlib because it restricts the derive macros to be usable only in stdlib environments. If you guys agree I can open another issue to make tls_codec_derive no_std compatible.

I will also be closing this PR since no_std compatibility is more work than what is done in this PR.

@franziskuskiefer

Copy link
Copy Markdown
Contributor

Yes, the derive crate currently requires std.
Making it no_std compatible would be nice but indeed a little more work.

@imor

imor commented Jun 13, 2023

Copy link
Copy Markdown
Contributor Author

Closing this PR. A new issue for no_std support is added here

@imor imor closed this Jun 13, 2023
@imor imor deleted the fix_tls_serialize_macro branch June 13, 2023 11:53
@imor imor restored the fix_tls_serialize_macro branch July 2, 2023 07:53
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