[rustdoc] Fix invalid handling of field followed by negated macro call

[GuillaumeGomez]

This is the bug uncovered in #150022. Once fixed Ill rebuild all compiler docs and see if we can enable the option for compiler docs. =D

It's a weird case where we extracted some tokens out of the iterator and then, when checking next items (from this iterator), it didn't find the : token, and therefore badly assumed the token kind.

The solution I came up with is to instead not extract tokens from the iterator and to count how many tokens are in the current path. So when iterate over the items, instead of having a mix of extracted tokens and tokens still inside the iterator, we now only iterate over the iterator.

The biggest change here is that get_full_ident_path will return an option instead of a Vec, and if it's contains : (one, not two), then it will return None and the : will be handled like any token and not like a path (which is more correct imo).

r? @yotamofek

[GuillaumeGomez]

So in case we don't find the item we're interested in, we don't want the item to impact the "peeked" items position, so I needed to write this function to "put back the item" in case it didn't match the condition.

[yotamofek]

I wonder if it would make sense to switch the implementation of peek_next and peek_next_if around, and then implement peek_next in terms of peek_next_if(|_| true).

[GuillaumeGomez]

Huuuum... I see your point but I tend to prefer to have the "call-less"/simpler function as core and then build on top of it. I think it's the "personal taste" area in here. 😆

[GuillaumeGomez]

Code doesn't need this continue anymore and that makes it much simpler to understand (so yeah, kinda congratulating myself here ^^').

[yotamofek]

This is much better 😁

[GuillaumeGomez]

We don't return in this loop because we need to call stop_peeking before exiting this function. So instead, we break the value to return, call stop_peeking and then actually return. Not super graceful but it works.

[yotamofek]

Yeah, would be nice to be able to just use return but also call stop_peeking() indiscriminately. Oh well... maybe in a future PR.

[GuillaumeGomez]

Well we could but then the whole loop content needs to be in another function. Then it's basically a debate over personal taste. ^^'

[yotamofek]

The change makes sense, and all the tests plus the new one pass, so I'm inclined to just accept it as-is, without diving too deep into this. The highlighting code is quite complex and I'm more reliant on tests than my ability to run the code in my head.

So r+ from me if you want to get this in, we can always make improvements later. If it's not urgent then give me a few more days to spend some quality time with this. Up to you :)

[yotamofek]

Suggested change

	let mut len = 0;
	for _ in 0..nb_items {
	if let Some((_, text, _)) = classifier.next() {
	len += text.len();
	}
	}
	let len: usize = iter::from_fn(|| classifier.next())
	.take(nb_items)
	.map(|(_, text, _)| text.len())
	.sum();

Take it or leave it:

I know you're not a big fan of iterator combinators and the functional style, but I think that apart from being more readable (IMHO, of course), this also has an additional benefit that it'll stop calling classifier.next() after the first None, even if it yields less than nb_items items :)

[GuillaumeGomez]

In this case I like it. ;)

[GuillaumeGomez]

@yotamofek: Gonna go ahead and approve it. I implemented your suggestion for the iterator. I'm looking forward to which improvements you will come up with next. :)

@bors r=yotamofek rollup

[bors]

📌 Commit 2114b21 has been approved by yotamofek

It is now in the queue for this repository.