Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

self as target doesn't seem to work #25

Closed
Boscop opened this issue Jul 27, 2020 · 9 comments · Fixed by #26
Closed

self as target doesn't seem to work #25

Boscop opened this issue Jul 27, 2020 · 9 comments · Fixed by #26

Comments

@Boscop
Copy link
Collaborator

Boscop commented Jul 27, 2020

I have a derived getter, using the getset crate, but I also need to impl a trait that has the same getter.
This works:

#[derive(Getters)]
pub struct Struct {
	member: Member,
	#[getset(get)]
	foo: Foo,
}

impl Trait for Struct {
	delegate! {
		to self.member {
			fn bar(&self) -> Bar;
		}
	}

	fn foo(&self) -> &Foo {
		self.foo()
	}
}

But this doesn't work:

impl Trait for Struct {
	delegate! {
		to self.member {
			fn bar(&self) -> Bar;
		}

		to self {
			fn foo(&self) -> &Foo;
		//  ^^ error: expected identifier or integer
		}
	}
}

But it should also work, because the handwritten delegation from trait to self.foo() works :)

@Kobzol
Copy link
Owner

Kobzol commented Jul 27, 2020

Hmm, for some reason syn refuses to parse self as an Expr. This is pretty tricky, I'll probably have to do through the tokens one by one and build the target expression manually if I can't just parse it as an expression.

@danielhenrymantilla
Copy link

@Kobzol syn does parse self as an Expr:

The problem here is that it is parsing self { ... } as an expression as a whole (e.g., self { 0: hi } is, syntactically, a valid expression: Demo), and so it commits to start parsing a struct-like expression and fails when it encounters the token fn instead of a field name or a tuple index.

In general, you cannot really parse a braced group right after arbitrary expressions. Try adding some separator (such as , or =>) to disambiguate. For instance:

impl Trait for Struct {
    delegate! {
        {
            fn foo ...
        } => self,
    }

Otherwise you would have to do the parsing of the part before the braces manually (e.g., with Punctuated<IdentOrInteger, Token![.]>::parse_separated_non_empty or something like that, given some enum IdentOrInteger that impls Parse).


@Boscop in the meantime, you can use (self) or {self} to disambiguate 🙂

@Kobzol
Copy link
Owner

Kobzol commented Jul 29, 2020

Ooh, now it actually makes sense, thank you! I'll probably parse it manually, if it won't be too problematic, because I don't want to introduce another syntactic layer.

@Kobzol Kobzol mentioned this issue Aug 14, 2020
@Kobzol
Copy link
Owner

Kobzol commented Aug 14, 2020

So I decided to special case this, because self is pretty much the only bare token that makes sense here (delegating to a global variable or parameters of the delegated method is not currently supported). It works fine for normal cases, however there is some problem with macros generating the delegation tokens.

It seems that rustc inserts None-delimited groups around macro tokens (media-io/yaserde#79), so this test generates something like this:

Group {
                   delimiter: None,
                   stream: TokenStream [
                       Ident {
                           ident: "self",
                           span: #3 bytes(659..671),
                       },
                       Punct {
                           ch: '.',
                           spacing: Alone,
                           span: #3 bytes(659..671),
                       },
                       Literal {
                           kind: Integer,
                           symbol: "0",
                           suffix: None,
                           span: #3 bytes(659..671),
                       },
                   ],
                   span: #3 bytes(659..671),
               },
... rest of input

Before, when I was parsing this as syn::Expr, it worked fine, so I tried to parse it as an Expr and if it doesn't work, switch to self, something like this:

impl syn::parse::Parse for DelegationTarget {
    fn parse(input: ParseStream) -> Result<Self, Error> {
        let cursor = input.cursor().token_stream();
        Ok(if syn::parse2::<syn::Expr>(cursor).is_ok() {
            DelegationTarget::Expr(input.parse()?)
        } else {
            DelegationTarget::SelfTarget(input.parse()?)
        })
    }
}

The problem is that the first condition doesn't succeed. I tracked it down to this:

syn::parse2::<syn::Expr>(input.cursor().token_stream()).is_ok() // false
input.parse::<syn::Expr>.is_ok() // true

I printed the contents of the two token streams and they seem to be the same, so it seems like there is something wrong with my usage of the parse2 function. This is hitting the limits of my syn-fu, as I have no idea why should this return a different result.
Maybe it's caused by this: https://github.com/dtolnay/syn/blob/master/src/parse.rs#L1173? Could it be avoided somehow?
@danielhenrymantilla any ideas? :)

@danielhenrymantilla
Copy link

Sorry for my late answer, I forgot about this thread.

The good thing is the results you are hitting make sense in my head; and I like the idea of using a None-delimited group as a workaround. I'm gonna be using and as sigils to represent the delimiters of None-delimited groups.

The key thing to understand about such ⟨…⟩ groups is:

