Addressing T19, and adding explicit generic type parameters to delegated methods #35

saresend · 2018-10-02T05:51:08Z

Thanks for being patient with me! The current implementation is quite naive, but effectively what it is doing is simply computing the number of needed type params, and if there is a non-zero quantity then adds them explicitly to the output, and otherwise elides the generic type params.

LukasKalbertodt

Glad to see your PR. Thanks :)

I left a few line comments. And I have a few additional comments:

Could you rename your test to non_inferred_generic_types_for_all_refs (or something like that)? The test names can be very verbose IMO. Then the trait should be implemented for other proxy types as well: #[auto_impl(Arc, Box, Rc, &, &mut)]

Please add another test non_inferred_generic_types_for_box:

#[auto_impl(Box)]
trait Foo {
    fn foo<T>();
    fn bar<U>(&self);
    fn baz<V>(&mut self);
    fn qux<W>(self);
}

With the test above you will notice a thing we forgot: the self receiver case.

If anything is unclear, please let us know. Thanks again ^_^

LukasKalbertodt · 2018-10-02T09:30:29Z

src/gen.rs

@@ -430,6 +430,10 @@ fn gen_method_item(
    // Generate the list of argument used to call the method.
    let args = get_arg_list(sig.decl.inputs.iter())?;

+    // Determines the number of generic parameters, and what they are
+    let type_param_count = sig.decl.generics.type_params().count();


I don't actually think we have to distinguish between 0 or more type parameters. Rust accepts empty parameter lists like foo::<>(). Playground. Unless I'm missing something, I think we can drop this distinction.

I initially just tried adding it as such, but it seems that some field expressions will not accept generic types, meaning that some expansions will fail in that case. Specifically I believe #[auto_impl(Box)] was throwing errors if we don't make this check.

Ah, now I get it. So the problem is that an empty "type generics" from syn is an empty token stream and not <>. This means that we generate self.method::(args) when we don't have generics. That's incorrect syntax, hence the error.

But I found the method TypeGenerics::as_turbofish(). This adds the double-colon if the token tree is not empty. So with that, we don't need that distinction. So what you basically need to do:

Write let generic_types = generic_types.as_turbofish(); in the line below the split_for_impl line.

Remove the :: from the quote!s, like this: quote! { #proxy_ty_param::#name #generic_types(#args) } (because if we don't have generics #generic_types will be an empty token stream, otherwise ::<T ...>)

Then you can remove this count() line and the match-distinctions below.

LukasKalbertodt · 2018-10-02T09:31:27Z

src/gen.rs

@@ -430,6 +430,10 @@ fn gen_method_item(
    // Generate the list of argument used to call the method.
    let args = get_arg_list(sig.decl.inputs.iter())?;

+    // Determines the number of generic parameters, and what they are
+    let type_param_count = sig.decl.generics.type_params().count();
+    let (_, generic_types, _ ) = &sig.decl.generics.split_for_impl();


The space between _ and ) should be removed.

Why is there a & before sig? I don't think that's necessary, so we could remove it.

LukasKalbertodt · 2018-10-02T09:37:48Z

src/gen.rs

+             match type_param_count {
+              0 => quote! { (*self).#name(#args) },
+              _ => quote! { (*self).#name::#generic_types(#args) }
+            }


I was thinking about if we should maybe use the explicit form here: #proxy_ty_param::#name::#generic_types(*self, #args). @KodrAus any opinions about this? I think it is more explicit and it should work.

Hm, being consistent about always calling methods as path(args) whether they're static or instance seems nice to me 👍

@saresend Could you also change this? :)

Yep, will do!

LukasKalbertodt · 2018-10-03T10:40:23Z

tests/compile-pass/non_inferred_generic_types_for_box.rs

+    fn bar<U>(&self);
+    fn baz<V>(&mut self);
+    fn qux<W>(self);
+}


Could you add a newline here? As a general hint: it's usually expected that all files end with a single trailing newline. You should probably configure your editor to automatically insert one. While doing that you should also configure your editor to automatically remove any trailing whitespace (whitespace at the end of a line). That's also something most people don't want in their code. Thanks! :)

LukasKalbertodt · 2018-10-03T10:41:15Z

@saresend Thanks for updating the PR and including another test. That lifetime test was a good idea!

If you address the three line comments that are still up to date right now, I think this PR would be ready to merge :)

saresend · 2018-10-03T18:41:08Z

Hey! A couple of things that I ran into while trying to fix the previous issues. First, would it be possible to provide a rustfmt.toml file? Unfortunately the latest commit I provided was formatted with default rustfmt, and to try to stay in line with your current formatting it would be helpful to have a formatting config specified. Also, when moving from (*self)... to #proxy_ty_param::... , it seems to not resolve to the appropriate type, specifically that it would expect a &T, but provide a Box when trying to implement on multiple types. I was wondering how exactly the proxy_ty_param is generated? Thanks!

LukasKalbertodt · 2018-10-04T13:18:33Z

Hi!

Regarding rustfmt: I'm not a big fan of global automatic formatting. Rustfmt can't get everything right everywhere. While I think it's super useful in some situations (keeping import lists sorted), it's really bad in others. I'm very committed to keeping the Rust standard style, but I disagree with quite a few minor changes done by rustfmt here.

Regarding the self.foo(..) to T::foo(self, ..) transformation: I think this doesn't need to be done in this PR as it doesn't really have anything to do with the issue this PR fixes. So this can be done later.

So I think your PR is feature complete, but the rustfmt formatting should be removed. If you don't mind, I would just do that. I think this cleanup is only boring work, so I can quickly do everything I would like to see. If you enabled the "allow modifications from maintainers" box, I can just push a new commit to your branch (to be included in this PR). Then I can merge it.

And if you don't mind, I would squash all your commits when merging.

LukasKalbertodt · 2018-10-04T13:37:33Z

Okay, update: I just created that commit and already pushed it. That way I can merge this now (otherwise async communication is too slow :P).

Travis will not succeed, but this is not your fault and it will be fixed in a later PR.

Thanks again for your PR :)
Consider taking a look at the gitter channel if you want to tackle another issue. That way I know which issues I can tackle.

