icu4x icon indicating copy to clipboard operation
icu4x copied to clipboard

CollatorBorrowed should have an infallible constructor from baked data

Open hsivonen opened this issue 5 months ago • 8 comments
trafficstars

AFAICT, the only way for CollatorBorrowed::try_new to fail is to override baked data with badly-generated baked data.

It seems to me that broken baked data is a different category of error than e.g. a bogus Postcard, so it seems to me that it would be appropriate to panic in the case of broken baked data and not expect the app to handle an error result from baked data-backed CollatorBorrowed construction.

hsivonen avatar May 27 '25 07:05 hsivonen

AFAICT, the only way for CollatorBorrowed::try_new to fail is to override baked data with badly-generated baked data.

No, you can give it bad options.

https://github.com/unicode-org/icu4x/blob/6187c85412aa9154ac2d8e6f5310fa297ff0026b/components/collator/src/comparison.rs#L577-L594

I'm going to close this since the premise is incorrect, but we can reopen if that premise changes.

Manishearth avatar May 29 '25 17:05 Manishearth

But you are correct: we do not error for "incorrect baked data"; we have GIGO behavior.

(we still try to not panic)

Manishearth avatar May 29 '25 17:05 Manishearth

I thought we had discussed at some point having a specialized constructor like new_root() that was infallible.

sffc avatar May 29 '25 17:05 sffc

See https://github.com/unicode-org/icu4x/issues/5856 for what we decided to do with Segmenter. I think we could do something very similar for Collator.

(going to re-open as I don't think this was addressed with @Manishearth's comment above)

sffc avatar May 29 '25 17:05 sffc

Or we could close this as a duplicate of #6633

sffc avatar May 29 '25 17:05 sffc

That's https://github.com/unicode-org/icu4x/issues/6633, we should close this as a duplicate then.

There are two potential issues: Adding const constructible new_root (#6633), and making try_new infallible (this issue, which cannot be done)

Manishearth avatar May 29 '25 17:05 Manishearth

AFAICT, the only way for CollatorBorrowed::try_new to fail is to override baked data with badly-generated baked data.

No, you can give it bad options.

What bad options? The error returns in the code you quoted are returns on bad data, and we're not supposed to have bad baked data.

hsivonen avatar Jun 12 '25 16:06 hsivonen

Yeah, it appears that the checks are for invariants between data structs, which shouldn't be reachable with baked data.

sffc avatar Jun 12 '25 16:06 sffc

Collator[Borrowed]::try_new does a data request with the preferences locale, which can always fail.

sffc avatar Aug 22 '25 21:08 sffc

@hsivonen am I missing something here?

sffc avatar Aug 22 '25 21:08 sffc