Use `Pattern` more directly #5531

robertbastian · 2024-09-11T14:56:04Z

Removed the Store generic from Pattern. It now always uses the unsized PatternBackend::Store. Instead of using Pattern<B, String> for the owned version, it's now Box<Pattern>
Implemented ToOwned for Pattern so that we can use Cow<Pattern>. The owned type is Box<Pattern>
Replaced the FromStr implementation by try_from_str, which returns Box<Self>
- Together these changes completely remove the need of downstream users to fuss around with the store.
Removed the PluralPatterns type, this now uses PluralElementsPackedCow<Pattern> directly
~~Changed pattern JSON serialization to use actual pattern string, instead of vecs of items~~
~~Changed CLDR parsing to use patterns directly in CLDR JSON structs~~

sffc

Praise: I'm in favor of this change and the work you did looks quite good. I have some suggestions/nits/issues about some corners.

sffc · 2024-09-11T16:25:34Z

utils/pattern/src/frontend/mod.rs

-    ///
-    /// To parse a pattern string, use [`FromStr::from_str`].
-    ///
-    /// # Examples


Nit: Bring back the example, please

This is a moot examples because users don't have to do store-juggling anymore.

Stores are still public, yes?

utils/pattern/src/frontend/mod.rs

sffc · 2024-09-11T16:29:25Z

utils/pattern/src/frontend/serde.rs

+        #[allow(clippy::unwrap_used)] // todo does this work?
+        Self(Pattern::try_from_boxed_store(Default::default()).unwrap())


Why do you need to impl Default?

To use this in CLDR JSON.

utils/pattern/src/common.rs

utils/pattern/src/frontend/mod.rs

sffc · 2024-09-12T21:18:54Z

@robertbastian I want to add custom baked constructors in order to fix #5230, but it would directly conflict with this PR. Are you planning to finish/merge this soon?

This reverts commit 6b6a535.

sffc

(almost done reviewing)

provider/source/src/tests/make_testdata.rs

sffc

Good work; nothing blocks landing this PR, which I prefer to do in order to prevent bitrot, but there are a few small things I'd like to see fixed in a follow-up

sffc · 2024-09-16T17:31:23Z

components/experimental/src/dimension/provider/currency.rs

+            deserialize_with = "icu_pattern::deserialize_option_borrowed_cow"
+        )
+    )]
+    pub standard_pattern: Option<Cow<'data, DoublePlaceholderPattern>>,


Thought: I think we should do as you suggested in the other issue. We can export a ZeroCow type from zerovec that has the correct Serde impls and reduces one layer of struct construction in the Bake impl. (Not in this PR)

utils/pattern/src/frontend/serde.rs

sffc · 2024-09-16T18:43:04Z

utils/pattern/src/frontend/serde.rs

