Fix question mark lint.

[vortexofdoom]

Along with a warning about unused field here

gdext/godot-macros/src/class/godot_api.rs

Line 59 in c006a09

Const(AttributeValue),

CI failed when I ran it locally on everything. I don't know where I should put the annotation, or if the field should be removed, so I left that out for now.

As for the other, after fixing the ? lint on the check, I felt that the bindings and extra lines in the changed functions were redundant and didn't really help readability, so I changed both to be what I thought was a little clearer. I thought about including this in my last one but it felt unrelated enough. Sorry if I'm spamming them.

I also got a lot of warnings suggesting addr_of! instead of a mutable static, but I think that will be covered by #581

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-592

[Bromeon]

Thanks for the cleanup!

How exactly did you run local lints? We should find out why they are not triggered in GitHub's CI.

---

As for the other, after fixing the ? lint on the check, I felt that the bindings and extra lines in the changed functions were redundant and didn't really help readability

I disagree -- I would avoid ? inside Some, Ok or Err expressions, as it makes it very hard to see that there's a hidden return path. In

Some(self.peek()?.as_ident().is_ok())

, the self.peek()? should get its own variable on a separate line.

[vortexofdoom]

I literally just ran ./check.sh in the terminal.

I'm also gonna annotate line 58 of godot_api.rs, the field is used, but never read for the moment.

[Bromeon]

I literally just ran ./check.sh in the terminal.

What does rustc --version print for you?

I'm asking because the clippy invocation in check.sh and in CI should be very close:

gdext/check.sh

Lines 130 to 138 in c006a09

	run cargo clippy --all-targets "${extraCargoArgs[@]}" -- \
	-D clippy::suspicious \
	-D clippy::style \
	-D clippy::complexity \
	-D clippy::perf \
	-D clippy::dbg_macro \
	-D clippy::todo \
	-D clippy::unimplemented \
	-D warnings

gdext/.github/workflows/full-ci.yml

Lines 89 to 97 in c006a09

	cargo clippy --all-targets $GDEXT_FEATURES -- \
	-D clippy::suspicious \
	-D clippy::style \
	-D clippy::complexity \
	-D clippy::perf \
	-D clippy::dbg_macro \
	-D clippy::todo \
	-D clippy::unimplemented \
	-D warnings

The check.sh one uses --no-default-features in Cargo, but if anything that should detect less problems, not more.

[vortexofdoom]

Oh I just realized I default to nightly. I can override that for the future.

[Bromeon]

Only the inner AttributeValue is unused, there are Const(_) match arms.
Can you apply the attribute to the nested type?

[vortexofdoom]

Also, I think some of the clippy stuff is only running on changed files, which is how GitHub caught the &Vec<T> vs &[T] signature on my first PR last time, despite the fact that I didn't change the signature of the function.

[vortexofdoom]

Actually, I think the documentation of next_ident and try_next_ident is at least incomplete, if not incorrect.

next_ident takes the next element unconditionally, and Errors if it's not an identifier.

try_next_ident will only take the next element if it's an identifier, will return an error from the peek otherwise.

Neither documentation reflects this behavior accurately.

Hypothetically, changing the latter to this:

pub fn try_next_ident(&mut self) -> Option<Ident> {
        let kv = self.peek()?;

        kv.as_ident().ok().map(|id| {
            _ = self.pop_next();
            id
        })
    }

would be the more accurate semantics if you never want it to Error, which is what the documentation implies. But it is used as written (returning an error without consuming the next pair.)

[vortexofdoom]

We might not need try_next_ident at all, we can just check is_next_ident and return Err/next_ident().unwrap() based on it.

Or we could just add an is_ident function to KvValue that removes the need for an Option<bool>.

Actually it already doesn't need to be an Option. We only care if it's false, so we can just return false in the single case we care about and true otherwise. It would need a different name though.

The transpose I put in got me thinking... I think most of the difficulty around these functions as discussed in #484 could be subverted if they returned Option<Result<T>> instead of Result<Option<T>>. They literally never return Err when it's None so if None is an error, it can be handled in the code that calls it without needing to be wrapped in an Ok

[Bromeon]

We might not need try_next_ident at all, we can just check is_next_ident and return Err/next_ident().unwrap() based on it.

Or we could just add an is_ident function to KvValue that removes the need for an Option<bool>.

Actually it already doesn't need to be an Option. We only care if it's false, so we can just return false in the single case we care about and true otherwise. It would need a different name though.

A lot of this grew historically, in the sense that whenever some new parsing feature was needed, a new method was added without consolidating the existing API. So I can imagine that try_next_ident was there before is_next_ident.

If you see a straightforward path to improve this, feel free 🙂 also in terms of docs. I'm not yet sure what features we're going to need in the future, so requirements might change again.

---

