Skip to content

Commit

Permalink
Produce error for default with field.type
Browse files Browse the repository at this point in the history
If `field.type` is used without `field.build`, a Move conversion is used instead of the OptionOrDefault conversion.
This means that the presence of `field.type` prevents a field-level `default` from doing anything.
Standard practice in derive_builder is to produce an error when parameters are not going to be used.

Fixes #269
  • Loading branch information
TedDriggs committed Oct 26, 2022
1 parent 243a10a commit 36cb900
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 18 deletions.
3 changes: 3 additions & 0 deletions derive_builder/CHANGELOG.md
Expand Up @@ -2,6 +2,9 @@
All notable changes to this project will be documented in this file.
This project adheres to [Semantic Versioning](http://semver.org/).

## Unreleased
- Produce error when `default` is used with `field(type = "...")` rather than silently ignoring `default` #269

## [0.11.2] - 2022-04-20
- Allow restricted visibility using `vis = "..."` for builders, build methods, setters, and fields #247
- Allow specifying the type of a builder field using `#[builder(field(type = "..."))]` #246
Expand Down
14 changes: 13 additions & 1 deletion derive_builder/tests/compile-fail/builder_field_custom.rs
Expand Up @@ -3,8 +3,20 @@ extern crate derive_builder;

#[derive(Debug, PartialEq, Default, Builder, Clone)]
pub struct Lorem {
#[builder(default = "88", field(type = "usize", build = "self.ipsum + 42"))]
// `default` is incompatible with `field.build`
#[builder(
default = "1",
field(build = "self.ipsum.map(|v| v + 42).unwrap_or(100)")
)]
ipsum: usize,

// `default` is incompatible with `field.type`, even without `field.build`
#[builder(default = "2", field(type = "usize"))]
sit: usize,

// Both errors can occur on the same field
#[builder(default = "3", field(type = "usize", build = "self.ipsum + 42"))]
amet: usize,
}

fn main() {}
24 changes: 21 additions & 3 deletions derive_builder/tests/compile-fail/builder_field_custom.stderr
@@ -1,5 +1,23 @@
error: #[builder(default)] and #[builder(field(build="..."))] cannot be used together
--> tests/compile-fail/builder_field_custom.rs:6:25
--> tests/compile-fail/builder_field_custom.rs:8:19
|
6 | #[builder(default = "88", field(type = "usize", build = "self.ipsum + 42"))]
| ^^^^
8 | default = "1",
| ^^^

error: #[builder(default)] and #[builder(field(type="..."))] cannot be used together
--> tests/compile-fail/builder_field_custom.rs:14:25
|
14 | #[builder(default = "2", field(type = "usize"))]
| ^^^

error: #[builder(default)] and #[builder(field(build="..."))] cannot be used together
--> tests/compile-fail/builder_field_custom.rs:18:25
|
18 | #[builder(default = "3", field(type = "usize", build = "self.ipsum + 42"))]
| ^^^

error: #[builder(default)] and #[builder(field(type="..."))] cannot be used together
--> tests/compile-fail/builder_field_custom.rs:18:25
|
18 | #[builder(default = "3", field(type = "usize", build = "self.ipsum + 42"))]
| ^^^
41 changes: 27 additions & 14 deletions derive_builder_core/src/macro_options/darling_opts.rs
Expand Up @@ -335,30 +335,43 @@ impl Field {

/// Resolve and check (post-parsing) options which come from multiple darling options
///
/// * Check that we don't have a custom field builder *and* a default value
/// * Check that we don't have a custom field type or builder *and* a default value
/// * Populate `self.field_attrs` and `self.setter_attrs` by draining `self.attrs`
fn resolve(mut self) -> darling::Result<Self> {
let mut errors = darling::Error::accumulator();

// `field.build` is stronger than `default`, as it contains both instructions on how to
// deal with a missing value and conversions to do on the value during target type
// construction. Because default will not be used, we disallow it.
// `default` can be preempted by properties in `field`. Silently ignoring a
// `default` could cause the direct user of `derive_builder` to see unexpected
// behavior from the builder, so instead we require that the deriving struct
// not pass any ignored instructions.
if let Field {
default: Some(field_default),
field:
FieldLevelFieldMeta {
build: Some(_custom_build),
..
},
..
} = &self
{
errors.push(
darling::Error::custom(
r#"#[builder(default)] and #[builder(field(build="..."))] cannot be used together"#,
// `field.build` is stronger than `default`, as it contains both instructions on how to
// deal with a missing value and conversions to do on the value during target type
// construction.
if self.field.build.is_some() {
errors.push(
darling::Error::custom(
r#"#[builder(default)] and #[builder(field(build="..."))] cannot be used together"#,
)
.with_span(field_default),
);
}

// `field.type` being set means `default` will not be used, since we don't know how
// to check a custom field type for the absence of a value and therefore we'll never
// know that we should use the `default` value.
if self.field.builder_type.is_some() {
errors.push(
darling::Error::custom(
r#"#[builder(default)] and #[builder(field(type="..."))] cannot be used together"#,
)
.with_span(field_default)
)
.with_span(field_default),
);
}
};

errors.handle(distribute_and_unnest_attrs(
Expand Down

0 comments on commit 36cb900

Please sign in to comment.