Skip to content

Commit

Permalink
fix(linter): remove all* remaining "Disallow <foo>" messages (#5812)
Browse files Browse the repository at this point in the history
Cleans diagnostic messages and help texts for (almost) all remaining rules.
There were two rules I couldn' find good replacements for.

I also made some logical improvements to a few rules, and added `pending` fixers
to others.
  • Loading branch information
DonIsaac committed Sep 17, 2024
1 parent 3bf7b24 commit f942485
Show file tree
Hide file tree
Showing 62 changed files with 1,070 additions and 1,143 deletions.
28 changes: 21 additions & 7 deletions crates/oxc_linter/src/rules/eslint/no_array_constructor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ use oxc_span::Span;
use crate::{context::LintContext, rule::Rule, AstNode};

fn no_array_constructor_diagnostic(span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow `Array` constructors")
.with_help("Use array literal instead")
OxcDiagnostic::warn("Do not use `new` to create arrays")
.with_help("Use an array literal instead")
.with_label(span)
}

Expand All @@ -16,19 +16,33 @@ pub struct NoArrayConstructor;

declare_oxc_lint!(
/// ### What it does
/// Disallow array constructor
/// Disallows creating arrays with the `Array` constructor.
///
/// ### Why is this bad?
///
/// Use of the Array constructor to construct a new array is generally discouraged in favor of array literal notation because of the single-argument pitfall and because the Array global may be redefined.
/// The exception is when the Array constructor is used to intentionally create sparse arrays of a specified size by giving the constructor a single numeric argument.
/// Use of the `Array` constructor to construct a new array is generally
/// discouraged in favor of array literal notation because of the
/// single-argument pitfall and because the `Array` global may be redefined.
/// The exception is when the `Array` constructor is used to intentionally
/// create sparse arrays of a specified size by giving the constructor a
/// single numeric argument.
///
/// ### Example
/// ### Examples
///
/// Examples of **incorrect** code for this rule:
/// ```javascript
/// let arr = new Array();
/// ```
///
/// Examples of **correct** code for this rule:
/// ```javascript
/// let arr = [];
/// let arr2 = Array.from(iterable);
/// let arr3 = new Array(9);
/// ```
NoArrayConstructor,
pedantic
pedantic,
pending
);

impl Rule for NoArrayConstructor {
Expand Down
8 changes: 4 additions & 4 deletions crates/oxc_linter/src/rules/eslint/no_caller.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ use oxc_span::Span;

use crate::{context::LintContext, rule::Rule, AstNode};

fn no_caller_diagnostic(span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow the use of arguments.caller or arguments.callee")
.with_help("'caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them")
fn no_caller_diagnostic(span: Span, method_name: &str) -> OxcDiagnostic {
OxcDiagnostic::warn(format!("Do not use `arguments.{method_name}`"))
.with_help("'caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them.")
.with_label(span)
}

Expand Down Expand Up @@ -79,7 +79,7 @@ impl Rule for NoCaller {
if (expr.property.name == "callee" || expr.property.name == "caller")
&& expr.object.is_specific_id("arguments")
{
ctx.diagnostic(no_caller_diagnostic(expr.property.span));
ctx.diagnostic(no_caller_diagnostic(expr.property.span, &expr.property.name));
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions crates/oxc_linter/src/rules/eslint/no_empty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ use oxc_span::Span;
use crate::{context::LintContext, rule::Rule, AstNode};

fn no_empty_diagnostic(stmt_kind: &str, span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow empty block statements")
.with_help(format!("Add comment inside empty {stmt_kind} statement"))
.with_label(span.label(format!("Empty {stmt_kind} statement")))
OxcDiagnostic::warn("Unexpected empty block statements")
.with_help(format!("Remove this {stmt_kind} or add a comment inside it"))
.with_label(span)
}

#[derive(Debug, Default, Clone)]
Expand Down
95 changes: 86 additions & 9 deletions crates/oxc_linter/src/rules/eslint/no_empty_function.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,26 @@
use oxc_ast::AstKind;
use std::borrow::Cow;

use oxc_ast::{
ast::{IdentifierName, IdentifierReference, MethodDefinitionKind},
AstKind,
};
use oxc_diagnostics::OxcDiagnostic;
use oxc_macros::declare_oxc_lint;
use oxc_span::Span;

use crate::{context::LintContext, rule::Rule, AstNode};

fn no_empty_function_diagnostic(span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow empty functions")
.with_help("Unexpected empty function block")
fn no_empty_function_diagnostic<S: AsRef<str>>(
span: Span,
fn_kind: &str,
fn_name: Option<S>,
) -> OxcDiagnostic {
let message = match fn_name {
Some(name) => Cow::Owned(format!("Unexpected empty {fn_kind} `{}`", name.as_ref())),
None => Cow::Borrowed("Unexpected empty function"),
};
OxcDiagnostic::warn(message)
.with_help(format!("Consider removing this {fn_kind} or adding logic to it."))
.with_label(span)
}

Expand All @@ -23,26 +36,90 @@ declare_oxc_lint!(
/// intentional or not. So writing a clear comment for empty functions is a good practice.
///
/// ### Example
/// ```javascript
///
/// Examples of **incorrect** code for this rule:
/// ```javascript
/// function foo() {
/// }
///
/// const bar = () => {};
/// ```
///
/// Examples of **correct** code for this rule:
/// ```javascript
/// function foo() {
/// // do nothing
/// }
///
/// function foo() {
/// return;
/// }
/// const add = (a, b) => a + b
/// ```
NoEmptyFunction,
restriction,
);

impl Rule for NoEmptyFunction {
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
if let AstKind::FunctionBody(fb) = node.kind() {
if fb.is_empty() && !ctx.semantic().trivias().has_comments_between(fb.span) {
ctx.diagnostic(no_empty_function_diagnostic(fb.span));
}
let AstKind::FunctionBody(fb) = node.kind() else {
return;
};
if fb.is_empty() && !ctx.semantic().trivias().has_comments_between(fb.span) {
let (kind, fn_name) = get_function_name_and_kind(node, ctx);
ctx.diagnostic(no_empty_function_diagnostic(fb.span, kind, fn_name));
}
}
}

fn get_function_name_and_kind<'a>(
node: &AstNode<'a>,
ctx: &LintContext<'a>,
) -> (&'static str, Option<Cow<'a, str>>) {
for parent in ctx.nodes().iter_parents(node.id()).skip(1).map(AstNode::kind) {
match parent {
AstKind::Function(f) => {
if let Some(name) = f.name() {
let kind = if f.generator { "generator function" } else { "function" };
return (kind, Some(name.into()));
}
continue;
}
AstKind::ArrowFunctionExpression(_) => {
continue;
}
AstKind::IdentifierName(IdentifierName { name, .. })
| AstKind::IdentifierReference(IdentifierReference { name, .. }) => {
return ("function", Some(Cow::Borrowed(name.as_str())));
}
AstKind::PropertyDefinition(prop) => {
return ("function", prop.key.name());
}
AstKind::MethodDefinition(method) => {
let kind = match method.kind {
MethodDefinitionKind::Method => {
if method.r#static {
"static method"
} else {
"method"
}
}
MethodDefinitionKind::Get => "getter",
MethodDefinitionKind::Set => "setter",
MethodDefinitionKind::Constructor => "constructor",
};
return (kind, method.key.name());
}
AstKind::VariableDeclarator(decl) => {
return ("function", decl.id.get_identifier().map(Into::into));
}
_ => return ("function", None),
}
}
#[cfg(debug_assertions)]
unreachable!();
#[cfg(not(debug_assertions))]
("function", None)
}

#[test]
Expand Down
6 changes: 2 additions & 4 deletions crates/oxc_linter/src/rules/eslint/no_empty_pattern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,9 @@ use oxc_span::Span;
use crate::{context::LintContext, rule::Rule, AstNode};

fn no_empty_pattern_diagnostic(pattern_type: &str, span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow empty destructuring patterns.")
OxcDiagnostic::warn(format!("Empty {pattern_type} binding pattern"))
.with_help("Passing `null` or `undefined` will result in runtime error because `null` and `undefined` cannot be destructured.")
.with_label(
span.label(format!("Empty {pattern_type} binding pattern")),
)
.with_label(span)
}

#[derive(Debug, Default, Clone)]
Expand Down
7 changes: 4 additions & 3 deletions crates/oxc_linter/src/rules/eslint/no_empty_static_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ use oxc_span::Span;
use crate::{context::LintContext, rule::Rule, AstNode};

fn no_empty_static_block_diagnostic(span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow empty static blocks")
.with_help("Unexpected empty static block.")
OxcDiagnostic::warn("Unexpected empty static blocks")
.with_help("Remove this empty block or add content to it.")
.with_label(span)
}

Expand Down Expand Up @@ -47,7 +47,8 @@ declare_oxc_lint!(
/// }
/// ```
NoEmptyStaticBlock,
correctness
correctness,
pending // TODO: add a safe suggestion
);

impl Rule for NoEmptyStaticBlock {
Expand Down
31 changes: 17 additions & 14 deletions crates/oxc_linter/src/rules/eslint/no_eq_null.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ use oxc_syntax::operator::BinaryOperator;

use crate::{context::LintContext, rule::Rule, AstNode};

fn no_eq_null_diagnostic(span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Use '===' to compare with null")
.with_help("Disallow `null` comparisons without type-checking operators.")
fn no_eq_null_diagnostic(span: Span, suggested_operator: &str) -> OxcDiagnostic {
OxcDiagnostic::warn("Do not use `null` comparisons without type-checking operators.")
.with_help(format!("Use '{suggested_operator}' to compare with null"))
.with_label(span)
}

Expand Down Expand Up @@ -72,22 +72,25 @@ impl Rule for NoEqNull {
& binary_expression.left.is_null()
& bad_operator
{
let suggested_operator = if binary_expression.operator == BinaryOperator::Equality {
" === "
} else {
" !== "
};
ctx.diagnostic_with_dangerous_fix(
no_eq_null_diagnostic(Span::new(
binary_expression.span.start,
binary_expression.span.end,
)),
no_eq_null_diagnostic(
// Span::new(
// binary_expression.span.start,
// binary_expression.span.end,
// )
binary_expression.span,
suggested_operator.trim(),
),
|fixer| {
let start = binary_expression.left.span().end;
let end = binary_expression.right.span().start;
let span = Span::new(start, end);
let new_operator_str =
if binary_expression.operator == BinaryOperator::Equality {
" === "
} else {
" !== "
};
fixer.replace(span, new_operator_str)
fixer.replace(span, suggested_operator)
},
);
}
Expand Down
5 changes: 3 additions & 2 deletions crates/oxc_linter/src/rules/eslint/no_iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::{context::LintContext, rule::Rule, AstNode};

fn no_iterator_diagnostic(span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Reserved name '__iterator__'")
.with_help("Disallow the use of the `__iterator__` property.")
.with_help("Consider using [Symbol.iterator] instead")
.with_label(span)
}

Expand Down Expand Up @@ -53,7 +53,8 @@ declare_oxc_lint!(
/// };
/// ```
NoIterator,
restriction
restriction,
pending // TODO: suggestion
);

impl Rule for NoIterator {
Expand Down
9 changes: 4 additions & 5 deletions crates/oxc_linter/src/rules/eslint/no_new_wrappers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,8 @@ use oxc_span::Span;
use crate::{context::LintContext, rule::Rule, AstNode};

fn no_new_wrappers_diagnostic(builtin_name: &str, new_span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow new operators with the String, Number, and Boolean objects")
.with_help(format!(
"do not use {builtin_name} as a constructor, consider removing the new operator."
))
OxcDiagnostic::warn(format!("Do not use `{builtin_name}` as a constructor"))
.with_help("Remove the `new` operator.")
.with_label(new_span)
}

Expand Down Expand Up @@ -48,7 +46,8 @@ declare_oxc_lint!(
/// var booleanObject = Boolean(value);
/// ```
NoNewWrappers,
pedantic
pedantic,
pending
);

impl Rule for NoNewWrappers {
Expand Down
4 changes: 2 additions & 2 deletions crates/oxc_linter/src/rules/eslint/no_obj_calls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ const GLOBAL_THIS: &str = "globalThis";
const NON_CALLABLE_GLOBALS: [&str; 5] = ["Atomics", "Intl", "JSON", "Math", "Reflect"];

fn no_obj_calls_diagnostic(obj_name: &str, span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow calling some global objects as functions")
.with_help(format!("{obj_name} is not a function."))
OxcDiagnostic::warn(format!("`{obj_name}` is not a function and cannot be called"))
.with_help("This call will throw a TypeError at runtime.")
.with_label(span)
}

Expand Down
11 changes: 7 additions & 4 deletions crates/oxc_linter/src/rules/eslint/no_proto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::{context::LintContext, rule::Rule, AstNode};

fn no_proto_diagnostic(span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("The '__proto__' property is deprecated")
.with_help("Disallow the use of the `__proto__` property.")
.with_help("use `Object.getPrototypeOf` and `Object.setPrototypeOf` instead.")
.with_label(span)
}

Expand All @@ -16,10 +16,12 @@ pub struct NoProto;

declare_oxc_lint!(
/// ### What it does
/// Disallow the use of the __proto__ property
/// Disallow the use of the `__proto__` property
///
/// ### Why is this bad?
/// __proto__ property has been deprecated as of ECMAScript 3.1 and shouldn’t be used in the code. Use Object.getPrototypeOf and Object.setPrototypeOf instead.
/// The `__proto__` property has been deprecated as of ECMAScript 3.1 and
/// shouldn’t be used in new code. Use `Object.getPrototypeOf` and
/// `Object.setPrototypeOf` instead.
///
/// ### Example
/// ```javascript
Expand All @@ -34,7 +36,8 @@ declare_oxc_lint!(
/// obj["__proto__"] = b;
/// ```
NoProto,
restriction
restriction,
pending
);

impl Rule for NoProto {
Expand Down
Loading

0 comments on commit f942485

Please sign in to comment.