Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 45 additions & 31 deletions src/librustdoc/html/highlight.rs
Original file line number Diff line number Diff line change
Expand Up @@ -832,6 +832,20 @@ impl<'a> PeekIter<'a> {
.copied()
}

fn peek_next_if<F: Fn((TokenKind, &'a str)) -> bool>(
Copy link
Member Author

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.

Copy link
Contributor

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_next and peek_next_if around, and then implement peek_next in terms of peek_next_if(|_| true).

&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;
}
Expand Down Expand Up @@ -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();
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much better 😁

}
Comment on lines +924 to 929
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 call classifier.next() after the first None, even if yield less than nb_items items :)

}
if let Some((token, text, before)) = classifier.next() {
let text = &classifier.src[start..start + len];
classifier.advance(TokenKind::Ident, text, sink, start as u32);
} else if let Some((token, text, before)) = classifier.next() {
classifier.advance(token, text, sink, before);
} else {
break;
Expand Down Expand Up @@ -957,47 +972,47 @@ impl<'src> Classifier<'src> {
}

/// Concatenate colons and idents as one when possible.
fn get_full_ident_path(&mut self) -> Vec<(TokenKind, usize, usize)> {
let start = self.byte_pos as usize;
let mut pos = start;
fn get_full_ident_path(&mut self) -> Option<usize> {
let mut has_ident = false;
let mut nb_items = 0;

loop {
let ret = loop {
Copy link
Member Author

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.

Copy link
Contributor

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.

let mut nb = 0;
while let Some((TokenKind::Colon, _)) = self.tokens.peek() {
self.tokens.next();
while self.tokens.peek_next_if(|(token, _)| token == TokenKind::Colon).is_some() {
nb += 1;
nb_items += 1;
}
// Ident path can start with "::" but if we already have content in the ident path,
// the "::" is mandatory.
if has_ident && nb == 0 {
return vec![(TokenKind::Ident, start, pos)];
break Some(nb_items);
} else if nb != 0 && nb != 2 {
if has_ident {
return vec![(TokenKind::Ident, start, pos), (TokenKind::Colon, pos, pos + nb)];
// Following `;` will be handled on its own.
break Some(nb_items - 1);
} else {
return vec![(TokenKind::Colon, start, pos + nb)];
break None;
}
}

if let Some((TokenKind::Ident, text)) = self.tokens.peek()
if let Some((TokenKind::Ident, text)) =
self.tokens.peek_next_if(|(token, _)| token == TokenKind::Ident)
&& let symbol = Symbol::intern(text)
&& (symbol.is_path_segment_keyword() || !is_keyword(symbol))
{
// We only "add" the colon if there is an ident behind.
pos += text.len() + nb;
has_ident = true;
self.tokens.next();
nb_items += 1;
} else if nb > 0 && has_ident {
return vec![(TokenKind::Ident, start, pos), (TokenKind::Colon, pos, pos + nb)];
} else if nb > 0 {
return vec![(TokenKind::Colon, start, start + nb)];
// Following `;` will be handled on its own.
break Some(nb_items - 1);
} else if has_ident {
return vec![(TokenKind::Ident, start, pos)];
break Some(nb_items);
} else {
return Vec::new();
break None;
}
}
};
self.tokens.stop_peeking();
ret
}

/// Wraps the tokens iteration to ensure that the `byte_pos` is always correct.
Expand Down Expand Up @@ -1243,7 +1258,6 @@ impl<'src> Classifier<'src> {
Class::MacroNonTerminal
}
TokenKind::Ident => {
let file_span = self.file_span;
let span = || new_span(before, text, file_span);

match text {
Expand Down
26 changes: 26 additions & 0 deletions tests/rustdoc/macro-expansion/field-followed-by-exclamation.rs
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!(),
};
Loading