saresend · 2018-10-04T18:39:43Z

Hey, thanks so much for being so helpful, I really appreciate it!

saresend added 5 commits October 1, 2018 20:43

adding const ref generic test

6adb8a1

fixing T19, and adding generic types on method delegation

58feffb

removing tmp files

6391ac8

cleaning up comments and printlns

ae8708f

whitespace

144f32b

LukasKalbertodt requested changes Oct 2, 2018

View reviewed changes

addressing issues and adding tests

cfa0d99

LukasKalbertodt reviewed Oct 3, 2018

View reviewed changes

adding turbofish conversion, and fixing some whitespace issues

ba3af03

Revert most rustfmt formatting from previous commit

5cc2b42

LukasKalbertodt approved these changes Oct 4, 2018

View reviewed changes

LukasKalbertodt merged commit ce3ff8d into auto-impl-rs:master Oct 4, 2018

This was referenced Oct 4, 2018

Improve Travis-CI config, fix warning and update to newest nightly #34

Merged

Explicitly pass type parameters when delegating the method #19

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addressing T19, and adding explicit generic type parameters to delegated methods #35

Addressing T19, and adding explicit generic type parameters to delegated methods #35

saresend commented Oct 2, 2018

LukasKalbertodt left a comment

LukasKalbertodt Oct 2, 2018

saresend Oct 2, 2018 •

edited

Loading

LukasKalbertodt Oct 3, 2018

LukasKalbertodt Oct 2, 2018

LukasKalbertodt Oct 2, 2018

KodrAus Oct 3, 2018 •

edited

Loading

LukasKalbertodt Oct 3, 2018

saresend Oct 3, 2018

LukasKalbertodt Oct 3, 2018

LukasKalbertodt commented Oct 3, 2018 •

edited

Loading

saresend commented Oct 3, 2018

LukasKalbertodt commented Oct 4, 2018

LukasKalbertodt commented Oct 4, 2018

saresend commented Oct 4, 2018

Addressing T19, and adding explicit generic type parameters to delegated methods #35

Addressing T19, and adding explicit generic type parameters to delegated methods #35

Conversation

saresend commented Oct 2, 2018

LukasKalbertodt left a comment

Choose a reason for hiding this comment

LukasKalbertodt Oct 2, 2018

Choose a reason for hiding this comment

saresend Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

LukasKalbertodt Oct 3, 2018

Choose a reason for hiding this comment

LukasKalbertodt Oct 2, 2018

Choose a reason for hiding this comment

LukasKalbertodt Oct 2, 2018

Choose a reason for hiding this comment

KodrAus Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

LukasKalbertodt Oct 3, 2018

Choose a reason for hiding this comment

saresend Oct 3, 2018

Choose a reason for hiding this comment

LukasKalbertodt Oct 3, 2018

Choose a reason for hiding this comment

LukasKalbertodt commented Oct 3, 2018 • edited Loading

saresend commented Oct 3, 2018

LukasKalbertodt commented Oct 4, 2018

LukasKalbertodt commented Oct 4, 2018

saresend commented Oct 4, 2018

saresend Oct 2, 2018 •

edited

Loading

KodrAus Oct 3, 2018 •

edited

Loading

LukasKalbertodt commented Oct 3, 2018 •

edited

Loading