From 36cb900478741bcc0175ee7aca493e21eb8692cc Mon Sep 17 00:00:00 2001 From: Ted Driggs Date: Wed, 26 Oct 2022 06:05:37 -0700 Subject: [PATCH] Produce error for `default` with `field.type` 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 --- derive_builder/CHANGELOG.md | 3 ++ .../compile-fail/builder_field_custom.rs | 14 ++++++- .../compile-fail/builder_field_custom.stderr | 24 +++++++++-- .../src/macro_options/darling_opts.rs | 41 ++++++++++++------- 4 files changed, 64 insertions(+), 18 deletions(-) diff --git a/derive_builder/CHANGELOG.md b/derive_builder/CHANGELOG.md index d5bb244d..0aa79933 100644 --- a/derive_builder/CHANGELOG.md +++ b/derive_builder/CHANGELOG.md @@ -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 diff --git a/derive_builder/tests/compile-fail/builder_field_custom.rs b/derive_builder/tests/compile-fail/builder_field_custom.rs index 1d5c5ade..74c37efb 100644 --- a/derive_builder/tests/compile-fail/builder_field_custom.rs +++ b/derive_builder/tests/compile-fail/builder_field_custom.rs @@ -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() {} diff --git a/derive_builder/tests/compile-fail/builder_field_custom.stderr b/derive_builder/tests/compile-fail/builder_field_custom.stderr index 9730ed5a..6cfc7335 100644 --- a/derive_builder/tests/compile-fail/builder_field_custom.stderr +++ b/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"))] + | ^^^ diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index b75f34cc..ebe1f380 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -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 { 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(