support required attribute on non-optional fields

[JeanMertz]

This is a rough draft, that is probably incomplete, but does compile on my end (I haven't tested it at runtime yet). I wanted to see how invasive it was to support required non-optional fields.

Turns out, somewhat invasive, but doable, specifically for the fallout of making from_partial fallible.

This isn't ready to merge, so feel free to close if you have no desire to support this. But if you do, I'll continue working on this until it works for my use-case, after which I can add tests to validate correctness.

See also: #99 (comment)

[milesj]

required only exists for situations where a type doesn't support Default, as there's no possible way to use it in the "final" config without wrapping it in Option. For example, semver::Version has this problem, and this is not possible:

pub version: Version,

The only way to make this "required" is to wrap it.

#[setting(required)]
pub version: Option<Version>,

But this is still kind of gross because it still has a major drawback: you must unwrap it to use the value!

I'm not a fan of this, so I'm attempting a different approach. Newtype wrappers that implement the necessary traits. For example, the new VersionSetting type: https://github.com/moonrepo/schematic/tree/master/crates/schematic/src/config/settings/semver.rs

With this, we can now simply do the following, and completely avoid required and Option.

pub version: VersionSetting,

If you want something to be truly required, you should use validation instead.

#[setting(validate = is_not_empty)]
pub value: String,

Because otherwise, how can you properly determine that a field is required (doesn't have a missing/empty value)? Based on your changes, I don't think this will actually work the way you want it to, because required checks should ideally happen at runtime, not macro compile time.

[JeanMertz]

required only exists for situations where a type doesn't support Default, as there's no possible way to use it in the "final" config without wrapping it in Option. For example, semver::Version has this problem, and this is not possible:

pub version: Version,

The only way to make this "required" is to wrap it.

#[setting(required)]
pub version: Option<Version>,

But this is still kind of gross because it still has a major drawback: you must unwrap it to use the value!

I understand the original purpose, and mentioned the ugliness of unwrap in the linked issue:

Even if this were possible currently with the required attribute on Option fields, that makes working with the library in the code itself less ergonomic, as that means I have to constantly unwrap that field, expecting/knowing that it can never be None, but only if generated through the schematic builder.

If you want something to be truly required, you should use validation instead.

#[setting(validate = is_not_empty)]
pub value: String,

Because otherwise, how can you properly determine that a field is required (doesn't have a missing/empty value)? Based on your changes, I don't think this will actually work the way you want it to, because required checks should ideally happen at runtime, not macro compile time.

The implementation does two things:

• At macro compile time, if a field value is not an Option, and it has the required macro, then we do not try to populate the Partial field with Some(Default::default()), but instead keep it as None.

• At runtime (when calling from_partial), instead of calling unwrap_or_default() on any partial field that is still None, we call ok_or(Error) on any field on the partial config that is marked as required.

Essentially, this means we've automated/codified your validate in the required property, which I believe is a nicer API to use for something as common as this, and since required already existed for Option<T>, making it work similarly for T additionally removes one more undesired implementation gap in the library that people could run into.

That is of course, unless I missed something in my implementation that makes it not work the way I expect it to, but I've added a few tests in my own CLI using this branch since then, and I haven't found any drawbacks/bugs so far.

The other major change that had to be done, is that impl Default for Config is no longer implemented for all Config types, but only for those that do not have any non-optional required fields. I consider this a good change, because not all types should have defaults, and guarding against someone doing MyNestedConfigType::default() and it resulting in an instance of that type that is technically not useable, is error-prone, unless you encode it in the type system, which this PR allows you to do. I understand that you can attach validators, but that doesn't guard against someone calling Default::default(), which doesn't run any validation.

[milesj]

Yeah you're right. I was misremembering how it was implemented.

However, I'm curious why you went with that approach and not the validate approach that the current required implementation uses? https://github.com/moonrepo/schematic/blob/master/crates/macros/src/config/field.rs#L171

I'm also hesitant to accept this because 1) it's a breaking change and I try not to do those often, and 2) I personally use the Default impl heavily.

[JeanMertz]

However, I'm curious why you went with that approach and not the validate approach that the current required implementation uses? https://github.com/moonrepo/schematic/blob/master/crates/macros/src/config/field.rs#L171

Well, I believe the code you linked is technically broken, because the ValidateError type is behind the validation feature flag, but that piece of code is not. If I wanted to use ValidateError, I had to still have a fallback solution for from_partial if validation wasn't enabled, or I'd have to remove the feature flag entirely and always have it enabled, which was a much bigger change than I wanted to introduce in a PR that (as you mentioned) I wasn't even sure was going to be accepted.

I'm also hesitant to accept this because 1) it's a breaking change and I try not to do those often, and 2) I personally use the Default impl heavily.

Correct. I can't think of a way to do this without introducing the breaking change. I still think it's highly desireable to allow modelling configuration structs in the exact way they are intended to be used for a given situation (e.g. optionals, required, and fields that do not have sensible default values).

Also, to be clear, Default impl still works, just not when you have a non-optional field marked as required, which is the only rational thing to do in that case. One can still manually implement Default in such situations, but then you defeat the whole point of marking a field as required, so you might as well remove it or replace it with default instead.

[milesj]

Well, I believe the code you linked is technically broken, because the ValidateError type is behind the validation feature flag, but that piece of code is not. If I wanted to use ValidateError, I had to still have a fallback solution for from_partial if validation wasn't enabled, or I'd have to remove the feature flag entirely and always have it enabled, which was a much bigger change than I wanted to introduce in a PR that (as you mentioned) I wasn't even sure was going to be accepted.

Good point. This is definitely an oversight on my part. The features were added only recently.

Also, to be clear, Default impl still works, just not when you have a non-optional field marked as required, which is the only rational thing to do in that case. One can still manually implement Default in such situations, but then you defeat the whole point of marking a field as required, so you might as well remove it or replace it with default instead.

While this makes sense, IMO, it would be confusing for users when the Default impl randomly does not exist because they're not familiar with the required logic. Maybe we can solve this with an attribute, like #[config(no_default)] or #[config(with_default)] or something. Let me think about it.

[JeanMertz]

Good point. This is definitely an oversight on my part. The features were added only recently.

You could look into running cargo-hack on the CI to see if all feature combinations compile as expected. That would capture issues like these.

Maybe we can solve this with an attribute, like #[config(no_default)] or #[config(with_default)] or something. Let me think about it.

I guess we could panic in the proc macro if required is used but no_default is not.