-
Notifications
You must be signed in to change notification settings - Fork 900
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
Add option to control trailing zero in floating-point literals #5834
base: master
Are you sure you want to change the base?
Changes from all commits
366f35b
3436b40
004bc6b
497e17e
cb2929a
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 |
---|---|---|
|
@@ -264,7 +264,7 @@ impl ChainItemKind { | |
return ( | ||
ChainItemKind::Parent { | ||
expr: expr.clone(), | ||
parens: is_method_call_receiver && should_add_parens(expr), | ||
parens: is_method_call_receiver && should_add_parens(expr, context), | ||
}, | ||
expr.span, | ||
); | ||
|
@@ -1049,12 +1049,12 @@ fn trim_tries(s: &str) -> String { | |
/// 1. .method(); | ||
/// ``` | ||
/// Which all need parenthesis or a space before `.method()`. | ||
fn should_add_parens(expr: &ast::Expr) -> bool { | ||
fn should_add_parens(expr: &ast::Expr, context: &RewriteContext<'_>) -> bool { | ||
match expr.kind { | ||
ast::ExprKind::Lit(ref lit) => crate::expr::lit_ends_in_dot(lit), | ||
ast::ExprKind::Lit(ref lit) => crate::expr::lit_ends_in_dot(lit, context), | ||
ast::ExprKind::Closure(ref cl) => match cl.body.kind { | ||
ast::ExprKind::Range(_, _, ast::RangeLimits::HalfOpen) => true, | ||
ast::ExprKind::Lit(ref lit) => crate::expr::lit_ends_in_dot(lit), | ||
ast::ExprKind::Lit(ref lit) => crate::expr::lit_ends_in_dot(lit, context), | ||
Comment on lines
+1052
to
+1057
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missed this before, but can we also add test cases to the existing files to make sure we're correctly adding parentheses around method calls on float literals and closures that contain a single float literal. |
||
_ => false, | ||
}, | ||
_ => false, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,8 +13,8 @@ use crate::comment::{ | |
CharClasses, FindUncommented, combine_strs_with_missing_comments, contains_comment, | ||
recover_comment_removed, rewrite_comment, rewrite_missing_comment, | ||
}; | ||
use crate::config::lists::*; | ||
use crate::config::{Config, ControlBraceStyle, HexLiteralCase, IndentStyle, StyleEdition}; | ||
use crate::config::{FloatLiteralTrailingZero, lists::*}; | ||
use crate::lists::{ | ||
ListFormatting, Separator, definitive_tactic, itemize_list, shape_for_tactic, | ||
struct_lit_formatting, struct_lit_shape, struct_lit_tactic, write_list, | ||
|
@@ -54,8 +54,11 @@ pub(crate) enum ExprType { | |
SubExpression, | ||
} | ||
|
||
pub(crate) fn lit_ends_in_dot(lit: &Lit) -> bool { | ||
matches!(lit, Lit { kind: LitKind::Float, suffix: None, symbol } if symbol.as_str().ends_with('.')) | ||
pub(crate) fn lit_ends_in_dot(lit: &Lit, context: &RewriteContext<'_>) -> bool { | ||
match lit.kind { | ||
LitKind::Float => rewrite_float_lit_inner(context, *lit).ends_with('.'), | ||
_ => false, | ||
} | ||
} | ||
|
||
pub(crate) fn format_expr( | ||
|
@@ -297,7 +300,7 @@ pub(crate) fn format_expr( | |
|
||
fn needs_space_before_range(context: &RewriteContext<'_>, lhs: &ast::Expr) -> bool { | ||
match lhs.kind { | ||
ast::ExprKind::Lit(token_lit) => lit_ends_in_dot(&token_lit), | ||
ast::ExprKind::Lit(token_lit) => lit_ends_in_dot(&token_lit, context), | ||
ast::ExprKind::Unary(_, ref expr) => needs_space_before_range(context, expr), | ||
ast::ExprKind::Binary(_, _, ref rhs_expr) => { | ||
needs_space_before_range(context, rhs_expr) | ||
|
@@ -1275,6 +1278,7 @@ pub(crate) fn rewrite_literal( | |
match token_lit.kind { | ||
token::LitKind::Str => rewrite_string_lit(context, span, shape), | ||
token::LitKind::Integer => rewrite_int_lit(context, token_lit, span, shape), | ||
token::LitKind::Float => rewrite_float_lit(context, token_lit, span, shape), | ||
_ => wrap_str( | ||
context.snippet(span).to_owned(), | ||
context.config.max_width(), | ||
|
@@ -1318,7 +1322,12 @@ fn rewrite_int_lit( | |
span: Span, | ||
shape: Shape, | ||
) -> RewriteResult { | ||
if token_lit.is_semantic_float() { | ||
return rewrite_float_lit(context, token_lit, span, shape); | ||
} | ||
|
||
let symbol = token_lit.symbol.as_str(); | ||
let suffix = token_lit.suffix.as_ref().map(|s| s.as_str()); | ||
|
||
if let Some(symbol_stripped) = symbol.strip_prefix("0x") { | ||
let hex_lit = match context.config.hex_literal_case() { | ||
|
@@ -1328,11 +1337,7 @@ fn rewrite_int_lit( | |
}; | ||
if let Some(hex_lit) = hex_lit { | ||
return wrap_str( | ||
format!( | ||
"0x{}{}", | ||
hex_lit, | ||
token_lit.suffix.as_ref().map_or("", |s| s.as_str()) | ||
), | ||
format!("0x{}{}", hex_lit, suffix.unwrap_or("")), | ||
Comment on lines
-1331
to
+1340
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did this need to change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. I thought it would be nice to keep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you could revert the change that would be great |
||
context.config.max_width(), | ||
shape, | ||
) | ||
|
@@ -1348,6 +1353,71 @@ fn rewrite_int_lit( | |
.max_width_error(shape.width, span) | ||
} | ||
|
||
fn rewrite_float_lit_inner(context: &RewriteContext<'_>, token_lit: token::Lit) -> String { | ||
let symbol = token_lit.symbol.as_str(); | ||
let suffix = token_lit.suffix.as_ref().map(|s| s.as_str()); | ||
|
||
if matches!( | ||
context.config.float_literal_trailing_zero(), | ||
FloatLiteralTrailingZero::Preserve | ||
) { | ||
return format!("{}{}", symbol, suffix.unwrap_or("")); | ||
} | ||
|
||
let float_parts = parse_float_symbol(symbol).unwrap(); | ||
let FloatSymbolParts { | ||
integer_part, | ||
fractional_part, | ||
exponent, | ||
} = float_parts; | ||
|
||
let has_postfix = exponent.is_some() || suffix.is_some(); | ||
let fractional_part_nonzero = !float_parts.is_fractional_part_zero(); | ||
|
||
let (include_period, include_fractional_part) = | ||
match context.config.float_literal_trailing_zero() { | ||
FloatLiteralTrailingZero::Preserve => unreachable!("handled above"), | ||
FloatLiteralTrailingZero::Always => (true, true), | ||
FloatLiteralTrailingZero::IfNoPostfix => ( | ||
fractional_part_nonzero || !has_postfix, | ||
fractional_part_nonzero || !has_postfix, | ||
), | ||
FloatLiteralTrailingZero::Never => ( | ||
fractional_part_nonzero || !has_postfix, | ||
fractional_part_nonzero, | ||
), | ||
}; | ||
|
||
let period = if include_period { "." } else { "" }; | ||
let fractional_part = if include_fractional_part { | ||
fractional_part.unwrap_or("0") | ||
} else { | ||
"" | ||
}; | ||
format!( | ||
"{}{}{}{}{}", | ||
integer_part, | ||
period, | ||
fractional_part, | ||
exponent.unwrap_or(""), | ||
suffix.unwrap_or(""), | ||
) | ||
} | ||
|
||
fn rewrite_float_lit( | ||
context: &RewriteContext<'_>, | ||
token_lit: token::Lit, | ||
span: Span, | ||
shape: Shape, | ||
) -> RewriteResult { | ||
wrap_str( | ||
rewrite_float_lit_inner(context, token_lit), | ||
context.config.max_width(), | ||
shape, | ||
) | ||
.max_width_error(shape.width, span) | ||
} | ||
|
||
fn choose_separator_tactic(context: &RewriteContext<'_>, span: Span) -> Option<SeparatorTactic> { | ||
if context.inside_macro() { | ||
if span_ends_with_comma(context, span) { | ||
|
@@ -2273,9 +2343,45 @@ pub(crate) fn is_method_call(expr: &ast::Expr) -> bool { | |
} | ||
} | ||
|
||
/// Indicates the parts of a float literal specified as a string. | ||
struct FloatSymbolParts<'a> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a doc comment for this struct, and it's fields. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. BTW, is the policy on doc comments described somewhere? I thought only public entities should be doc-commented. I see now that it's not the case, but I'm not sure which comments should be doc vs non-doc for private items. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There isn't really a policy, but moving forward I'd like to have more of the internals documented. Especially for newer items that I'm not 100% familiar with. It doesn't make much of a difference to me whether they're doc comments or regular comments, but some explanation would be nice |
||
/// The integer part, e.g. `123` in `123.456e789`. | ||
/// Always non-empty, because in Rust `.1` is not a valid floating-point literal: | ||
/// <https://doc.rust-lang.org/reference/tokens.html#floating-point-literals> | ||
integer_part: &'a str, | ||
/// The fractional part excluding the decimal point, e.g. `456` in `123.456e789`. | ||
fractional_part: Option<&'a str>, | ||
/// The exponent part including the `e` or `E`, e.g. `e789` in `123.456e789`. | ||
exponent: Option<&'a str>, | ||
} | ||
|
||
impl FloatSymbolParts<'_> { | ||
fn is_fractional_part_zero(&self) -> bool { | ||
let zero_literal_regex = static_regex!(r"^[0_]+$"); | ||
self.fractional_part | ||
.is_none_or(|s| zero_literal_regex.is_match(s)) | ||
} | ||
} | ||
|
||
/// Parses a float literal. The `symbol` must be a valid floating point literal without a type | ||
/// suffix. Otherwise the function may panic or return wrong result. | ||
fn parse_float_symbol(symbol: &str) -> Result<FloatSymbolParts<'_>, &'static str> { | ||
// This regex may accept invalid float literals (such as `1`, `_` or `2.e3`). That's ok. | ||
// We only use it to parse literals whose validity has already been established. | ||
let float_literal_regex = static_regex!(r"^([0-9_]+)(?:\.([0-9_]+)?)?([eE][+-]?[0-9_]+)?$"); | ||
let caps = float_literal_regex | ||
.captures(symbol) | ||
.ok_or("invalid float literal")?; | ||
Ok(FloatSymbolParts { | ||
integer_part: caps.get(1).ok_or("missing integer part")?.as_str(), | ||
fractional_part: caps.get(2).map(|m| m.as_str()), | ||
exponent: caps.get(3).map(|m| m.as_str()), | ||
}) | ||
} | ||
|
||
#[cfg(test)] | ||
mod test { | ||
use super::last_line_offsetted; | ||
use super::*; | ||
|
||
#[test] | ||
fn test_last_line_offsetted() { | ||
|
@@ -2297,4 +2403,60 @@ mod test { | |
let lines = "one\n two three"; | ||
assert_eq!(last_line_offsetted(2, lines), false); | ||
} | ||
|
||
#[test] | ||
fn test_parse_float_symbol() { | ||
let parts = parse_float_symbol("123.456e789").unwrap(); | ||
assert_eq!(parts.integer_part, "123"); | ||
assert_eq!(parts.fractional_part, Some("456")); | ||
assert_eq!(parts.exponent, Some("e789")); | ||
|
||
let parts = parse_float_symbol("123.456e+789").unwrap(); | ||
assert_eq!(parts.integer_part, "123"); | ||
assert_eq!(parts.fractional_part, Some("456")); | ||
assert_eq!(parts.exponent, Some("e+789")); | ||
|
||
let parts = parse_float_symbol("123.456e-789").unwrap(); | ||
assert_eq!(parts.integer_part, "123"); | ||
assert_eq!(parts.fractional_part, Some("456")); | ||
assert_eq!(parts.exponent, Some("e-789")); | ||
|
||
let parts = parse_float_symbol("123e789").unwrap(); | ||
assert_eq!(parts.integer_part, "123"); | ||
assert_eq!(parts.fractional_part, None); | ||
assert_eq!(parts.exponent, Some("e789")); | ||
|
||
let parts = parse_float_symbol("123E789").unwrap(); | ||
assert_eq!(parts.integer_part, "123"); | ||
assert_eq!(parts.fractional_part, None); | ||
assert_eq!(parts.exponent, Some("E789")); | ||
|
||
let parts = parse_float_symbol("123.").unwrap(); | ||
assert_eq!(parts.integer_part, "123"); | ||
assert_eq!(parts.fractional_part, None); | ||
assert_eq!(parts.exponent, None); | ||
} | ||
|
||
#[test] | ||
fn test_parse_float_symbol_with_underscores() { | ||
let parts = parse_float_symbol("_123._456e_789").unwrap(); | ||
assert_eq!(parts.integer_part, "_123"); | ||
assert_eq!(parts.fractional_part, Some("_456")); | ||
assert_eq!(parts.exponent, Some("e_789")); | ||
|
||
let parts = parse_float_symbol("123_.456_e789_").unwrap(); | ||
assert_eq!(parts.integer_part, "123_"); | ||
assert_eq!(parts.fractional_part, Some("456_")); | ||
assert_eq!(parts.exponent, Some("e789_")); | ||
|
||
let parts = parse_float_symbol("1_23.4_56e7_89").unwrap(); | ||
assert_eq!(parts.integer_part, "1_23"); | ||
assert_eq!(parts.fractional_part, Some("4_56")); | ||
assert_eq!(parts.exponent, Some("e7_89")); | ||
|
||
let parts = parse_float_symbol("_1_23_._4_56_e_7_89_").unwrap(); | ||
assert_eq!(parts.integer_part, "_1_23_"); | ||
assert_eq!(parts.fractional_part, Some("_4_56_")); | ||
assert_eq!(parts.exponent, Some("e_7_89_")); | ||
} | ||
} |
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.
Also, we'll need to add a new tracking issue for this one. #3187 isn't a tracking issue. We can certainly add the tracking issue after this PR is merged.
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.
Why don't we create an issue now, so that we don't have to go back and modify the link later?
Is there a document that describes how a tracking issue should look like?