The transpose I put in got me thinking... I think most of the difficulty around these functions as discussed in #484 could be subverted if they returned Option<Result<T>> instead of Result<Option<T>>. They literally never return Err when it's None so if None is an error, it can be handled in the code that calls it without needing to be wrapped in an Ok

Did you mean #484 with export ranges?

Regarding KvParser, the idea of having ParseResult as the outer type is to uniformly handle parsing errors with ?. But I'm not sure which exact functions you refer to.

[Bromeon]

Btw, I'm currently working on #539 and refactor #[class(init)] in the process, so if you want to change something in KvParser, we should align to avoid conflicts, or do it a bit later...

[StatisMike]

We might not need try_next_ident at all, we can just check is_next_ident and return Err/next_ident().unwrap() based on it.

IMO: For the vast majority of our macros we tend to look solely for identifiers, so peeking to check if the next is Ident, then taking the step back and consuming it afterward seems like an overshoot - also a cognitive one. More often than not the situation above should lead to an error.

Or we could just add an is_ident function to KvValue that removes the need for an Option<bool>.

KvValue::as_ident provides similar functionality - as in is_next_ident impl. KvValue::is_ident addition to cut KvParser methods would need exposing it further, which I think should be avoided - we could just expose peek() on the higher level, but I'm not really sure it would help readability and further code quality, as it could be then used in different levels of abstraction.

We only care if it's false, so we can just return false in the single case we care about and true otherwise. It would need a different name though.

To cut Option<bool>, the is_next_ident could be maybe streamlined to something like is_end_or_identifier()? Though we haven't yet stumbled upon another usage for a more general check with the former, it is possible that the latter could be even less useful (even if has a more straightforward return type), which could lead to the introduction of new specialized methods in the future.

[vortexofdoom]

I've been giving it a go, removed the function that was hacked in purely for that commit, I AM inverting the return types, but since some of the other parsers return straight tokens and we care more about the state of this one, I actually think the semantics might be a little clearer this way.

There's a little more indentation in next_allowed_ident than I'd prefer, but I think pretty much everything else is fairly intelligible.

I didn't change anything in KvParser but most of the stuff I changed is at least pretty readable. Open to suggestions.

Actually, I won't commit it at all until I know whether I should have even been working on these. Only changed
field_export.rs, field_var.rs, and list_parser.rs. I'm suddenly not convinced I executed it quite correctly anyway.

Honestly, exposing peek should be fine: if parser.peek().is_some_and(|kv| kv.as_ident().is_err()) reads very naturally to me.

[Bromeon]

Ah yeah, the ListParser -- I was never fully happy about it. Quite a bit of duplication but I guess it does the job.

You could add those changes as a separate commit, then it's possible to inspect them in isolation 🙂

[vortexofdoom]

I did have to expose peek, I realized, so some of it might have been for nothing.

THAT SAID, I didn't run into any errors when I was consuming the next identifier unwittingly. So an option was just getting skipped and we aren't testing for that, since all the remaining ones were valid.

[Bromeon]

Thanks for the update!

