Make derive(ULE) check for public fields

[sffc]

Consider the following struct, which exists in ICU4X:

pub struct Region(TinyAsciiStr<REGION_NUM_LENGTH>);

To make this into a ULE, it is tempting to simply apply #[derive(ULE)]. However, this is unsafe, because it enables the creation of Region instances that are not valid region subtags.

This footgun is shared with #[derive(Deserialize)], as we discuss at length in #1290.

To fix this, I propose that we:

1. By default, require that fields are pub when #[derive(ULE)] is applied
2. Allow a user to opt out of this behavior with something like #[zerovec::allow_private(reason = "...")]

@Manishearth

[Manishearth]

Sounds good, though some additional changes to how attribute parsing works may need to happen to support the reason field.

[Manishearth]

So in general my worry here is that Rust really doesn't have a story for doing warnings in proc macros, and producing an error for what really should be a warning feels really janky to me. Most users will not be using the #[derive(ULE)] so it's not a huge deal but if anything that somewhat makes me less likely to want to do this since it would be an extra feature to protect a corner of the API that even we use only a couple times.

There are many reasons for fields to be private, not just because of invariants. The philosophy for lints we apply on clippy is to try and minimize annoyance in general, but be okay with folks with special needs disabling things. However, for that to be true we need to be sure of what the common case is, and my experience is not that "has invariants" is the overarching common case for private fields. Clippy is also super conservative about deny-by-default lints, and this seems similar to me.

So while I'm not super strongly against it, I think I'll revise my initial position to be somewhat against it.

[Manishearth]

@sffc thoughts on the above? In general I'm not really in favor of the lower level API having janky warnings-as-errors since someone using the lower level API probably already knows what they intend.

[sffc]

I don't think my position is changed relative to the OP

• I see no substantive difference between warnings and errors that can be suppressed
• I disagree that #[derive(ULE)] is a low-level thing, since it's common to use it on types that are transparent over ULE (such that ULE = Self)
• I tend to feel in cases like this that we should assume the worst and allow users to tell us otherwise

[sffc]

This is not a 1.0 issue though since this affects a util crate. It is 1.0 for utils only.

[Manishearth]

I see no substantive difference between warnings and errors that can be suppressed

This is not an error that can be suppressed at a global level, it has to be suppressed per-type. People really dislike warnings that can't be disabled that way, there's a reason Rust has rich infrastructure around this.

Like, my main issue here is that when users don't want an error it's very annoying and they consider the crate less usable. It's a common problem we see with clippy, which is very much about explicitly opting in to errors! We're extremely careful about what is error-by-default in clippy, and clippy does have global controls.

"warnings" in macros as errors are not a Rusty pattern.

This is not a 1.0 issue though since this affects a util crate. It is 1.0 for utils only.

I'm going to clear the milestone then and put it in 2.0 or something.