Skip to content

Commit

Permalink
refactor(linter): improve numeric rules (#1716)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Conaclos
Conaclos authored Feb 1, 2024
1 parent 907c252 commit 674ebb6
Show file tree
Hide file tree
Showing 19 changed files with 1,258 additions and 481 deletions.
30 changes: 26 additions & 4 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,6 @@ Read our [guidelines for writing a good changelog entry](https://github.com/biom

### Linter

#### Enhancements

- [noUselessTernary](https://biomejs.dev/linter/rules/no-useless-ternary/) now provides unsafe code fixes. Contributed by @vasucp1207

#### New features

- Add the rule [noSkippedTests](https://biomejs.dev/linter/rules/no-skipped-tests), to disallow skipped tests:
Expand All @@ -89,6 +85,32 @@ Read our [guidelines for writing a good changelog entry](https://github.com/biom
```
Contributed by @DaniGuardiola

#### Enhancements

- [noUselessTernary](https://biomejs.dev/linter/rules/no-useless-ternary) now provides unsafe code fixes. Contributed by @vasucp1207

- [noApproximativeNumericConstant](https://biomejs.dev/linter/rules/no-approximative-numeric-constant) now provides unsafe code fixes and handle numbers without leading zero and numbers with digit separators.

The following numbers are now reported as approximated constants.

```js
3.14_15; // PI
.4342; // LOG10E
```

Contributed by @Conaclos

- [noPrecisionLoss](https://biomejs.dev/linter/rules/no-precision-loss) no longer reports number with extra zeros.

The following numbers are now valid.

```js
.1230000000000000000000000;
1230000000000000000000000.0;
```

Contributed by @Conaclos

#### Bug fixes

- Fix [#1651](https://github.com/biomejs/biome/issues/1651). [noVar](https://biomejs.dev/linter/rules/no-var/) now ignores TsGlobalDeclaration. Contributed by @vasucp1207
Expand Down
74 changes: 31 additions & 43 deletions crates/biome_js_analyze/src/analyzers/correctness/no_precision_loss.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,23 +22,15 @@ declare_rule! {
/// ```
///
/// ```js,expect_diagnostic
/// const x = 5123000000000000000000000000001
/// const x = 5.123000000000000000000000000001
/// ```
///
/// ```js,expect_diagnostic
/// const x = 1230000000000000000000000.0
/// const x = 0x20000000000001
/// ```
///
/// ```js,expect_diagnostic
/// const x = .1230000000000000000000000
/// ```
///
/// ```js,expect_diagnostic
/// const x = 0X20000000000001
/// ```
///
/// ```js,expect_diagnostic
/// const x = 0X2_000000000_0001;
/// const x = 0x2_000000000_0001;
/// ```
///
/// ### Valid
Expand Down Expand Up @@ -105,21 +97,22 @@ fn is_precision_lost(node: &JsNumberLiteralExpression) -> Option<bool> {
}

fn is_precision_lost_in_base_10(num: &str) -> Option<bool> {
const MAX_SIGNIFICANT_DIGITS_BASE10: u32 = 17;
let normalized = NormalizedNumber::new(num);
let precision = normalized.precision();
if precision == 0 {
return Some(false);
}
if precision > 100 {
if precision > MAX_SIGNIFICANT_DIGITS_BASE10 as usize {
return Some(true);
}
let parsed = num.parse::<f64>().ok()?;
let stored_num = format!("{:.*e}", precision - 1, parsed);
Some(stored_num != normalized.to_scientific())
}

fn is_precision_lost_in_base_other(num: &str, radix: u32) -> bool {
let parsed = match i64::from_str_radix(num, radix) {
fn is_precision_lost_in_base_other(num: &str, radix: u8) -> bool {
let parsed = match i64::from_str_radix(num, radix as u32) {
Ok(x) => x,
Err(e) => {
return matches!(
Expand All @@ -145,7 +138,7 @@ fn remove_trailing_zeros(num: &str) -> &str {
}

#[derive(Debug)]
/// Normalized number in the form `0.{digits}.{digits_rest}e{exponent}`
/// Normalized number in the form `0.{digits}{digits_rest}e{exponent}`
struct NormalizedNumber<'a> {
digits: &'a str,
digits_rest: &'a str,
Expand All @@ -155,49 +148,44 @@ struct NormalizedNumber<'a> {
impl NormalizedNumber<'_> {
fn new(num: &str) -> NormalizedNumber<'_> {
let num = remove_leading_zeros(num);
let mut split = num.splitn(2, ['e', 'E']);

// SAFETY: unwrap is ok because even an empty string will produce one part.
let mantissa = split.next().unwrap();
let exponent = split.next();
let mut mantissa_parts = mantissa.splitn(2, '.');

// SAFETY: unwrap is ok because even an empty string will produce one part.
let mut normalized = match (mantissa_parts.next().unwrap(), mantissa_parts.next()) {
("", Some(fraction)) => {
let (mantissa, exponent) = num
.split_once(['e', 'E'])
.and_then(|(mantissa, exponent)| Some((mantissa, exponent.parse::<isize>().ok()?)))
.unwrap_or((num, 0));
match mantissa.split_once(['.']) {
None => NormalizedNumber {
digits: remove_trailing_zeros(mantissa),
digits_rest: "",
exponent: exponent + mantissa.len() as isize,
},
Some(("", fraction)) => {
let digits = remove_leading_zeros(fraction);
NormalizedNumber {
digits,
digits: remove_trailing_zeros(digits),
digits_rest: "",
exponent: digits.len() as isize - fraction.len() as isize,
exponent: digits.len() as isize - fraction.len() as isize + exponent,
}
}
(integer, Some(fraction)) => NormalizedNumber {
digits: integer,
digits_rest: fraction,
exponent: integer.len() as isize,
},
(integer, None) => {
let digits = remove_trailing_zeros(integer);
Some((integer, fraction)) => {
let fraction = remove_trailing_zeros(fraction);
let digits = if fraction.is_empty() {
remove_trailing_zeros(integer)
} else {
integer
};
NormalizedNumber {
digits,
digits_rest: "",
exponent: integer.len() as isize,
digits_rest: fraction,
exponent: exponent + integer.len() as isize,
}
}
};

if let Some(exponent) = exponent.and_then(|it| it.parse::<isize>().ok()) {
normalized.exponent += exponent;
}

normalized
}

fn to_scientific(&self) -> String {
let fraction = &self.digits[1..];
if fraction.is_empty() && self.digits_rest.is_empty() {
format!("{}e{}", &self.digits[..1], self.exponent - 1)
format!("{}e{}", self.digits, self.exponent - 1)
} else {
format!(
"{}.{}{}e{}",
Expand Down
168 changes: 108 additions & 60 deletions crates/biome_js_analyze/src/analyzers/suspicious/no_approximative_numeric_constant.rs
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,25 @@
use std::f64::consts as f64;
use std::cmp::Ordering;

use biome_analyze::{context::RuleContext, declare_rule, Ast, Rule, RuleDiagnostic, RuleSource};
use biome_analyze::{
context::RuleContext, declare_rule, ActionCategory, Ast, FixKind, Rule, RuleDiagnostic,
RuleSource,
};
use biome_console::markup;
use biome_js_syntax::JsNumberLiteralExpression;
use biome_rowan::AstNode;
use biome_diagnostics::Applicability;
use biome_js_factory::make;
use biome_js_syntax::{
numbers::split_into_radix_and_number, AnyJsExpression, AnyJsLiteralExpression,
JsNumberLiteralExpression, T,
};
use biome_rowan::{AstNode, BatchMutationExt};

use crate::JsRuleAction;

declare_rule! {
/// Usually, the definition in the standard library is more precise than what people come up with or the used constant exceeds the maximum precision of the number type.
/// Use standard constants instead of approximated literals.
///
/// Usually, the definition in the standard library is more precise than
/// what people come up with or the used constant exceeds the maximum precision of the number type.
///
/// ## Examples
///
Expand All @@ -15,6 +28,7 @@ declare_rule! {
/// ```js,expect_diagnostic
/// let x = 3.141;
/// ```
///
/// ```js,expect_diagnostic
/// let x = 2.302;
/// ```
Expand All @@ -23,7 +37,9 @@ declare_rule! {
///
/// ```js
/// let x = Math.PI;
/// let y = 3.14;
/// ```
///
/// ```js
/// let x = Math.LN10;
/// ```
Expand All @@ -32,81 +48,113 @@ declare_rule! {
name: "noApproximativeNumericConstant",
source: RuleSource::Clippy("approx_constant"),
recommended: false,
fix_kind: FixKind::Unsafe,
}
}

impl Rule for NoApproximativeNumericConstant {
type Query = Ast<JsNumberLiteralExpression>;
type State = ();
type State = &'static str;
type Signals = Option<Self::State>;
type Options = ();

fn run(ctx: &RuleContext<Self>) -> Self::Signals {
let node = ctx.query();

if get_approximative_literal_diagnostic(node).is_some() {
Some(())
} else {
None
let token = ctx.query().value_token().ok()?;
let num = token.text_trimmed();
let (10, num) = split_into_radix_and_number(num) else {
return None;
};
let (decimal, fraction) = num.split_once('.')?;
if fraction.len() < (MIN_FRACTION_DIGITS as usize)
|| !matches!(decimal, "" | "0" | "1" | "2" | "3")
|| fraction.contains(['e', 'E'])
{
return None;
}
let num = num.trim_matches('0');
for (constant, name) in KNOWN_CONSTS {
let is_constant_approximated = match constant.len().cmp(&num.len()) {
Ordering::Less => is_approx_const(num, constant),
Ordering::Equal => constant == num,
Ordering::Greater => is_approx_const(constant, num),
};
if is_constant_approximated {
return Some(name);
}
}
None
}

fn diagnostic(ctx: &RuleContext<Self>, _: &Self::State) -> Option<RuleDiagnostic> {
let node = ctx.query();
get_approximative_literal_diagnostic(node)
Some(RuleDiagnostic::new(
rule_category!(),
node.range(),
markup! { "Prefer constants from the standard library." },
))
}
}

// Tuples are of the form (constant, name, min_digits)
const KNOWN_CONSTS: [(f64, &str, usize); 8] = [
(f64::E, "E", 4),
(f64::LN_10, "LN10", 4),
(f64::LN_2, "LN2", 4),
(f64::LOG10_E, "LOG10E", 4),
(f64::LOG2_E, "LOG2E", 4),
(f64::PI, "PI", 4),
(f64::FRAC_1_SQRT_2, "SQRT1_2", 4),
(f64::SQRT_2, "SQRT2", 4),
];

fn get_approximative_literal_diagnostic(
node: &JsNumberLiteralExpression,
) -> Option<RuleDiagnostic> {
let binding = node.text();
let s = binding.trim();
if s.parse::<f64>().is_err() {
return None;
fn action(ctx: &RuleContext<Self>, constant_name: &Self::State) -> Option<JsRuleAction> {
let node = ctx.query();
let new_node = make::js_static_member_expression(
make::js_identifier_expression(make::js_reference_identifier(make::ident("Math")))
.into(),
make::token(T![.]),
make::js_name(make::ident(constant_name)).into(),
);
let mut mutation = ctx.root().begin();
mutation.replace_node(
AnyJsExpression::AnyJsLiteralExpression(AnyJsLiteralExpression::from(node.clone())),
AnyJsExpression::from(new_node),
);
Some(JsRuleAction {
category: ActionCategory::QuickFix,
applicability: Applicability::MaybeIncorrect,
message: markup! { "Use "<Emphasis>"Math."{ constant_name }</Emphasis>" instead." }
.to_owned(),
mutation,
})
}
}

for &(constant, name, min_digits) in &KNOWN_CONSTS {
if is_approx_const(constant, s, min_digits) {
return Some(
RuleDiagnostic::new(
rule_category!(),
node.syntax().text_trimmed_range(),
markup! { "Prefer constants from the standard library." },
)
.note(markup! { "Use "<Emphasis>"Math."{ name }</Emphasis>" instead." }),
);
}
}
const MIN_FRACTION_DIGITS: u8 = 3;

None
}
// Tuples are of the form (constant, name)
const KNOWN_CONSTS: [(&str, &str); 8] = [
("2.718281828459045", "E"),
("2.302585092994046", "LN10"),
(".6931471805599453", "LN2"),
(".4342944819032518", "LOG10E"),
("1.4426950408889634", "LOG2E"),
("3.141592653589793", "PI"),
(".7071067811865476", "SQRT1_2"),
("1.4142135623730951", "SQRT2"),
];

/// Returns `false` if the number of significant figures in `value` are
/// less than `min_digits`; otherwise, returns true if `value` is equal
/// to `constant`, rounded to the number of digits present in `value`.
/// Taken from rust-clippy/clippy_lints:
/// https://github.com/rust-lang/rust-clippy/blob/9554e477c29e6ddca9e5cdce71524341ef9d48e8/clippy_lints/src/approx_const.rs#L118-L132
fn is_approx_const(constant: f64, value: &str, min_digits: usize) -> bool {
if value.len() <= min_digits {
false
} else if constant.to_string().starts_with(value) {
/// Returns true if `value` is equal to `constant`,
/// or rounded to the number of digits present in `value`.
fn is_approx_const(constant: &str, value: &str) -> bool {
if constant.starts_with(value) {
// The value is a truncated constant
true
} else {
let round_const = format!("{constant:.*}", value.len() - 2);
value == round_const
return true;
}
let (digits, last_digit) = value.split_at(value.len() - 1);
if constant.starts_with(digits) {
let Ok(last_digit) = last_digit.parse::<u8>() else {
return false;
};
let Ok(extra_constant_digit) = constant[value.len()..value.len() + 1].parse::<u8>() else {
return false;
};
let can_be_rounded = extra_constant_digit < 5;
if can_be_rounded {
return false;
}
let Ok(constant_digit) = constant[digits.len()..digits.len() + 1].parse::<u8>() else {
return false;
};
let rounded_constant_digit = constant_digit + 1;
return last_digit == rounded_constant_digit;
}
false
}
Loading

0 comments on commit 674ebb6

Please sign in to comment.