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

Impl UUID macro #543

Merged
merged 7 commits into from Nov 1, 2021
Merged

Impl UUID macro #543

merged 7 commits into from Nov 1, 2021

Conversation

QnnOkabayashi
Copy link
Contributor

@QnnOkabayashi QnnOkabayashi commented Oct 31, 2021

I'm submitting a feature

Description

Implementing the uuid! macro for parsing string literals at compile time.

Motivation

Very convenient for config files

Tests

I duplicated the valid str parsing tests for uuid!(), and they all compiled. For testing for failure, I checked that invalid inputs don't work:

  1. non literal: uuid!(struct) fails properly.
  2. non string literal: uuid!(44) fails properly.
  3. invalid string: uuid!("hello world") fails properly

I didn't bother to test more extensively then that, do we care?

Also, can someone verify that tokenizing to ::uuid::Uuid::from_bytes([#tokens]) is hygienic?

Related Issue(s)

#422

#[cfg(any(feature = "std", test))]
#[macro_use]
extern crate std;

#[cfg(all(not(feature = "std"), not(test)))]
#[macro_use]
extern crate core as std;
Copy link
Contributor Author

@QnnOkabayashi QnnOkabayashi Oct 31, 2021

Choose a reason for hiding this comment

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

Required to stop shared/error.rs and shared/parser.rs from complaining. I just copy-pasted it from uuid.

@KodrAus
Copy link
Member

KodrAus commented Oct 31, 2021

Thanks for working on this @QnnOkabayashi!

I think we might need to wrap this up in a macro_rules macro in uuid that can pass the fully-qualified path to the Uuid type using the $crate metavariable. We could then move the docs to this public macro and make the proc-macro a private implementation detail.

It would also be great to test this using trybuild so we can look at diagnostics for invalid inputs, and test that renaming uuid or defining a root module called uuid doesn’t cause the macro to break.

Is this something you’d be interested in exploring further? If not I’m happy to merge and fill those bits in.

Thanks again! 🙇

@QnnOkabayashi
Copy link
Contributor Author

QnnOkabayashi commented Nov 1, 2021

I think we might need to wrap this up in a macro_rules macro in uuid that can pass the fully-qualified path to the Uuid type using the $crate metavariable. We could then move the docs to this public macro and make the proc-macro a private implementation detail.

Done 👍

It would also be great to test this using trybuild so we can look at diagnostics for invalid inputs, and test that renaming uuid or defining a root module called uuid doesn’t cause the macro to break.

Done 👍

Also since the macro is integrated with the crate now, I was able to use the internal diagnostic information in the ErrorKind type to improve the diagnostics. For example, if there's an invalid character, it only points to that character

error: invalid character: expected an optional prefix of `urn:uuid:` followed by 0123456789abcdefABCDEF-, found G at 20
  --> benches/macros/invalid_parse.rs:25:44
   |
25 | const _: Uuid = uuid!("F9168C5E-CEB2-4faa-BGBF-329BF39FA1E4");
   |                                            ^

If there's an invalid group length, it points to that group

error: invalid group length: expected 12, found 8 in group 4
  --> benches/macros/invalid_parse.rs:26:48
   |
26 | const _: Uuid = uuid!("01020304-1112-2122-3132-41424344");
   |                                                ^^^^^^^^

KodrAus
KodrAus approved these changes Nov 1, 2021
Copy link
Member

@KodrAus KodrAus left a comment

This looks great, thanks @QnnOkabayashi!

I should've merged this one ahead of my changes to our benches. I just renamed some of the files. We probably don't really need benches for the uuid! macro, since it should be "free" 🙂

@KodrAus
Copy link
Member

KodrAus commented Nov 1, 2021

Besides the merge conflict this should be ready to go now.

@QnnOkabayashi
Copy link
Contributor Author

QnnOkabayashi commented Nov 1, 2021

Besides the merge conflict this should be ready to go now.

Should be good to go now!

@KodrAus KodrAus merged commit fe7bbc7 into uuid-rs:main Nov 1, 2021
18 checks passed
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.

None yet

2 participants