+        D: serde::Deserializer<'de>,
+    {
+        Ok(Self(
+            Pattern::<B>::try_from_str(&<Cow<str>>::deserialize(deserializer)?, Default::default())


Nit: I think <Cow<str>>::deserialize always deserializes to a String. serde_derive has special-case code that essentially adds a deserialize_with if Syn sees a Cow<str>. It doesn't actually specialize Cow<str> to borrow when possible.

utils/pattern/src/frontend/serde.rs

sffc · 2024-09-16T18:49:54Z

utils/pattern/src/implementations.rs

-unsafe impl VarULE for Pattern<SinglePlaceholder, str> {
+unsafe impl<B> VarULE for Pattern<B>
+where
+    B: PatternBackend<Store = str>,


Suggestion: does this work with B::Store: VarULE?

robertbastian · 2024-09-16T19:33:11Z

Cool this triggers an ICE

sffc · 2024-09-16T20:47:19Z

error: internal compiler error: compiler/rustc_traits/src/codegen.rs:44:13: Encountered error SignatureMismatch(SignatureMismatchData { found_trait_ref: <fn(<frontend::Pattern as alloc::borrow::ToOwned>::Owned) -> alloc::borrow::Cow<'_, frontend::Pattern> {alloc::borrow::Cow::<'_, frontend::Pattern>::Owned} as core::ops::FnOnce<(<frontend::Pattern as alloc::borrow::ToOwned>::Owned,)>>, expected_trait_ref: <fn(<frontend::Pattern as alloc::borrow::ToOwned>::Owned) -> alloc::borrow::Cow<'_, frontend::Pattern> {alloc::borrow::Cow::<'_, frontend::Pattern>::Owned} as core::ops::FnOnce<(alloc::boxed::Box<frontend::Pattern>,)>>, terr: Sorts(ExpectedFound { expected: alloc::boxed::Box<frontend::Pattern<B/#2>, alloc::alloc::Global>, found: Alias(Projection, AliasTy { args: [frontend::Pattern<B/#2>], def_id: DefId(4:790 ~ alloc[a325]::borrow::ToOwned::Owned) }) }) }) selecting <fn(<frontend::Pattern as alloc::borrow::ToOwned>::Owned) -> alloc::borrow::Cow<'_, frontend::Pattern> {alloc::borrow::Cow::<'_, frontend::Pattern>::Owned} as core::ops::FnOnce<(alloc::boxed::Box<frontend::Pattern>,)>> during codegen

thread 'rustc' panicked at compiler/rustc_traits/src/codegen.rs:44:13:
Box

sffc

LGTM up to this point

sffc · 2024-09-17T01:39:01Z

The ICE says:

found_trait_ref:

<fn(<frontend::Pattern<B> as alloc::borrow::ToOwned>::Owned) -> alloc::borrow::Cow<'_, frontend::Pattern<B>> {alloc::borrow::Cow::<'_, frontend::Pattern<B>>::Owned} as core::ops::FnOnce<(<frontend::Pattern<B> as alloc::borrow::ToOwned>::Owned,)>>

expected_trait_ref:

<fn(<frontend::Pattern<B> as alloc::borrow::ToOwned>::Owned) -> alloc::borrow::Cow<'_, frontend::Pattern<B>> {alloc::borrow::Cow::<'_, frontend::Pattern<B>>::Owned} as core::ops::FnOnce<(alloc::boxed::Box<frontend::Pattern<B>>,)>>

So something about <frontend::Pattern as alloc::borrow::ToOwned>::Owned not matching with alloc::boxed::Box<frontend::Pattern> (even though they are the same type)

I see some HRTBs in this PR which probably doesn't help.

Also, this only happens in test-c and test-dart, which suggests a no_std-related issue (although for some reason it passes in other no_std tests).

Initial guess: what happens if you change the Pattern::from_ref_store_unchecked(B::empty()).to_owned() in impl Default to construct the box directly instead of trying to re-use impl ToOwned?

sffc · 2024-09-18T06:21:14Z

Oh I just noticed this in the error output, too

query stack during panic:
#0 [codegen_select_candidate] computing candidate for `<alloc::borrow::Cow<'_, frontend::Pattern<B>>::Owned as core::ops::function::FnOnce<(alloc::boxed::Box<frontend::Pattern<B>>,)>>`
#1 [resolve_instance] resolving instance `<alloc::borrow::Cow<'_, frontend::Pattern<B>>::Owned as core::ops::function::FnOnce<(alloc::boxed::Box<frontend::Pattern<B>>,)>>::call_once`
#2 [mir_callgraph_reachable] computing if `frontend::serde::deserialize_borrowed_cow::<'_, B, __D>` (transitively) calls `frontend::serde::deserialize_option_borrowed_cow::_::<impl at utils/pattern/src/frontend/serde.rs:20:14: 20:25>::deserialize`
#3 [optimized_mir] optimizing MIR for `frontend::serde::deserialize_option_borrowed_cow::_::<impl at utils/pattern/src/frontend/serde.rs:20:14: 20:25>::deserialize`
end of query stack

So it's in Serde deserialize_option_borrowed_cow. Maybe those two failing jobs are the only no_std + serde jobs.

sffc · 2024-09-18T16:30:13Z

@robertbastian ICE fixed. Ready to land?

robertbastian · 2024-09-18T16:37:05Z

it's not pretty that you have to add the generics when using deserialize_option_borrowed_cow, but for now we can merge

sffc · 2024-09-18T17:41:52Z

it's not pretty that you have to add the generics when using deserialize_option_borrowed_cow, but for now we can merge

We switched from exercising one compiler bug to another. Adding the generics is required due to rust-lang/rust#130180 (see #5510 (comment))

sffc · 2024-09-18T17:47:25Z

The way to actually fix this, I think, is to introduce a new type such as ZeroCow that has all the correct impls, and then we won't need deserialize_with.

sffc reviewed Sep 11, 2024

View reviewed changes

robertbastian force-pushed the genericplurals branch from 94d671a to 4691344 Compare September 11, 2024 18:06

robertbastian mentioned this pull request Sep 11, 2024

Fix quotes in patterns #5532

Merged

reoder

446a1a1

robertbastian force-pushed the genericplurals branch from 13a4b0f to 6b6a535 Compare September 16, 2024 11:56

robertbastian requested a review from sffc September 16, 2024 11:57

robertbastian marked this pull request as ready for review September 16, 2024 11:57

robertbastian requested review from zbraniecki, Manishearth, pdogr, younies and a team as code owners September 16, 2024 11:57

robertbastian force-pushed the genericplurals branch from 44de101 to 5a02124 Compare September 16, 2024 12:43

robertbastian added 4 commits September 16, 2024 14:47

pattern

6b97407

data

f7567dc

pretty json

7739669

Revert "pretty json"

ca82adb

This reverts commit 6b6a535.

robertbastian force-pushed the genericplurals branch from 5a02124 to ca82adb Compare September 16, 2024 12:47

sffc reviewed Sep 16, 2024

View reviewed changes

provider/source/src/tests/make_testdata.rs Outdated Show resolved Hide resolved

borrow

30c6630

robertbastian requested a review from sffc September 16, 2024 17:27

sffc previously approved these changes Sep 16, 2024

View reviewed changes

default

4b2706a

robertbastian dismissed sffc’s stale review via 4b2706a September 16, 2024 19:13

robertbastian requested a review from sffc September 16, 2024 19:33

robertbastian added 3 commits September 16, 2024 21:47

hide

b12e1ba

generalize

463e4c6

unsized

2d49916

fmt

4a8d73f

robertbastian removed request for zbraniecki, younies, Manishearth and pdogr September 16, 2024 20:16

sffc previously approved these changes Sep 17, 2024

View reviewed changes

Manishearth mentioned this pull request Sep 17, 2024

ICE: Encountered error SignatureMismatch rust-lang/rust#129214

Open

'static

9443bfa

robertbastian dismissed sffc’s stale review via 9443bfa September 17, 2024 15:39

sffc added 2 commits September 18, 2024 01:11

Tweaks

a51b45e

Make it build

f42cb71

sffc previously approved these changes Sep 18, 2024

View reviewed changes

nit

91a82e0

sffc dismissed their stale review via 91a82e0 September 18, 2024 16:34

robertbastian requested a review from sffc September 18, 2024 16:37

sffc approved these changes Sep 18, 2024

View reviewed changes

robertbastian merged commit 30d488e into unicode-org:main Sep 19, 2024
28 checks passed

robertbastian deleted the genericplurals branch September 19, 2024 08:15

This was referenced Sep 19, 2024

ZeroCow: a one-element, owned-or-borrowed VarULE #5561

Open

Improve and add more impls in Pattern crate #5241

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `Pattern` more directly #5531

Use `Pattern` more directly #5531

robertbastian commented Sep 11, 2024 •

edited

Loading

sffc left a comment

sffc Sep 11, 2024

robertbastian Sep 11, 2024

sffc Sep 11, 2024

robertbastian Sep 11, 2024

sffc Sep 11, 2024

robertbastian Sep 11, 2024

sffc commented Sep 12, 2024

sffc left a comment

sffc left a comment

sffc Sep 16, 2024

sffc Sep 16, 2024

sffc Sep 16, 2024

robertbastian commented Sep 16, 2024

sffc commented Sep 16, 2024 •

edited

Loading

sffc left a comment

sffc commented Sep 17, 2024

sffc commented Sep 18, 2024

sffc commented Sep 18, 2024

robertbastian commented Sep 18, 2024

sffc commented Sep 18, 2024

sffc commented Sep 18, 2024

		#[allow(clippy::unwrap_used)] // todo does this work?
		Self(Pattern::try_from_boxed_store(Default::default()).unwrap())

Use Pattern more directly #5531

Use Pattern more directly #5531

Conversation

robertbastian commented Sep 11, 2024 • edited Loading

sffc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Sep 12, 2024

sffc left a comment

Choose a reason for hiding this comment

sffc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertbastian commented Sep 16, 2024

sffc commented Sep 16, 2024 • edited Loading

sffc left a comment

Choose a reason for hiding this comment

sffc commented Sep 17, 2024

sffc commented Sep 18, 2024

sffc commented Sep 18, 2024

robertbastian commented Sep 18, 2024

sffc commented Sep 18, 2024

sffc commented Sep 18, 2024

Use `Pattern` more directly #5531

Use `Pattern` more directly #5531

robertbastian commented Sep 11, 2024 •

edited

Loading

sffc commented Sep 16, 2024 •

edited

Loading