Skip to content
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

On format!() arg count mismatch provide extra info #63121

Merged
merged 5 commits into from
Aug 3, 2019
Merged
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
72 changes: 48 additions & 24 deletions src/libfmt_macros/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,20 @@ pub struct Argument<'a> {
/// Specification for the formatting of an argument in the format string.
#[derive(Copy, Clone, PartialEq)]
pub struct FormatSpec<'a> {
/// Optionally specified character to fill alignment with
/// Optionally specified character to fill alignment with.
pub fill: Option<char>,
/// Optionally specified alignment
/// Optionally specified alignment.
pub align: Alignment,
/// Packed version of various flags provided
/// Packed version of various flags provided.
pub flags: u32,
/// The integer precision to use
/// The integer precision to use.
pub precision: Count,
/// The string width requested for the resulting format
/// The span of the precision formatting flag (for diagnostics).
pub precision_span: Option<InnerSpan>,
estebank marked this conversation as resolved.
Show resolved Hide resolved
/// The string width requested for the resulting format.
pub width: Count,
/// The span of the width formatting flag (for diagnostics).
pub width_span: Option<InnerSpan>,
/// The descriptor string representing the name of the format desired for
/// this argument, this can be empty or any number of characters, although
/// it is required to be one word.
Expand Down Expand Up @@ -285,19 +289,24 @@ impl<'a> Parser<'a> {
}

/// Optionally consumes the specified character. If the character is not at
/// the current position, then the current iterator isn't moved and false is
/// returned, otherwise the character is consumed and true is returned.
/// the current position, then the current iterator isn't moved and `false` is
/// returned, otherwise the character is consumed and `true` is returned.
fn consume(&mut self, c: char) -> bool {
if let Some(&(_, maybe)) = self.cur.peek() {
self.consume_pos(c).is_some()
}

/// Optionally consumes the specified character. If the character is not at
/// the current position, then the current iterator isn't moved and `None` is
/// returned, otherwise the character is consumed and the current position is
/// returned.
fn consume_pos(&mut self, c: char) -> Option<usize> {
if let Some(&(pos, maybe)) = self.cur.peek() {
estebank marked this conversation as resolved.
Show resolved Hide resolved
if c == maybe {
self.cur.next();
true
} else {
false
return Some(pos);
}
} else {
false
}
None
}

fn to_span_index(&self, pos: usize) -> InnerOffset {
Expand Down Expand Up @@ -465,7 +474,9 @@ impl<'a> Parser<'a> {
align: AlignUnknown,
flags: 0,
precision: CountImplied,
precision_span: None,
width: CountImplied,
width_span: None,
ty: &self.input[..0],
};
if !self.consume(':') {
Expand Down Expand Up @@ -502,6 +513,7 @@ impl<'a> Parser<'a> {
}
// Width and precision
let mut havewidth = false;

if self.consume('0') {
// small ambiguity with '0$' as a format string. In theory this is a
// '0' flag and then an ill-formatted format string with just a '$'
Expand All @@ -515,17 +527,28 @@ impl<'a> Parser<'a> {
}
}
if !havewidth {
spec.width = self.count();
let width_span_start = if let Some((pos, _)) = self.cur.peek() {
*pos
} else {
0
};
let (w, sp) = self.count(width_span_start);
spec.width = w;
spec.width_span = sp;
}
if self.consume('.') {
if self.consume('*') {
if let Some(start) = self.consume_pos('.') {
if let Some(end) = self.consume_pos('*') {
// Resolve `CountIsNextParam`.
// We can do this immediately as `position` is resolved later.
let i = self.curarg;
self.curarg += 1;
spec.precision = CountIsParam(i);
spec.precision_span =
Some(self.to_span_index(start).to(self.to_span_index(end + 1)));
} else {
spec.precision = self.count();
let (p, sp) = self.count(start);
spec.precision = p;
spec.precision_span = sp;
}
}
// Optional radix followed by the actual format specifier
Expand Down Expand Up @@ -554,24 +577,25 @@ impl<'a> Parser<'a> {
/// Parses a Count parameter at the current position. This does not check
/// for 'CountIsNextParam' because that is only used in precision, not
/// width.
fn count(&mut self) -> Count {
fn count(&mut self, start: usize) -> (Count, Option<InnerSpan>) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it's pretty tailored in the implementation, but perhaps this could be somewhat more generalized? For example instead of modifying existing methods and having some which return a span and some which don't (in addition to passing around start spans and such) could one new function be added liek:

fn span<R>(&mut self, f: impl FnOnce(&mut Self) -> R) -> (R, Span) {
    // ...
}

and that's called like let (count, span) = self.span(|me| me.count()); or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is that the actual span for the thing being composed requires some passing of location between different methods. count in particular is only used in two places and both care about the span. These methods are meant to only be used from a single module, which is why I didn't bother with an externally pleasing api surface for them.

if let Some(i) = self.integer() {
if self.consume('$') {
CountIsParam(i)
if let Some(end) = self.consume_pos('$') {
let span = self.to_span_index(start).to(self.to_span_index(end + 1));
(CountIsParam(i), Some(span))
} else {
CountIs(i)
(CountIs(i), None)
}
} else {
let tmp = self.cur.clone();
let word = self.word();
if word.is_empty() {
self.cur = tmp;
CountImplied
(CountImplied, None)
} else if self.consume('$') {
CountIsName(Symbol::intern(word))
(CountIsName(Symbol::intern(word)), None)
} else {
self.cur = tmp;
CountImplied
(CountImplied, None)
}
}
}
Expand Down
Loading