proc macros ergonomics for ICU4X

[zbraniecki]

Rust is evolving its proc macros functionality, and we're already trying to use them.

I tried to write a patch that switches us to use icu-locale-macros that I added in #220 and there are two ergonomics issues that I think we should try to resolve:

Imports

In the perfect world, I'd like to do:

icu-locale = { version = 0.1, features = ["macros"] }

use icu_locale::macros::langid;

It doesn't seem like I can add icu-locale-macros as defined in #220 as an optional dependency on icu-locale because it creates a circular dependency.

That's unfortunate.

We can workaround it by adding:

icu-locale = "0.1"
icu-locale-macros = "0.1"

and then

use icu_locale_macros::langid;

That's unfortunate and I think it makes the experience worse, but I don't know if there's a good way out of it except of what I did in unic-locale with separating icu-locale-impl with code, then icu-locale-macros depend on it and icu-locale depends on those two. That seems like a high maintenance overhead (I know that it is for releasing unic-locale) and I'd love to avoid it, but I don't know how.

tinystr dependency

With the current import of

icu-locale = "0.1"
icu-locale-macros = "0.1"

and then

use icu_locale_macros::langid;

I'm seeing an error about unknown module tinystr and in result I need to add:

icu-locale = "0.1"
icu-locale-macros = "0.1"
tinystr = "0.3"

to make it work.
That's unergonomic and I'd love to have tinystr in scope because it is used in icu-locale-macros.

