Repository: BrynCooke/buildstructor (language: rust)

Issue:
`Self` not supported in fallible builders (as return type of constructor)
Hi,

First of all, thanks for building `buildstructor`!

I like the chosen approach quite a lot.

I noticed, that the `Self` type alias isn't supported, when defining a fallible builder:

```rust

// Works:
pub struct Foo {
    foo: (),
}

#[buildstructor::builder]
impl Foo {
    pub fn new(foo: ()) -> Self { // <- `Self`
        Self { foo }
    }
}

// Doesn't work:
pub struct Bar {
    bar: (),
}

#[buildstructor::builder]
impl Bar {
    pub fn new(bar: ()) -> Result<Self, ()> { // <- `Self`
        Ok(Self { bar })
    }
}

// Works:
pub struct Baz {
    baz: (),
}

#[buildstructor::builder]
impl Baz {
    pub fn new(baz: ()) -> Result<Baz, ()> { // <- no `Self`
        Ok(Self { baz })
    }
}
```

The resulting error is as follows:

```
error[E0308]: mismatched types
  --> my_crate/main.rs:30:1
   |
30 | #[buildstructor::builder]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `__BarBuilder`, found struct `rustdoc::Bar`
31 | impl Bar {
32 |     pub fn new(bar: ()) -> Result<Self, ()> {
   |                            ---------------- expected `Result<__BarBuilder<(__P0,)>, ()>` because of return type
   |
   = note: expected enum `Result<__BarBuilder<(__P0,)>, _>`
              found enum `Result<rustdoc::Bar, _>`
   = note: this error originates in the attribute macro `buildstructor::builder` (in Nightly builds, run with -Z macro-backtrace for more info)
```

...which is a bit confusing, to be honest... ;)


Discussion / hints:
Hi. 
Thanks for the detailed bug report!

I'll get round to this in the next couple of weeks.

Not sure what can be done about error messages, the generated code is not pleasant, and only testing and time is going to surface issues. Hopefully I'll get some time to improve things over the coming months.


Sounds good. Thank you, @BrynCooke.

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index fb5c323..b0a4efa 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,13 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## 0.1.11 - 2022-05-06
+[#39](https://github.com/BrynCooke/buildstructor/issues/39)
+Visibility of builder now matches the visibility of the constructor.
+
+[#28](https://github.com/BrynCooke/buildstructor/issues/28)
+Generalize replacing of self in return type.
+
 ## 0.1.10 - 2022-05-04
 [#30](https://github.com/BrynCooke/buildstructor/issues/30)
 The original token stream is output if there are compile errors.
diff --git a/README.md b/README.md
index 4ffa9d5..5b7bc72 100644
--- a/README.md
+++ b/README.md
@@ -249,7 +249,7 @@ If your type does not conform to these patterns then you can use a type alias to
 
 #### Naming
 
-Use the plural form in your constructor argument and `buildstructor` will automatically try to figure out the singular form for individual entry. For isntance:
+Use the plural form in your constructor argument and `buildstructor` will automatically try to figure out the singular form for individual entry. For instance:
 
 `addresses` => `address`
 
diff --git a/src/buildstructor/codegen.rs b/src/buildstructor/codegen.rs
index c80c736..d33bc85 100644
--- a/src/buildstructor/codegen.rs
+++ b/src/buildstructor/codegen.rs
@@ -73,7 +73,7 @@ pub fn codegen(ir: Ir) -> Result<TokenStream> {
                 }
             }
 
-           pub struct __Required<T> {
+            pub struct __Required<T> {
                 _phantom: std::marker::PhantomData<T>,
             }
             pub struct __Optional<T> {
diff --git a/src/buildstructor/lower.rs b/src/buildstructor/lower.rs
index 3218c35..b63c768 100644
--- a/src/buildstructor/lower.rs
+++ b/src/buildstructor/lower.rs
@@ -5,8 +5,8 @@ use quote::{format_ident, quote};
 use std::default::Default;
 use syn::punctuated::Punctuated;
 use syn::{
-    Expr, ExprField, FnArg, GenericArgument, GenericParam, Generics, Index, Member, Pat, Result,
-    ReturnType, Type, TypeParam, TypeTuple, VisRestricted, Visibility,
+    Expr, ExprField, FnArg, GenericArgument, GenericParam, Generics, Index, Member, Pat,
+    PathArguments, Result, ReturnType, Type, TypeParam, TypeTuple, VisRestricted, Visibility,
 };
 use try_match::try_match;
 
@@ -68,7 +68,7 @@ pub fn lower(model: ConstrutorModel) -> Result<Ir> {
         builder_method_name: builder_method_name(&model),
         builder_fields: builder_fields(&model),
         constructor_name: format_ident!("{}Constructor", model.ident.to_string()),
-        return_type: builder_return_type(model.output, model.ident),
+        return_type: builder_return_type(model.output, &model.ident),
         is_async: model.is_async,
         generics: model.generics,
         builder_generics: Ir::builder_generics(),
@@ -89,16 +89,32 @@ fn builder_vilibility(vis: &Visibility) -> Visibility {
     }
 }
 
-fn builder_return_type(mut return_type: ReturnType, target: Ident) -> ReturnType {
+fn builder_return_type(mut return_type: ReturnType, target: &Ident) -> ReturnType {
     if let ReturnType::Type(_, ty) = &mut return_type {
-        let self_type = Box::new(Type::Path(format_ident!("Self").to_type_path()));
-        if ty == &self_type {
-            *ty = Box::new(Type::Path(target.to_type_path()));
-        }
+        replace_self(ty, target);
     }
     return_type
 }
 
+fn replace_self(ty: &mut Type, target: &Ident) {
+    let self_type = format_ident!("Self").to_type_path();
+    if let Type::Path(path) = ty {
+        if path == &self_type {
+            *path = target.to_type_path();
+        } else {
+            for segment in path.path.segments.iter_mut() {
+                if let PathArguments::AngleBracketed(args) = &mut segment.arguments {
+                    for mut arg in args.args.iter_mut() {
+                        if let GenericArgument::Type(ty) = &mut arg {
+                            replace_self(ty, target);
+                        }
+                    }
+                }
+            }
+        }
+    }
+}
+
 fn builder_fields(model: &ConstrutorModel) -> Vec<BuilderField> {
     model
         .args


Test patch:
diff --git a/tests/buildstructor/pass/fallible.rs b/tests/buildstructor/pass/fallible.rs
index b6aa5f6..46e94b8 100644
--- a/tests/buildstructor/pass/fallible.rs
+++ b/tests/buildstructor/pass/fallible.rs
@@ -8,8 +8,16 @@ impl Foo {
     fn new(simple: usize) -> Result<Foo, String> {
         Ok(Self { simple })
     }
+    fn self_new(simple: usize) -> Result<Self, String> {
+        Ok(Self { simple })
+    }
+    fn deep_self_new(simple: usize) -> Result<Result<Self, String>, String> {
+        Ok(Ok(Self { simple }))
+    }
 }
 
 fn main() {
     let _ = Foo::builder().simple(2).build().is_ok();
+    let _ = Foo::self_builder().simple(2).build().is_ok();
+    let _ = Foo::deep_self_builder().simple(2).build().is_ok();
 }