Add try_setter option and generate appropriate methods #72

TedDriggs · 2017-04-10T20:00:39Z

This implements the method-level proposal described in #25. The initial PR is not ready for merging; it's meant to start a discussion.

Open Questions

What blanket TryFrom and TryInto implementations will be provided when the feature stabilizes? Right now, the lack of support among built-in types makes this only narrowly useful. It looks like there will be a blanket impl for TryFrom<&'a str> for things that currently implement FromStr (source comment)
Is there an annotation needed on the generated methods to allow an exporting crate to offer these methods only when the consuming crate has enabled the language try_from feature?

NOT YET DONE * Generate appropriate documentation for `try_` methods * Confirm core::convert::TryInto exists * Finalize declaration (`try_setter` vs. `setter(include_try)` CAVEATS * An error on a try_ with the owned builder pattern is irrecoverable because the builder was consumed and not returned * Without error_chain or the like, error handling can be verbose

colin-kiegel · 2017-04-10T21:21:24Z

Hey, that was fast. Hm, good questions - especially the second one.

String parsing will probably be the most ubiquitous use case.
I think #[cfg_attr(feature = "foo", builder(...))] should allow crates to set builder attributes conditionally. We could add that to a tips section. And if that doesn't work I'd just wait and hope that TryFrom doesn't take too long for stabilization.

I'll look at the code tomorrow. :-)

colin-kiegel

Ok, looks good so far. I left some feedback, which I'll also summarize here. :-)

Please incorporate these changes

rename the FieldOptions, OptionsBuilder and Setter field to try_setter.
remove the || self.try_enabled and pull out the shared variables
remove the redundant #deprecation_notes

And don't forget to

update changelog
update documentation in lib.rs (optional: Readme)
add unit tests to derive_builder_core/src/setter.rs (two variants (a) vanilla settings (b) everything customized, custom name, custom builder pattern, custom privacy etc.)
add integration tests to derive_builder_test/tests/ (two variants as with the unit tests please). Also see this coment about the neccessary feature gating of these tests.

colin-kiegel · 2017-04-11T09:55:28Z

derive_builder/src/options/field_options.rs

@@ -31,6 +31,8 @@ pub struct FieldOptions {
    pub attrs: Vec<syn::Attribute>,
    /// Bindings to libstd or libcore.
    pub bindings: Bindings,
+    /// Enables code generation for the TryInto setter.
+    pub try_setter_enabled: bool,


please change this to try_setter

colin-kiegel · 2017-04-11T09:56:01Z

derive_builder/src/options/mod.rs

@@ -45,6 +45,7 @@ pub struct OptionsBuilder<Mode> {
    setter_vis: Option<syn::Visibility>,
    default_expression: Option<DefaultExpression>,
    setter_into: Option<bool>,
+    try_setter_enabled: Option<bool>,


please change this to try_setter

colin-kiegel · 2017-04-11T09:56:19Z

derive_builder_core/src/setter.rs

@@ -34,6 +34,8 @@ use Bindings;
 pub struct Setter<'a> {
    /// Enables code generation for this setter fn.
    pub enabled: bool,
+    /// Enables code generation for the `try_` variant of this setter fn.
+    pub try_enabled: bool,


please change this to try_setter

colin-kiegel · 2017-04-11T09:58:38Z

derive_builder_core/src/setter.rs

@@ -58,7 +60,7 @@ pub struct Setter<'a> {

 impl<'a> ToTokens for Setter<'a> {
    fn to_tokens(&self, tokens: &mut Tokens) {
-        if self.enabled {
+        if self.enabled || self.try_enabled {


please remove the || self.try_enabled. If we piggy back the try-variant onto the namespace and code generation of the standard setter, there should be no try-setter without the standard setter.

PS: You can either pull all shared variables out of the if self.enabled { statement, or just emit two Setter objects per field (as suggested in another comment).

colin-kiegel · 2017-04-11T10:01:02Z

derive_builder_core/src/setter.rs

+                    #vis fn #try_ident #try_ty_params (#self_param, value: VALUE)
+                        -> #result<#return_ty, VALUE::Err>
+                    {
+                        #deprecation_notes


Please remove this line, because we want to emit this only once per setter combination.
Since we piggy back the try-variant onto the standard setter, it will already be emitted in the standard setter.

colin-kiegel · 2017-04-11T10:24:21Z

derive_builder/Cargo.toml

@@ -25,6 +25,7 @@ proc-macro = true
 logging = [ "log", "env_logger", "derive_builder_core/logging" ]
 struct_default = []
 skeptic_tests = ["skeptic"]
+try_setter = []


The user should not need to set a feature flag to use generate try-setters.

Instead we only need some feature flag to activate the integration tests. The situation with having multiple crates in a workspace and running tests on travis is a bit complicated. When you look at travis.yml:57, you'll see that tests are started in the derive_builder_test folder with the arguments --all --no-fail-fast --features "$CARGO_FEATURES". My experience is that the features are only passed to the derive_builder_test crate, but not derive_builder or derive_builder_core unless derive_builder_test/Cargo.toml specifies to propagate the features (which is does not right now). So the easiest solution is to place all integration tests that require nightly into derive_builder_test/tests and use a feature flag in the derive_builder_test crate. That also has the advantage that the public derive_builder does not expose any feature flag that's useless for users.

You can use the compiletests feature flag and rename it to nightlytests. Please also change all occurences of feature = "compiletests", but don't change filenames compiletests.rs or other occurences of the word. ;-)

Let me know if you need any further advice.

colin-kiegel · 2017-04-11T11:03:23Z

derive_builder/src/options/mod.rs

@@ -198,6 +206,9 @@ impl<Mode> OptionsBuilder<Mode> where
                // setter implicitly enabled
                self.setter_enabled(true)
            },
+            "try_setter" => {
+                self.try_setter_enabled(true)


please set setter_enabled implicitly if the user specifies #[builder(try_setter)]

"try_setter" => { /* ... */ // setter implicitly enabled self.setter_enabled(true) }

Or wait - let me think about it again. Initially I thought you were implementing an API like #[builder(setter(try))] as in this comment.

I think the #[builder(setter(try))] is the better API in the long run, as having a try_{field} without the regular setter seems bad.

ok, please forget that comment about implicitly enabling the standard setter - as we decided not to piggy back. :-)

colin-kiegel · 2017-04-11T11:26:02Z

@TedDriggs before we continue with this PR let's first discuss what behaviour want to have in issue #25

* Renamed try_setter_enabled and try_enabled to `try_setter` * Removed excess #deprecation_notes * Removed `|| self.try_enabled`

colin-kiegel · 2017-04-11T16:48:27Z

I was thinking, if we go with the approach (c) discussed here, it might make sense to abstract over try_setter vs. setter differently. The current implementation of a Setter object can emit both setters in one go. Alternatively we could have to methods as_setter() and as_try_setter() on FieldOptions. Both would then each generate one Setter object with different settings. This would keep the complexity in the Setter object lower when we add options to overwrite the try_setter name etc.

I'm absolutely ok with deferring that refactoring to the point where we need it. But if you feel like doing it now, I'd welcome that of course. You decide. ;-)

TedDriggs · 2017-04-11T17:05:43Z

There's one more issue I need to address, which is controlling the generation of these based on the presence of the try_from language feature. There appear to be three possible ways to accomplish this:

Do a cfg!(feature = "try_from") check in derive_builder, and pass false to core if that feature isn't enabled.
Do a cfg!(feature = "try_from") check in derive_builder_core, and only emit the setter when the language feature is present.
Emit #[cfg(feature = "try_trom")] on the generated try_setter. This might require reconciling with existing cfg attributes added on the base struct.

I think approach 1 is the simplest, but I'm not sure if that makes the determination about exposing these be part of the exporting crate, or the consuming crate. Ideally, the consuming crate's language features would control this.

TedDriggs · 2017-04-11T17:22:02Z

derive_builder/examples/try_setter.rs

+}
+
+fn create(name: &str, addr: &str) -> Result<Lorem, String> {
+    // Fallible and infallible setters can be mixed freely when using


@colin-kiegel can you please try running cargo expand --example try_setter on this and watch what happens to the comment? Something weird is happening here, but I'm not sure what.

Let me guess, does the comment appear at random places in the generated output? :-)

I noticed very strange distributions of comments before. I could imagine that this is caused by the syn crate, which we use to parse the input into an ast. But didn't investigate it, because it didn't cause any trouble.

It seems to consistently get moved into the build method just before the uninitialized field error string for "name", even when I move it around in the raw source.

colin-kiegel

I noticed that you shortened the test in derive_builder_test/tests/run-pass/try_setter.rs. I know that I put some testcases in the run-pass folder where that doesn't add any value.

I think you can just move this whole test case up one level. This will give you all the usual test features. compiletests in the run-pass folder are really only needed, if you want to assert certain compiler outputs. Like if you want to check that it compiles and runs, but emits a deprecation warning or some other lint. We don't need that here and in that case using compile-tests only makes it more complicated. ;-)

colin-kiegel · 2017-04-11T17:06:41Z

derive_builder_core/src/setter.rs

+        setter.try_setter = true;
+
+        // Had to hoist these out to avoid a recursion limit in the quote!
+        // macro invocation below.


no, problem. You can just add #![cfg_attr(test, recursion_limit="100")] to the crate root. That should increase the recursion limit in test-mode only. Just put in a number that's roughly twice as high as the current minimum requirement. :-)

colin-kiegel · 2017-04-11T17:07:38Z

derive_builder_core/src/setter.rs

+            }
+        );
+
+        // hoisting necessary to avoid recursion limit.


colin-kiegel · 2017-04-11T17:10:21Z

derive_builder_test/tests/run-pass/try_setter.rs


 #[macro_use]
 extern crate derive_builder;

 #[allow(unused_imports)]
 mod struct_level {
-    #[cfg(feature = "try_setter")]
+    #[cfg(feature = "try_from")]


ok, gating on try_from instead of nightlytests is actually a good idea here. Please do it consistently in the rest of the test then - that'd be easier to understand. :-)

Alternatively also gate on nightlytests here.

colin-kiegel · 2017-04-11T17:13:16Z

derive_builder/CHANGELOG.md

+  conversions to the needed type through `TryInto`. This attribute can be used on any 
+  Rust channel, but will only emit setters on nightly when `#![feature(try_from)]` is 
+  declared in the consuming crate's root; this will change when Rust issue 
+  [#33417](https://github.com/rust-lang/rust/issues/33417) is resolved.


nice summary! 👍

colin-kiegel · 2017-04-11T17:15:50Z

derive_builder/src/lib.rs

@@ -366,6 +391,9 @@
 //! - If derive_builder depends on your crate, and vice versa, then a cyclic
 //!   dependency would occur. To break it you could try to depend on the
 //!   [`derive_builder_core`] crate instead.
+//! - The `try_setter` attribute and `owned` builder pattern are not compatible in practice;
+//!   an error during building will consume the builder, making it impossible to continue
+//!   construction.


nice catch :-)

colin-kiegel · 2017-04-11T17:21:18Z

derive_builder/src/lib.rs

+//! }
+//! ```
+//!
+//! A complete example of using fallible setters is available in `examples/try_setter.rs`.


please add a note that the name and visibility are inherited from the setter settings.

You could change the example into something like this, to better explain it?

//! # #[macro_use] //! # extern crate derive_builder; //! # //! #[derive(Builder, Debug, PartialEq)] //! struct Lorem { //! #[builder(try_setter, setter(name="foo"))] //! pub ipsum: u8, //! }

colin-kiegel · 2017-04-11T17:38:06Z

There's one more issue I need to address, which is controlling the generation of these based on the presence of the try_from language feature. There appear to be three possible ways to accomplish this ...

I think all three variants don't work, because I ran into exactly the same problem with no_std support in #45. TL;DR: I decided to leave it to the user of derive_builder to manually set #[builder(no_std)]. It was not possible to detect whether the user set #![no_std]. I suggest to do the same here. :-)

…ning

colin-kiegel

can you revive the tests you had in the old version of derive_builder_test/tests/try_setter.rs?

PS: Are you developing on Linux? I can commit some local shell scripts that I use to emulate what travis is doing. I find them really useful to test all the different combinations nightly with features, stable / beta without..

colin-kiegel · 2017-04-11T19:09:21Z

derive_builder/CHANGELOG.md

-  declared in the consuming crate's root; this will change when Rust issue 
+  normal field setters and allow callers to pass in values which have 
+  fallible conversions to the needed type through `TryInto`. This attribute 
+  can only be used on nightly when until `#![feature(try_from)]` is declared 


when until -> when

TedDriggs · 2017-04-11T19:58:26Z

can you revive the tests you had in the old version of derive_builder_test/tests/try_setter.rs?

Done, and added a more complete Ipsum to get coverage on renaming.

PS: Are you developing on Linux? I can commit some local shell scripts that I use to emulate what travis is doing. I find them really useful to test all the different combinations nightly with features, stable / beta without..

OS X; I can try the scripts to see if they'll work for me.

colin-kiegel · 2017-04-11T20:37:38Z

ok, I pushed a devtools branch. There is a dev folder and a README.md which hopefully explains everything. Let me know if that works for you - if it does, I'll merge it to master.

colin-kiegel · 2017-04-11T20:47:30Z

PS: Note running the tests might take a little while, because this will call rustup update for each toolchain. It buffers all writes to stdout/stderr and only prints it on failure. A successful run should look like this:

II: Working directory is... ✓ clean
II: Running tests on stable... ✓
II: Running tests on beta... ✓
II: Running tests on nightly... ✓
II: Running dev/compiletests.sh... ✓
II: Running dev/checkfeatures.sh... ✓
OK: All checks passed!

TedDriggs · 2017-04-11T22:56:06Z

@colin-kiegel I've pulled down that branch, and found one an issue. It appears OS X doesn't support readlink -f, which is causing a bunch of failures. I'm far from a bash expert, but I got it working with this:

function base_dir {
  if [ uname -s != "Darwin" ] -a hash readlink 2>/dev/null; then
    # use readlink, if installed, to follow symlinks
    local __DIR="$(dirname "$(readlink -f "$0")")"
  else
    local __DIR="$(dirname "$0")"
  fi
  echo ${__DIR}
}

OS X appears to reliably return Darwin for the uname, which was convenient. After I made that change, running dev/githook.sh started passing all tests, but when they ran as part of the push process they failed saying they couldn't find the derive_builder_test directory. That's probably related to my edit, but I don't know any Bash so I ran out of ability to diagnose after that.

That example seems to be causing many/all of the failures.

colin-kiegel · 2017-04-11T23:25:44Z

The basedir() function is supposed to get the directory of the script independently of how it was called. It may seem like overkill to follow symlinks, but actually that's what went wrong, because .git/hooks/pre-push is a symlink. Without following symlinks, the directory of the symlink will be reported by basedir().

To workaround this, you can remove the symlink in the .git directory and replace it with a wrapper bash script .git/hooks/pre-push

#!/bin/bash
../../dev/githook.sh

Or search for a way how to follow symlinks on OS X.

colin-kiegel · 2017-04-11T23:28:46Z

It's great that this was the only issue! :-)

Note the skeptic and compile tests sometimes have linking problems, when switching branches, updating dependencies etc. In that case just run cargo clean.

TedDriggs · 2017-04-11T23:35:33Z

Okay, so this is interesting... the one that fails says TryFrom doesn't have an associated type Err, then later complains that the associated type Err is missing. Not sure how to interpret that issue.

Update: Got bitten by nightly's instability; looks like the associated type got changed to Error a couple weeks ago (PR is here). My hooks are now in a weird state because the devtools aren't part of master; once they are, I'll go ahead and integrate them into my local push workflow.

rust-lang/rust#40281

colin-kiegel · 2017-04-11T23:50:53Z

I'd cargo expand a simple example and copy that into another temporary example file and debug from there.

TedDriggs · 2017-04-11T23:52:26Z

Or once I make that change everything will work :)

Added - try_setters, e.g. `#[builder(try_setter)]`. These setters are exposed alongside the normal field setters and allow callers to pass in values which have fallible conversions to the needed type through `TryInto`. This attribute can only be used on nightly when `#![feature(try_from)]` is declared in the consuming crate's root; this will change when Rust issue [#33417](rust-lang/rust#33417) is resolved.

TedDriggs added 5 commits April 10, 2017 11:47

Added attribute and hooks in derive_builder

fe38226

Removed unused feature flag

4cec554

Enabled crate-level control over try_setter activation

85f4d5a

Fixed a dead_code warning in the try_setter test

0b59e93

colin-kiegel requested changes Apr 11, 2017

View reviewed changes

colin-kiegel reviewed Apr 11, 2017

View reviewed changes

colin-kiegel mentioned this pull request Apr 11, 2017

Idea: Let the builder wrap a Result<T,Error> #25

Closed

TedDriggs added 2 commits April 11, 2017 08:29

Incorporated requested source changes

a56e870

* Renamed try_setter_enabled and try_enabled to `try_setter` * Removed excess #deprecation_notes * Removed `|| self.try_enabled`

Tests and documentation

6d497bc

Updated integration tests

24d7eb4

TedDriggs commented Apr 11, 2017

View reviewed changes

colin-kiegel reviewed Apr 11, 2017

View reviewed changes

TedDriggs added 4 commits April 11, 2017 10:40

Fixed recursion limit for tests in derive_builder_core

79491a5

Updated docs and changelog to reflect lack of lightup

fd7deeb

Moved try_setter integration test out of compiletests

79e4474

Trying to fix tests again by blocking the try_setter example from run…

0600c05

…ning

colin-kiegel reviewed Apr 11, 2017

View reviewed changes

Revived integration tests

1510419

Merged with master

22f49f0

Removing the try_setter example outright.

ec3cc06

That example seems to be causing many/all of the failures.

Made the entire try_setter test dependent on nightly.

12daa64

TryFrom's associated type was changed from Err to Error

c629cbd

rust-lang/rust#40281

colin-kiegel merged commit c629cbd into colin-kiegel:master Apr 12, 2017

TedDriggs deleted the try_setter branch April 12, 2017 15:01

TedDriggs mentioned this pull request Apr 12, 2017

Implement TryFrom<Builder> for Target for all builders exposing a public build method #76

Closed

colin-kiegel mentioned this pull request Apr 15, 2017

once-only setter: prevent overriding existing value #81

Closed

This was referenced Apr 17, 2017

Added macOS support for pre-push tests #83

Merged

Added TryFrom impl generation for target types behind feature flag #77

Closed

Add try_setter option and generate appropriate methods #72

Add try_setter option and generate appropriate methods #72

Conversation

TedDriggs commented Apr 10, 2017 • edited Loading

Open Questions

colin-kiegel commented Apr 10, 2017 • edited Loading

colin-kiegel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TedDriggs Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-kiegel commented Apr 11, 2017

colin-kiegel commented Apr 11, 2017

TedDriggs commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-kiegel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-kiegel commented Apr 11, 2017

colin-kiegel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TedDriggs commented Apr 11, 2017

colin-kiegel commented Apr 11, 2017

colin-kiegel commented Apr 11, 2017

TedDriggs commented Apr 11, 2017

colin-kiegel commented Apr 11, 2017 • edited Loading

colin-kiegel commented Apr 11, 2017

TedDriggs commented Apr 11, 2017 • edited Loading

colin-kiegel commented Apr 11, 2017

TedDriggs commented Apr 11, 2017

TedDriggs commented Apr 10, 2017 •

edited

Loading

colin-kiegel commented Apr 10, 2017 •

edited

Loading

colin-kiegel left a comment •

edited

Loading

TedDriggs Apr 11, 2017 •

edited

Loading

colin-kiegel commented Apr 11, 2017 •

edited

Loading

TedDriggs commented Apr 11, 2017 •

edited

Loading