Again, we could resolve it with moving all code in icu-locale to icu-locale-impl and then having icu-locale be a wrapper that imports icu-locale-impl, icu-locale-macros and tinystr but that feels wrong :(

@Manishearth - can you recommend a path forward?

The patch I'm testing it on is based on #220, using Rust Beta (1.47) and cargo +beta test in components/datetime with:

diff --git a/components/datetime/src/lib.rs b/components/datetime/src/lib.rs
index 74b8f70..7ce4d25 100644
--- a/components/datetime/src/lib.rs
+++ b/components/datetime/src/lib.rs
@@ -9,13 +9,12 @@
 //! # Examples
 //!
 //! ```
-//! use icu_locale::LanguageIdentifier;
+//! use icu_locale_macros::langid;
 //! use icu_datetime::{DateTimeFormat, options::style};
 //! use icu_datetime::MockDateTime;
 //! use icu_data_provider::InvariantDataProvider;
 //!
-//! let langid: LanguageIdentifier = "en".parse()
-//!     .expect("Failed to parse a language identifier.");
+//! let en_langid = langid!("en");
 //!
 //! let provider = InvariantDataProvider;
 //!
@@ -24,7 +23,7 @@
 //!     time: Some(style::Time::Short),
 //!     ..Default::default()
 //! };
-//! let dtf = DateTimeFormat::try_new(langid, &provider, &options.into())
+//! let dtf = DateTimeFormat::try_new(en_langid, &provider, &options.into())
 //!     .expect("Failed to create DateTimeFormat instance.");
 //!
 //!

[Manishearth]

That's unergonomic and I'd love to have tinystr in scope because it is used in icu-locale-macros.

Reexport it from icu-locale-macros and then use crate::tinystr

That's unfortunate and I think it makes the experience worse, but I don't know if there's a good way out of it except of what I did in unic-locale with separating icu-locale-impl with code, then icu-locale-macros depend on it and icu-locale depends on those two. That seems like a high maintenance overhead (I know that it is for releasing unic-locale) and I'd love to avoid it, but I don't know how.

There is no good solution here aside from splitting into an impl crate, sadly. In the future there may be better ways to do this, folks have been talking about it for a while, but there aren't any concrete plans.

[zbraniecki]

That's unergonomic and I'd love to have tinystr in scope because it is used in icu-locale-macros.

Reexport it from icu-locale-macros and then use crate::tinystr

zbraniecki@zibi-xps13:~/projects/icu4x/components/datetime$ cargo +beta test
   Compiling icu-locale-macros v0.0.1 (/home/zbraniecki/projects/icu4x/components/locale/macros)
error: `proc-macro` crate types currently cannot export any items other than functions tagged with `#[proc_macro]`, `#[proc_macro_derive]`, or `#[proc_macro_attribute]`
 --> components/locale/macros/src/lib.rs:5:1
  |
5 | pub use tinystr;
  | ^^^^^^^^^^^^^^^^

error: aborting due to previous error

error: could not compile `icu-locale-macros`.

Did you mean icu-locale? That wouldn't work without the impl crate tho.

[Manishearth]

Sorry, I meant $crate. Maybe don't make it pub? I'm not sure

[Manishearth]

Otherwise, yeah, you'll have to require people to import tinystr

[zbraniecki]

I don't think $crate works in proc macros as discussed zbraniecki/tinystr#20 and rust-lang/rust#54363

Someone created https://crates.io/crates/proc-macro-crate and I'll try that.

[Manishearth]

Yeah but that won't be useful without the reexport. If you had a facade crate you could introduce the reexport there.

[sffc]

I'm totally fine with having two lines in Cargo.toml with and without macros. I don't think it's un-ergonomic; it's just as easy as adding a feature to the dependency.

It would be nice to get the tinystr name resolution problem resolved. I still am of the opinion that there is probably a way to solve it using $crate.

[zbraniecki]

I intend to merge #220 ASAP and we can work on ergonomics of it in a new PR.

[zbraniecki]

done. We still need to resolve the ergonomics question

[sffc]

I think we can implement the locale macros using const functions. The standard library compile_error!("message") lets us return errors at compile time from const functions. So, as long as we can check for subtag validity at compile time (which should be easy to do by making a few more functions const), there's no need for a proc macro.

EDIT: Darn, the compile_error! trick doesn't work. It unconditionally fails, even if the code block isn't executed. There are other tricks that would work, like those in the static_assertions crate. Hopefully we'll be able to soon start using const_panic as suggested in rust-lang/rust#51999.

[sffc]

Const functions work perfectly with the #![feature(const_panic)] feature enabled.

@Manishearth, any sense about when const_panic will be stabilized?

[sffc]

Okay, I also do not see any sign of string/slice indexing operations. I saw it suggested in rust-lang/rust#52000 that this could change (allowing for loops), but there doesn't seem to be a lot of movement on the idea of the fundamental indexing operation in const functions.

That being said, since we can pretty trivially implement things like TinyStr4::is_ascii_alphabetic() as const functions, we could limit the locale macro to be along the lines of

langid!("en")
langid!("en", "US")
langid!("en", "Latn", "US")

We can validate the subtags, but we can't parse the "en-Latn-US" syntax inside the const function in any clean way. (I guess you could convert the string to a TinyStr16 and do bitwise operations on that...)

tl;dr: There are still several open questions on this thread. I would like to stick with a locale_macros crate in v0.1 and get answers to these questions to target v0.2 or later.

[zbraniecki]

that sounds good to me.

[zbraniecki]

With #337 the core functionality seems to be working quite well. Thank you Shane!

One limitation I found is that the macros crate expect there to be icu_locale crate in scope, which is not the case when using it as part of icu meta package.

The solution is to unfortunately require:

[dependencies]
icu = "0.1"
icu-locale = "0.1"
icu-locale-macros = "0.1"

This could be solved by https://crates.io/crates/proc-macro-crate if we're willing to take it.

Then, we'd use it to differentiate between icu_locale::LanguageIdentifier and icu::locale::LanguageIdentifier.

@sffc - what do you think?

[sffc]

Copying my comment from #337 into here:

I changed the raw format back to primitives in order to eliminate the tinystr lookup problem.

However, the proc macro still has problems:

• Does not work if icu_locale is not in scope
• Two crates instead of one

We will work on these problems in the context of #310.

The limitation you found is bullet point 1 above. "proc-macro-crate" looks promising. It's worth trying if it works out of the box!

[sffc]

Not quite done: #338 resolves icu vs icu_locale, but I think it still requires icu_locale to be in scope. You still need to have it in your Cargo.toml file; you can't build with icu_locale_macros but not icu_locale. I also want to keep this open to finish the discussion above on const functions, and a path toward getting rid of the second crate.

[zbraniecki]

With the merge of #338 we now can use proc macros from icu_locale or icu!

If used as a separate module it has to be loaded separately, but icu meta crate gives it as icu::locale::macros.

I'm going to close this issue and we can reopen with more narrow scope of ergonomics.

[sffc]

Okay. I made a follow-up issue instead: #348