Move error_value (and optionally default_value) to the CPT header?

[sffc]

See previous discussion in #1183.

The thread #1183 got derailed into a discussion on trust boundaries. I moved that conversation over to #1290. Our work on CrabBake is our solution to that side of the problem and allows us to reason about "invalid serialized data" in the serde case.

I increasingly feel that the option most consistent with how our data model works is to move these two fields, especially error_value, into the CPT header. This would help us eliminate the problematic DATA_GET_ERROR_VALUE.

This should be an easy change: the error value is always the last value in the data array, so at datagen time, we just remove the last value from the array and save it into the header.

Responses to the pushback on this idea from the previous thread:

@markusicu: Between UTrie2 and the newer CodePointTrie, I moved the error value from the header struct into the data array in order to make the code simpler and more uniform.

In Rust, I do not believe that this will increase code complexity. It would involve adding a type parameter to CodePointTrieHeader, but this parameter would just be bubbled down from CodePointTrie, which already has a type parameter.

It may require adding #[derive(Deserialize)] on TrieType types that don't already have it.

@hsivonen: This would make ICU4X compute bogus results if externally-traveled data actually has been corrupted. In terms of surprise and what it takes to understand what's going wrong, this seems worse than any of the other options. However, in terms of usage ergonomics, I'd rather settle on this option than option 2 if we can't agree on panicking on malformed data at the time of making lookups from the data.

We are moving from one type of GIGO to another type of GIGO. The new type of GIGO is in fact slightly more robust, because it allows us to automatically fail in the constructor if the error value field is not present in the header, something we can't as easily do when it is stored in the data array. Discussion of when or when not to rely on GIGO is the topic of #1290 and I do not want to derail this thread again on that separate, very important issue.

Needs approval from:

• @echeran
• @markusicu
• @Manishearth

[Manishearth]

This makes sense to me.

[echeran]

It seems fine to me, it seems to be an incremental improvement on code clarity and safety without actually changing any behavior.

If you're so inclined, since this value should be the last element in the CPT's data array (according to this constant), you could truncate the data array by 1 element. But I don't know if that's beneficial.

[Manishearth]

@pandusonu2 is working on this

The bug is to add an error_value field to this type, of type T

icu4x/utils/codepointtrie/src/codepointtrie.rs

Line 114 in b6774e2

data: ZeroVec<'trie, T>,

And we can parse it in from the last element of the data array here:

icu4x/utils/codepointtrie/src/toml.rs

Lines 103 to 122 in b6774e2

	impl<T: TrieValue> TryFrom<&CodePointTrieToml> for CodePointTrie<'static, T> {
	type Error = Error;
	fn try_from(cpt_data: &CodePointTrieToml) -> Result<CodePointTrie<'static, T>, Self::Error> {
	use CodePointDataSlice::*;
	let header = CodePointTrieHeader::try_from(cpt_data)?;
	let index: ZeroVec<u16> = ZeroVec::alloc_from_slice(&cpt_data.index);
	let data: Result<ZeroVec<'static, T>, T::TryFromU32Error> = match cpt_data.data_slice()? {
	U8(s) => s.iter().map(|i| T::try_from_u32(*i as u32)).collect(),
	U16(s) => s.iter().map(|i| T::try_from_u32(*i as u32)).collect(),
	U32(s) => s.iter().map(|i| T::try_from_u32(*i as u32)).collect(),
	};
	let data = data.map_err(|_| Error::FromDeserialized {
	reason: "Could not parse data array to typed array",
	})?;
	CodePointTrie::<T>::try_new(header, index, data)
	}
	}

And then replace all uses of the following constant with it:

icu4x/utils/codepointtrie/src/codepointtrie.rs

Line 44 in b6774e2

const DATA_GET_ERROR_VALUE: Self;

We also probably need a pub error_value() function that returns this.

[sffc]

@pandusonu2 What is the status of this issue?