To be honest, I find the new notation more confusing than before.

        while let Some(option) = parser.next_any_ident(&ALLOWED_OPTIONS[..])? {
            options.insert(option.to_string());

        while let Some(res) = parser.next_allowed_ident(&ALLOWED_OPTIONS[..]) {
            let option = res?.to_string();
            options.insert(option);

or:

            while let Some(flag) = parser.next_ident()? {
                flags.push(flag)

            while let Some(flag_res) = parser.next_ident() {
                flags.push(flag_res?)

The inner ? expresses less clearly that the operation can fail, and is also less idiomatic in the context where each parser function is followed by the ?. For example, it's now asymmetrical with the entire API of KvParser.

What was the main rationale? Avoiding the transpose()? Because overall, I don't see significant readability gains -- some places are slightly more concise, some more verbose.

[vortexofdoom]

The inner ? expresses less clearly that the operation can fail, and is also less idiomatic in the context where each parser function is followed by the ?. For example, it's now asymmetrical with the entire API of KvParser.

What was the main rationale? Avoiding the transpose()? Because overall, I don't see significant readability gains -- some places are slightly more concise, some more verbose.

Honestly most of it was to clean up the list_parser, a lot of the awkward functions were a direct result of None NEVER constituting an error in that context, that an error is only possible if there are any tokens left, which I don't think is unclear with the rewrite. Looking at KvParser I feel similarly about it. You can kind of make an argument either way, I just think with the iterator-ish semantics that Option<Result> tends to lend itself more to ergonomics than Result<Option>.

In fact, the inversion is very weird here:

gdext/godot-macros/src/util/kv_parser.rs

Lines 34 to 37 in c006a09

	match Self::parse(attributes, expected) {
	Ok(Some(result)) => Ok(result),
	Ok(None) => bail!(context, "expected attribute #[{expected}], but not present",),
	Err(e) => Err(e),

If parse returned an Option<Result> that could be:

Self::parse(attributes, expected)
    .unwrap_or(bail!(context, "expected attribute #[{expected}], but not present",))

But instead we have to deal with an Ok(None) despite None explicitly not being Ok, and it's ignored in the only other place parse is used.

Frankly the whole thing could be written as an iterator over Result<T>, but you said you were refactoring the rest already, so that'd be later anyway. This was mostly precipitated by me seeing how everything was used while trying to figure out the intended behavior vs the documentation.

[vortexofdoom]

Sorry, this ended up being WAY more of a rabbit hole than I planned on when I said to myself "hey those docs look weird"

[Bromeon]

a lot of the awkward functions were a direct result of None NEVER constituting an error in that context, that an error is only possible if there are any tokens left, which I don't think is unclear with the rewrite.

This is not generally true; KvParser even has explicit functions handle_ident_required for situations where None does constitute an error.

---

I look at it from this perspective: these functions could all panic. The result would be the same, but it's less user-friendly because error message and span aren't propagated nicely. But we would then have signatures like:

impl KvParser {
    /// is key present
    fn handle_alone(&mut self, key: &str) -> bool;

    /// expect key's value as ident
    fn handle_ident(&mut self, key: &str) -> Option<Ident>;

    ...
}

Now due to ergonomics, we want to propagate the error -- so I wrap it in Result. This is symmetric with the above:

impl KvParser {
    /// is key present
    fn handle_alone(&mut self, key: &str) -> ParseResult<bool>;

    /// expect key's value as ident
    fn handle_ident(&mut self, key: &str) -> ParseResult<Option<Ident>>;

    ...
}

The return type is always consistent, whether the inner type is Option, bool or whatever.

---

In fact, the inversion is very weird here:

The semantics of parse are:

• Ok if the parsing was successful (whether something was found or not)
• Err if not

The inversion is a direct consequence of parse_required having different semantics: it is now also an error if parsing succeeds but the thing is absent.

---

Frankly the whole thing could be written as an iterator over Result<T>

Could you elaborate? You mean ListParser in particular?

[vortexofdoom]

This is not generally true; KvParser even has explicit functions handle_ident_required for situations where None does constitute an error.

I meant in ListParser more specifically. But Overall, I like having the Option on the outside

I look at it from this perspective: these functions could all panic. The result would be the same, but it's less user-friendly because error message and span aren't propagated nicely.

I think the other way around actually can result in better span, because at the outer level you have more info on what's been tried and what the high-level goal is. You're never getting rid of the error, you're just deferring a decision on how to handle it, so you can use the inner span, or combine it with already parsed tokens to make a better message. I don't find the single-nesting of a ? inside a if let Some(_) block to obfuscate the possibility of early return (although I totally agree with your comment about early return inside a Some, that is something I'm taking to heart going forward,) I actually think that having it on the outer level can be not only easier to miss, but can also make it easier to forget to deal with the None case when it's relevant, since "the error has been handled."

The semantics of parse are:

* `Ok` if the parsing was successful (whether something was found or not)

* `Err` if not

The inversion is a direct consequence of parse_required having different semantics: it is now also an error if parsing succeeds but the thing is absent.

The way I see it, if nothing was found, no parsing was done. Then you can deal separately with whether you WANT something to have been found and if you did, whether it was what you wanted. Option<Result> lets you handle finding something or not independently of what was found. Sometimes it's fine, sometimes it's not, and I just tend to see Ok(None) as overly opinionated in that regard.

The higher level functions should know better than the simple ones whether a failure to parse is a problem or not, sometimes it is, and it's easy to propogate the error at that point, other times it just means you have to try to parse the next thing.

Frankly the whole thing could be written as an iterator over Result<T>

Could you elaborate? You mean ListParser in particular?

I meant that most parsing is basically a batching iterator, which is why I tend to default to returning Option of whatever I'm looking for. I don't necessarily want to return early by default if there was an error, which is enabled (and occasionally incorrectly (imo) encouraged) by wrapping the Option in Result. A lot of KvParser is basically writing extra functions for the sake of a possible early return from a function several layers above. There's a lot of match blocks that only serve to wrap None in Ok where they could just be a single if let block.

Now the functions that consume that iterator will indeed probably want to return a Result in the end, but internally, it makes sense for a parser to just return an Err if it didn't successfully parse the thing you're looking for, because then you can do what you want with it. It makes more sense to me for handle_ident to return a Result internally, because you tried to parse an Ident and failed. If you want an Option from it, you can just use .ok() on the Result.

impl KvParser {
    /// expect key's value as ident
    fn handle_ident(&mut self, key: &str) -> ParseResult<Ident>;
}

In the case of handle_alone(), parsing "unsuccessfully" isn't even a problem, and it will never return an error as written.
It doesn't make much sense to me for something that basically short-circuits to a check for whether it's Some, to return a Result. If handle_alone_ident returned an Option<Result> it wouldn't even need to exist, it could just be:

handle_alone_ident(key).is_some()

and an explicit early return with .unwrap_or() or when checking is_none() at the top level is easy and clear.

as is it could still be expressed with

handle_alone_ident(key).is_ok_and(|opt| opt.is_some())

But I find those semantics messier and less flexible in the general case.

similarly, expr() is returned as a Result when it can never fail, which actually surprises me because the ? gives the impression that it's a place where the function could return early, even though it never actually can. The user needs to remember that about expr() when reading code elsewhere looking for problems.

[Bromeon]

In the case of handle_alone(), parsing "unsuccessfully" isn't even a problem, and it will never return an error as written.

That's not true for KvParser though: handle_alone fails if the user specifies a value for the key.

gdext/godot-macros/src/util/kv_parser.rs

Lines 83 to 99 in c006a09

	/// Handles a key that can only occur without a value, e.g. `#[attr(toggle)]`. Returns whether
	/// the key is present.
	pub fn handle_alone(&mut self, key: &str) -> ParseResult<bool> {
	self.handle_alone_ident(key).map(|id| id.is_some())
	}
	/// Handles a key that can only occur without a value, e.g. `#[attr(toggle)]`. Returns the key if it is
	/// present.
	pub fn handle_alone_ident(&mut self, key: &str) -> ParseResult<Option<Ident>> {
	match self.handle_any_entry(key) {
	None => Ok(None),
	Some((id, value)) => match value {
	None => Ok(Some(id)),
	Some(value) => bail!(&value.tokens[0], "key `{key}` should not have a value"),
	},
	}
	}

(Btw, I renamed handle_alone_ident to handle_alone_with_span in my refactoring).

---

The way I see it, if nothing was found, no parsing was done. Then you can deal separately with whether you WANT something to have been found and if you did, whether it was what you wanted. Option<Result> lets you handle finding something or not independently of what was found. Sometimes it's fine, sometimes it's not, and I just tend to see Ok(None) as overly opinionated in that regard.

The higher level functions should know better than the simple ones whether a failure to parse is a problem or not, sometimes it is, and it's easy to propogate the error at that point, other times it just means you have to try to parse the next thing.

But that's why there are different methods with different guarantees:

• KvParser::handle_ident() gives you the optional Ident for that key (fails when the key has something else but an ident)
• KvParser::handle_ident_required() gives you the required Ident for that key, or fails.

We can of course keep the interface in KvParser simpler by letting the caller decide what is an error or not, but that pushes more work on the caller (which is written more than once). The whole point of that API is to make these cases expressive and always the same. Seeing handle_ident_required is an immediate signal that I require this Ident. (handle_ident could technically be renamed to handle_ident_optional, but that's besides the point; it's also the more common operation).

To give a bit of background, originally we just worked with the KvValues directly, because this is powerful enough to express everything. But we saw that a lot of patterns repeat, and it's more cumbersome to always have to match on the different scenarios, which is why we made things explicit in the KvParser API: whether I want an ident, a number, an expression, or whatever -- and whether these are required. These methods are deliberately specialized and not general.

So when you say "Then you can deal separately with whether you WANT something to have been found and if you did, whether it was what you wanted." -- this goes against the idea of having a specialized KvParser API. You want to simplify the parser -- I want to simplify the usage of the parser. I don't care if we need a few methods more, if this in turn allows to make call sites more uniform.

(I'm mostly talking about KvParser and not ListParser because I wasn't much involved in the latter's design, so it's possible to clean up some stuff there. Maybe we should consider alternative designs like iterators or so for the latter.)

[vortexofdoom]

Yeah I think a lot of this does just boil down to API preference. I think if I were to go through all of it with a fine-toothed comb I could get something that was both consistent and intuitive across the board while remaining simple at the higher levels, but I definitely understand the value of a consistent API and it's not worth the duplicated effort and risk of miscommunication when you're already refactoring.

I'm very much not opposed to a specialized Parser API, I just think it's confusing as is. Result<Option> vs Option<Result> doesn't result in more complexity, parse_required() as I wrote it was simpler to read and reason about, even if it's wrapped in the same function for ease of use.

I did misread handle_alone but it could still just be handle_alone_ident(key)?.is_some() as is, and I find the intent there clearer without being too much more verbose.

I'll revert the transposing changes but still expose peek() and clean up the docs, I think.

[Bromeon]

Thank you! 🙂

For next time, maybe consider making comments a bit longer. Not important enough to block merging though 😉