Remove Default impls that load data #5554
Comments
That's not what that says. It says
"This comes up often with structs that define a set of options", but is in no way limited to that. Compiled data is reasonable default data, that's why we're publishing it.
So? Some APIs are gated, and that includes impls.
It only exists when
Call sites can be written using |
|
The issue title misrepresents what's happening. There is never any "data loading" in a impl Default for SentenceSegmenter {
fn default() -> Self {
Self::new()
}
}
impl SentenceSegmenter {
pub fn new() -> Self {
Self {
payload: DataPayload::from_static_ref(
crate::provider::Baked::SINGLETON_SENTENCE_BREAK_DATA_V2_MARKER,
),
payload_locale_override: None,
}
}
} |
|
I don't think I agree with any of these points.
I don't agree with this. I've seen
Seems fine. A lot of things are gated on
There's no expectation of
This is a rather subjective style choice and i don't see a need to force it on our clients. They can always call |
|
Ok so it's not noncontroversial I'm reasonably convinced by the counter-arguments on points 1. On point 4, I'm mainly concerned when it is used for passing these things as unnamed positional arguments, but we can avoid that by not adding such APIs. I'm not convinced by the counter-arguments on points 2 and 3, and there are more I'd like to add:
I'll add to this: the fact that it is gated on
I used SentenceSegmenter as an example because it has two fields, one singleton and one non-singleton. Currently the non-singleton has no data in To respond to @Manishearth's "There's no expectation of Default to be cheap": my expectation is that it is cheap. For example, it is used in bounds on things like constructing an array and other operations that where the impl is called a large number of times. Is your argument an "in my experience" argument or "this is a documented convention" argument? New points:
|
|
On the point 7: we have generally been conservative with adding impls to our types, and I think it has served us well. We have a very large API surface, and every impl adds cost. The only impl we agreed to universally add has been |
Right, but if they're doing that they presumably need some default, without
Okay, you should not hold on to that expectation because it is not one followed by the ecosystem. My argument is one based on the ecosystem, and the fact that it is not documented as being cheap. I would consider this something that needs to be explicitly documented were it an expectation. Worth noting that our
I don't consider this to be a requirement of the
I'm open to having some types have
I can provide the motivation for the clippy lint: I definitely do not think this is a case in which the clippy lint is wrong. My position is that deviating from clippy here is something that needs justification, not the new impl. |
Example developer journey: they are using postcard data. They apply |
This ... seems like exactly what we want to happen? They now are forced to consider where their data should come from. That's good. That should exactly be a factor of the decision to disable the feature. I don't really think "they don't fully understand so they leave it alone" is that likely. It doesn't seem that confusing. We could add a "enabled by feature foo" tag on the impl if we really want to improve this experience. |
My point is that they should be forced to consider where their data comes from up front by using an API explicitly documented as "with compiled data", which was key to us agreeing to have the compiled data functions have short, convenient names. |
The problem with docs on trait impls is that they are not discoverable. Often, Default is used implicitly, as you noted with |
I mean, if you get a "missing Default impl" when you play with features I don't really see why it would be hard to figure out what happened. The docs on the trait impl are just an added bonus. I'm not convinced of the sketched developer story being particularly onerous.
I think that's fine? For singletons if someone is using a type they almost certainly need the data and 99% of users will get the data from the compiled data; the main benefit of loading data from a file for singletons is if it's some large program that wishes to lazily load everything. I can see a potential additional benefit of wishing to load the latest data. I want most of our singleton users to be able to just People who care about data to that granularity are going to be caeful about bounds anyway. Just out of curiosity, this is the current list of gated |
Currently we have a number of
impl Defaulton classes that have anew()function that load data. The data is usually singleton but doesn't need to be (see SentenceSegmenter for instance).This is bad because:
impl Defaultis to fill in default values for fields when there are reasonable default values. But these are opaques, not bag-of-field structs, andimpl Defaultis just not in the right spirit.impl Defaultis gated on thecompiled_datafeature, which is dangerous since it can be disabled.impl Defaultmight do work, since it is loading data. It should ideally exist only when POD can be filled in.TimeZone+ResolvedTimeZone#5549, where an unnamed positional argument that isn't an options bag is elided with&Default::default().We never had these impls until we added compiled_data constructors, and then we added them only because Clippy was telling us to. I don't think we ever really had a thorough discussion about it.
I hope this is a no-brainer and non-controversial.
@Manishearth @robertbastian @zbraniecki @hsivonen
The text was updated successfully, but these errors were encountered: