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

Macro-based baked data #3449

Merged
merged 12 commits into from
Jun 2, 2023
Merged

Macro-based baked data #3449

merged 12 commits into from
Jun 2, 2023

Conversation

robertbastian
Copy link
Member

@robertbastian robertbastian commented May 24, 2023

When reviewing, look at:

  • icu_globaldata rustdoc
  • The usage in icu_list
  • This commit that tries to split data into separate files again, which does not work because macro-lookup in icu_testdata (or anywhere that include!s baked data) cannot use the $crate prefix, but macro-lookup in icu_globaldata requires it

Turn off deleted files in the diff view to make it not lag.

#2743 #2945

@jira-pull-request-webhook

This comment was marked as spam.

@jira-pull-request-webhook

This comment was marked as spam.

@jira-pull-request-webhook

This comment was marked as spam.

@jira-pull-request-webhook

This comment was marked as spam.

@robertbastian robertbastian changed the title Globaldata WIP Global data for icu_list May 30, 2023
@robertbastian robertbastian marked this pull request as ready for review May 30, 2023 15:35
@robertbastian robertbastian requested review from sffc, Manishearth and a team as code owners May 30, 2023 15:35
@robertbastian robertbastian changed the title Global data for icu_list Global data for icu_list and icu_plurals May 31, 2023
@jira-pull-request-webhook

This comment was marked as spam.

@robertbastian robertbastian changed the title Global data for icu_list and icu_plurals Macro-based baked data May 31, 2023
@robertbastian
Copy link
Member Author

I've rescoped this PR, it now only changes the baked format.

}
}
#[doc(inline)]
pub use __impl_data_provider as impl_data_provider;
Copy link
Member

Choose a reason for hiding this comment

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

Question: why the rename and re-export?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because macros are weird. macro_export will put them at the root of the crate, so we hide that, and pub use respects module structure. This is explained in a comment in line 465.

@sffc
Copy link
Member

sffc commented Jun 1, 2023

Here's an example of what it generates for a single key

#[doc(hidden)]
#[macro_export]
macro_rules! __singleton_calendar_japanese_v1 {
    () => {
        icu_calendar::provider::JapaneseErasV1 {
            dates_to_eras: unsafe { zerovec::ZeroVec::from_bytes_unchecked(b"blahblahblah") },
        }
    }
}

#[doc(hidden)]
pub use __singleton_calendar_japanese_v1 as singleton_calendar_japanese_v1;

#[doc = " Implement [`DataProvider<JapaneseErasV1Marker>`](icu_provider::DataProvider) on the given struct using the data"]
#[doc = r" hardcoded in this file. This allows the struct to be used with"]
#[doc = r" `icu`'s `_unstable` constructors."]
#[doc(hidden)]
#[macro_export]
macro_rules! __impl_calendar_japanese_v1 {
    ($provider: path) => {
        # [clippy :: msrv = "1.61"]
        impl icu_provider :: DataProvider < icu_calendar :: provider :: JapaneseErasV1Marker > for $ provider {
            fn load (& self , req : icu_provider :: DataRequest ,) -> Result < icu_provider :: DataResponse < icu_calendar :: provider :: JapaneseErasV1Marker > , icu_provider :: DataError > {
                req . locale . is_empty ()
                    . then (|| {
                        static ANCHOR : < icu_calendar :: provider :: JapaneseErasV1Marker as icu_provider :: DataMarker > :: Yokeable = singleton_calendar_japanese_v1 ! () ; & ANCHOR
                    }) . map (icu_provider :: prelude :: zerofrom :: ZeroFrom :: zero_from)
                    . map (icu_provider :: DataPayload :: from_owned)
                    . map (| payload | {
                        icu_provider :: DataResponse { metadata : Default :: default () , payload : Some (payload) , }
                    }) . ok_or_else (|| icu_provider :: DataErrorKind :: MissingLocale . with_req (< icu_calendar :: provider :: JapaneseErasV1Marker as icu_provider :: KeyedDataMarker > :: KEY , req))
            }
        }
    }
}

#[doc(inline)]
pub use __impl_calendar_japanese_v1 as impl_calendar_japanese_v1;

@robertbastian robertbastian requested a review from sffc June 1, 2023 09:52
@Manishearth
Copy link
Member

I'm fine with this landing. I have a couple concerns:

  • I would eventually like these files to get nicely rustfmted again. It may mean moving off of quote for the final construction of code.
    • Reasoning: I have found myself looking at baked data when debugging (and I expect people unfamiliar with our architecture will do this even more often). It's useful. If we're using macros, it's going to be even more useful; people want to look at macro source to figure out how to invoke it.
  • I would like this to be one-file-per-macro. I recognize that this is not easy to achieve, but I am skeptical that there is no way to do so.
    • Reasoning: Smaller files, easier to read when necessary. More importantly keeps the door open for multi-crate solutions, and patching a key is far cleaner on people who vendor our code.

@robertbastian
Copy link
Member Author

I would eventually like these files to get nicely rustfmted again. It may mean moving off of quote for the final construction of code.
I would like this to be one-file-per-macro. I recognize that this is not easy to achieve, but I am skeptical that there is no way to do so.

Me too. As discussed I'll try to make this work after globaldata when the constraints are clearer

patching a key is far cleaner on people who vendor our code.

Oh no we should not support that. Just regen.

@Manishearth
Copy link
Member

@robertbastian not manually patching, but even in a regen you want manageable diffs

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

@robertbastian robertbastian merged commit 016ffd1 into unicode-org:main Jun 2, 2023
@robertbastian robertbastian deleted the bake branch June 14, 2023 15:19
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