Make derive(ULE) check for public fields

[sffc]

trafficstars

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.

[sffc]

I think we decided not to stabilize utils for ICU4X 1.0; only component APIs and data file format. I do want to get a decision on this issue for ZeroVec 1.0

[sffc]

Is there something like a Clippy lint plugin framework where zerovec adds zerovec-specific lints to Clippy that can be controlled as Clippy lints; then we could add this as a lint ("don't use derive(ULE) on structs with private fields")

[Manishearth]

Nope. Clippy has plans for this but it's a lot of work, and most likely would not be something a library can impose on clients anyway.

That said, you could propose some kind of #[clippy::warn_if_private] annotation if you can justify it in the general sense.

Alternatively, a good proposal would be for there to be a #[clippy::proc_macro_lint("zerovec", "description")] that allows proc macros to emit lints, and has some mechanism to be disabled at a per-crate level. This is also a lot of design work but easier to implement.

[sffc]

Hmm. #[derive(ULE)] could add #[clippy::warn_if_private] to the struct. It would make Clippy generate a warning clippy::exposed_private_fields if there are any private fields, and then #[allow(clippy::exposed_private_fields)] would suppress it.

[Manishearth]

Correct, but you need to be able to justify the existence of such a lint to other clippy maintainers, and as a primary maintainer of zerovec and clippy I'd probably not participate in that discussion due to the conflict:)

[sffc]

Note: related to this, I have an old draft PR to add validate_with: #2733