  • they group the contained tokens as a single one, much like (), {} and [] do,

  • but they don't show up / are invisible to operations such as stringification, hence your:

    I printed the contents of the two token streams and they seem to be the same

So, in your case, your sequence of tokens is:

⟨self . 0⟩ { … }

Then, the difference between:

syn::parse2::<syn::Expr>(input.cursor().token_stream()).is_ok() // false
input.parse::<syn::Expr>().is_ok() // true

is that the former tries to parse all the given tokens as an expression, whereas the latter only tries to parse a prefix of such tokens as one.

And given the capture, ⟨self . 0⟩ is an expression (self.0 is), but <expr> { ... } on the other hand, is not a valid expression (as I mentioned, the ambiguity originated from self being interpreted as a path, which is no longer the case with our ⟨self . 0⟩ captured expression).


The solution

The goot thing about having waited that long to keep this thread updated, is that, in the meantime, I have stumbled upon a bunch of posts from dtolnay where they mention that this is a common case of ambiguity in the language.

Because of that, I have suspected there had to be one function inside syn to disambiguate. And indeed there, which does contain the examples I had seen in such posts. Behold:

Expr::parse_without_eager_brace()

I'm pretty sure this will neatly solve your issues 😉

@Kobzol
Copy link
Owner

Kobzol commented Oct 26, 2020

Thank you! That is exactly what I needed. Actually if I thought of using Expr::parse(input) instead of input.parse<syn::Expr> I would probably find it immediately 🤦‍♂️

This test broke:

macro_rules! some_macro {
    ($delegate_to:expr) => {
        impl Trait for Struct2 {
            delegate! {
                // '$delegate' will expand to 'self.0' before ´delegate!' is expanded.
                to $delegate_to {
                    fn method_to_delegate(&self) -> bool;
                }
            }
        }
    };
}
some_macro! {self.0}

It produces

error[E0424]: expected value, found module `self`
  --> tests/in_macro_expansion.rs:34:14
   |
28 |                     fn method_to_delegate(&self) -> bool;
   |                     -- this function has a `self` parameter, but a macro invocation can only access identifiers it receives from parameters
...
34 | some_macro! {self.0}
   |              ^^^^ `self` value is a keyword only available in methods with a `self` parameter

Initially I thought that it's a regression of the new self parsing, but after bisecting it I realized that Rust 1.46 introduced this change (in macro hygiene?), as it also happens with the old code. Probably caused by this: rust-lang/rust#73293. Not sure if we can work around that easily.

@Boscop can you please check that #26 fixes your issue?

@danielhenrymantilla
Copy link

danielhenrymantilla commented Oct 26, 2020

Indeed, the self that the macro introduces is internal / hidden, so the caller of some_macro! has no way to refer to it / "cannot know what that self refers to" (the whole point of hygiene was to avoid such confusing situations).

A possible fix is to use:

macro_rules! some_macro {
    (|$self:ident| $delegate_to:expr) => {
        impl Trait for Struct2 {
            delegate! {
                // '$delegate' will expand to 'self.0' before ´delegate!' is expanded.
                to $delegate_to {
                    fn method_to_delegate(&$self) -> bool;
                }
            }
        }
    };
}
some_macro! { |self| self.0 }
  • (another option, since I agree that self is kind of special (e.g., within the context of delegate! it is not surprising at all), is to use a VisitMut from syn to map any Ident equal to self to the same ident, but with Span::call_site() (removing any hygiene interactions when dealing with selfs)

@lulivi
Copy link

lulivi commented Nov 16, 2020

Looking forward for the merge of #26 ! 😄

@Kobzol
Copy link
Owner

Kobzol commented Nov 16, 2020

@lulivi Published rust-delegate 0.5.0 with #26.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants