Support function receivers self, mut self, &self, &mut self

[Bromeon]

Adds support for receiver parameters self, mut self, &self, &mut self.

Also captures several "non-content" tokens like ( ) parameter list group, : parameter name-value separator, -> return type separator.

This PR contains breaking changes, as it splits FunctionParameter up:

#[derive(Clone, Debug)]
pub enum FunctionParameter {
    Receiver(FunctionReceiverParameter),
    Typed(FunctionTypedParameter),
}

// self, mut self, &self, &mut self
#[derive(Clone, Debug)]
pub struct FunctionReceiverParameter {
    pub attributes: Vec<Attribute>,
    pub tk_ref: Option<Punct>,
    pub tk_mut: Option<Ident>,
    pub tk_self: Ident,
}

// name: type
#[derive(Clone, Debug)]
pub struct FunctionTypedParameter {
    pub attributes: Vec<Attribute>,
    pub tk_mut: Option<Ident>,
    pub name: Ident,
    pub tk_colon: Punct,
    pub ty: TyExpr,
}

[PoignardAzur]

Always a pleasure to get a high-quality PR =)

From a quick read-through, everything looks good. I'll look at it again and (presumably) merge it before the end of the week.

[Bromeon]

Thanks a lot for the kind words 😊

[PoignardAzur]

Just a quick note to say I didn't forget about this PR. Still hoping to merge it and the other one soon.

[Bromeon]

No worries, and thanks for the heads-up!

One thing I was wondering when working on the other PR: would it make sense to use shorter identifiers, since this got quite long with the "Receiver"/"Typed" addition -- also for consistency with other identifiers like GenericParam? Instead of

pub enum FunctionParameter { ... }
pub struct FunctionReceiverParameter { ... }
pub struct FunctionTypedParameter { ... }

there could be

pub enum FnParam { ... }
pub struct FnReceiverParam { ... }
pub struct FnTypedParam{ ... }

If yes, I could do that either here or in a separate follow-up "consistency PR". Other types might have similar points.

[PoignardAzur]

I'd lean towards doing it once both PRs are merged.

[Bromeon]

Sounds good to me!

[PoignardAzur]

Some feedback:

• In the codebase, "consume_xxx" is meant to indicate functions that return an option or a vector that is potentially empty. Functions that panic if they can't find tokens they're looking for should be called "parse_xxx". I'm thinking of consume_fn in particular which should be renamed to parse_fn. (I don't know if this matches common practice, but I'd like to stay consistent)
• You added a use case for functions like fn foobar(mut arg: Foo) (functions with mutable arguments). You should add a unit test for that case (the tests only cover the mut self case).
• When parsing a function, the parser drops the semicolon if there's one. It should store it in the Function type instead of re-creating one in the ToTokens impl.
• FWIW, I tried to stick to a flow in the codebase where dependencies go "up". That is, the "exported" functions are at the bottom, and the function they call are above them. It's not better or worse than the alternative, but I'd like to stay consistent there too.

Overall, none of these are blockers, so I'm merging the PR now.

[Bromeon]

Thanks a lot for the merge and the feedback!

In the codebase, "consume_xxx" is meant to indicate functions that return an option or a vector that is potentially empty. Functions that panic if they can't find tokens they're looking for should be called "parse_xxx". I'm thinking of consume_fn in particular which should be renamed to parse_fn. (I don't know if this matches common practice, but I'd like to stay consistent)

I see, but this is currently not done consistently -- e.g. consume_attributes() or consume_vis_marker() both panic.

---

Slightly off-topic, what do you think about translating those panics into Result<..., venial::Error> and propagate them via ? operator, to have errors messages with spans? I'm aware that venial's declared trade-off is "It doesn't attempt to recover gracefully from errors. If [the input isn't a valid declaration], venial will summarily panic.", so I wanted to ask what your stance here is if readability doesn't suffer much.

To clarify, in my own code (that uses venial), I wrote this little utility:

type ParseResult<T> = Result<T, venial::Error>;

pub fn fail<R, T>(msg: &str, tokens: T) -> ParseResult<R>
where
    T: Spanned,
{
    Err(venial::Error::new_at_span(tokens.__span(), msg))
}

Usage:

fn try_parse(...) -> ParseResult<...> {
    // ...
    match tk {
        TokenTree::Ident(ident) => {
            let key = /* extract stuff, etc */
        }
        _ => fail("attribute must start with key", tk)?,
    }
}

Non-panicking functions would also help the problem mentioned here: when encountering const, I could first try consume_fn, on error fallback to consume_constant and only then cause an error.

Edit: after some more thought, it might actually be quite a bit of effort, since it's not just the direct panic! calls, but also assert!, unwrap(), expect(), operator[] etc.

---

Your other points should be clear, I'll integrate them as a separate commit in the impl PR.