-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[rustdoc] Fix invalid handling of field followed by negated macro call #150099
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -832,6 +832,20 @@ impl<'a> PeekIter<'a> { | |||||||||||||||||||||
| .copied() | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| fn peek_next_if<F: Fn((TokenKind, &'a str)) -> bool>( | ||||||||||||||||||||||
| &mut self, | ||||||||||||||||||||||
| f: F, | ||||||||||||||||||||||
| ) -> Option<(TokenKind, &'a str)> { | ||||||||||||||||||||||
| let next = self.peek_next()?; | ||||||||||||||||||||||
| if f(next) { | ||||||||||||||||||||||
| Some(next) | ||||||||||||||||||||||
| } else { | ||||||||||||||||||||||
| // We go one step back. | ||||||||||||||||||||||
| self.peek_pos -= 1; | ||||||||||||||||||||||
| None | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| fn stop_peeking(&mut self) { | ||||||||||||||||||||||
| self.peek_pos = 0; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
@@ -903,18 +917,19 @@ fn classify<'src>( | |||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if let Some((TokenKind::Colon | TokenKind::Ident, _)) = classifier.tokens.peek() { | ||||||||||||||||||||||
| let tokens = classifier.get_full_ident_path(); | ||||||||||||||||||||||
| for &(token, start, end) in &tokens { | ||||||||||||||||||||||
| let text = &classifier.src[start..end]; | ||||||||||||||||||||||
| classifier.advance(token, text, sink, start as u32); | ||||||||||||||||||||||
| classifier.byte_pos += text.len() as u32; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if !tokens.is_empty() { | ||||||||||||||||||||||
| continue; | ||||||||||||||||||||||
| if let Some((TokenKind::Colon | TokenKind::Ident, _)) = classifier.tokens.peek() | ||||||||||||||||||||||
| && let Some(nb_items) = classifier.get_full_ident_path() | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| let start = classifier.byte_pos as usize; | ||||||||||||||||||||||
| let mut len = 0; | ||||||||||||||||||||||
| for _ in 0..nb_items { | ||||||||||||||||||||||
| if let Some((_, text, _)) = classifier.next() { | ||||||||||||||||||||||
| len += text.len(); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
||||||||||||||||||||||
| 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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I like it. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, would be nice to be able to just use return but also call stop_peeking() indiscriminately. Oh well... maybe in a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well we could but then the whole loop content needs to be in another function. Then it's basically a debate over personal taste. ^^'
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| // This test ensures that the macro expansion is correctly handled in cases like: | ||
| // `field: !f!`, because the `:` was simply not considered because of how paths | ||
| // are handled. | ||
|
|
||
| //@ compile-flags: -Zunstable-options --generate-macro-expansion | ||
|
|
||
| #![crate_name = "foo"] | ||
|
|
||
| //@ has 'src/foo/field-followed-by-exclamation.rs.html' | ||
|
|
||
| struct Bar { | ||
| bla: bool, | ||
| } | ||
|
|
||
| macro_rules! f { | ||
| () => {{ false }} | ||
| } | ||
|
|
||
| const X: Bar = Bar { | ||
| //@ has - '//*[@class="expansion"]/*[@class="original"]/*[@class="macro"]' 'f!' | ||
| //@ has - '//*[@class="expansion"]/*[@class="original"]' 'f!()' | ||
| //@ has - '//*[@class="expansion"]/*[@class="expanded"]' '{ false }' | ||
| // It includes both original and expanded code. | ||
| //@ has - '//*[@class="expansion"]' ' bla: !{ false }f!()' | ||
| bla: !f!(), | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would make sense to switch the implementation of
peek_nextandpeek_next_ifaround, and then implementpeek_nextin terms ofpeek_next_if(|_| true).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😆