Closed Bromeon closed 2 years ago
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.
Thanks a lot for the kind words 😊
Just a quick note to say I didn't forget about this PR. Still hoping to merge it and the other one soon.
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.
I'd lean towards doing it once both PRs are merged.
Sounds good to me!
Some feedback:
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)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).Overall, none of these are blockers, so I'm merging the PR now.
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.
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: