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

fix(linter): improve diagnostic messages for various lint rules #5808

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
16 changes: 11 additions & 5 deletions crates/oxc_linter/src/rules/node/no_exports_assign.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ use oxc_span::{GetSpan, Span};
use crate::{context::LintContext, rule::Rule, AstNode};

fn no_exports_assign(span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow the assignment to `exports`.")
OxcDiagnostic::warn("Unexpected assignment to 'exports'.")
.with_label(span)
.with_help("Unexpected assignment to 'exports' variable. Use 'module.exports' instead.")
.with_help("Use 'module.exports' instead.")
}

fn is_exports(node: &AssignmentTarget, ctx: &LintContext) -> bool {
Expand Down Expand Up @@ -40,12 +40,18 @@ pub struct NoExportsAssign;

declare_oxc_lint!(
/// ### What it does
///
/// This rule is aimed at disallowing `exports = {}`, but allows `module.exports = exports = {}` to avoid conflict with `n/exports-style` rule's `allowBatchAssign` option.
/// Disallows assignment to `exports`.
///
/// ### Why is this bad?
///
/// Directly using `exports = {}` can lead to confusion and potential bugs because it reassigns the `exports` object, which may break module exports. It is more predictable and clearer to use `module.exports` directly or in conjunction with `exports`.
/// Directly using `exports = {}` can lead to confusion and potential bugs
/// because it reassigns the `exports` object, which may break module
/// exports. It is more predictable and clearer to use `module.exports`
/// directly or in conjunction with `exports`.
///
/// This rule is aimed at disallowing `exports = {}`, but allows
/// `module.exports = exports = {}` to avoid conflict with `n/exports-style`
/// rule's `allowBatchAssign` option.
///
/// ### Examples
///
Expand Down
19 changes: 15 additions & 4 deletions crates/oxc_linter/src/rules/promise/no_new_statics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ use oxc_span::Span;
use crate::{context::LintContext, rule::Rule, utils::PROMISE_STATIC_METHODS, AstNode};

fn static_promise_diagnostic(static_name: &str, span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn(format!("Disallow calling `new` on a `Promise.{static_name}`"))
OxcDiagnostic::warn(format!("Do not use `new` on `Promise.{static_name}`"))
.with_help(format!(
"`Promise.{static_name}` is not a constructor. Call it as a function instead."
))
.with_label(span)
}

Expand All @@ -16,15 +19,23 @@ pub struct NoNewStatics;
declare_oxc_lint!(
/// ### What it does
///
/// Disallow calling new on a Promise static method.
/// Disallows calling new on static `Promise` methods.
///
/// ### Why is this bad?
///
/// Calling a Promise static method with new is invalid, resulting in a TypeError at runtime.
/// Calling a static `Promise` method with `new` is invalid and will result
/// in a `TypeError` at runtime.
///
/// ### Example
///
/// Examples of **incorrect** code for this rule:
/// ```javascript
/// const x = new Promise.resolve(value);
/// ```
///
/// Examples of **correct** code for this rule:
/// ```javascript
/// new Promise.resolve(value);
/// const x = Promise.resolve(value);
/// ```
NoNewStatics,
correctness,
Expand Down
4 changes: 1 addition & 3 deletions crates/oxc_linter/src/rules/react/jsx_no_undef.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ use crate::{
};

fn jsx_no_undef_diagnostic(ident_name: &str, span1: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow undeclared variables in JSX")
.with_help(format!("'{ident_name}' is not defined."))
.with_label(span1)
OxcDiagnostic::warn(format!("'{ident_name}' is not defined.")).with_label(span1)
}

#[derive(Debug, Default, Clone)]
Expand Down
25 changes: 15 additions & 10 deletions crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use itertools::Itertools;
use oxc_ast::{ast::JSXAttributeItem, AstKind};
use oxc_diagnostics::OxcDiagnostic;
use oxc_macros::declare_oxc_lint;
use oxc_span::{Atom, Span};
use oxc_span::{Atom, GetSpan, Span};
use rustc_hash::FxHashMap;

use crate::{
Expand All @@ -17,14 +17,17 @@ fn jsx_props_no_spread_multiple_identifiers_diagnostic(
spans: Vec<Span>,
prop_name: &str,
) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow JSX prop spreading the same identifier multiple times.")
.with_help(format!("Prop '{prop_name}' is spread multiple times."))
OxcDiagnostic::warn(format!("Prop '{prop_name}' is spread multiple times."))
.with_help("Remove all but one spread.")
.with_labels(spans)
}

fn jsx_props_no_spread_multiple_member_expressions_diagnostic(spans: Vec<Span>) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow JSX prop spreading the same member expression multiple times.")
.with_help("Remove the first spread.")
fn jsx_props_no_spread_multiple_member_expressions_diagnostic(
spans: Vec<Span>,
member_name: &str,
) -> OxcDiagnostic {
OxcDiagnostic::warn(format!("'{member_name}' is spread multiple times."))
.with_help("Remove all but one spread.")
.with_labels(spans)
}

Expand Down Expand Up @@ -109,11 +112,13 @@ impl Rule for JsxPropsNoSpreadMulti {
member_expressions.iter().tuple_combinations().for_each(
|((left, left_span), (right, right_span))| {
if is_same_member_expression(left, right, ctx) {
// 'foo.bar'
let member_prop_name = ctx.source_range(left.span());
ctx.diagnostic_with_fix(
jsx_props_no_spread_multiple_member_expressions_diagnostic(vec![
*left_span,
*right_span,
]),
jsx_props_no_spread_multiple_member_expressions_diagnostic(
vec![*left_span, *right_span],
member_prop_name,
),
|fixer| fixer.delete_range(*left_span),
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,19 @@ use crate::{

fn void_dom_elements_no_children_diagnostic(tag: &str, span: Span) -> OxcDiagnostic {
// TODO: use imperative phrasing
OxcDiagnostic::warn(
"Disallow void DOM elements (e.g. `<img />`, `<br />`) from receiving children.",
)
.with_help(format!("Void DOM element <{tag:?} /> cannot receive children."))
.with_label(span)
OxcDiagnostic::warn(format!("Void DOM element <{tag:?} /> cannot receive children."))
.with_help("Remove this element's children or use a non-void element.")
.with_label(span)
}

#[derive(Debug, Default, Clone)]
pub struct VoidDomElementsNoChildren;

declare_oxc_lint!(
/// ### What it does
/// Disallow void DOM elements (e.g. `<img />`, `<br />`) from receiving children.
///
/// ### Why is this bad?
/// There are some HTML elements that are only self-closing (e.g. img, br, hr). These are collectively known as void DOM elements.
/// This rule checks that children are not passed to void DOM elements.
///
Expand Down
4 changes: 1 addition & 3 deletions crates/oxc_linter/src/rules/typescript/no_dynamic_delete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,7 @@ declare_oxc_lint!(
);

fn no_dynamic_delete_diagnostic(span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Do not delete dynamically computed property keys.")
.with_help("Disallow using the `delete` operator on computed key expressions")
.with_label(span)
OxcDiagnostic::warn("Do not delete dynamically computed property keys.").with_label(span)
}

impl Rule for NoDynamicDelete {
Expand Down
26 changes: 21 additions & 5 deletions crates/oxc_linter/src/rules/typescript/no_useless_empty_export.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_useless_empty_export_diagnostic(span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow empty exports that don't change anything in a module file")
.with_help("Empty export does nothing and can be removed.")
OxcDiagnostic::warn("Empty exports do nothing in module files")
.with_help("Remove this empty export.")
.with_label(span)
}

Expand All @@ -19,19 +19,35 @@ declare_oxc_lint!(
///
/// Disallow empty exports that don't change anything in a module file.
///
/// ## Why is this bad?
/// An empty `export {}` statement is sometimes useful in TypeScript code to
/// turn a file that would otherwise be a script file into a module file.
/// Per the [TypeScript Handbook Modules page](https://www.typescriptlang.org/docs/handbook/modules/introduction.html):
///
/// In TypeScript, just as in ECMAScript 2015, any file containing a
/// top-level import or export is considered a module. Conversely, a file
/// without any top-level import or export declarations is treated as a
/// script whose contents are available in the global scope (and therefore
/// to modules as well).
///
/// However, an `export {}` statement does nothing if there are any other
/// top-level import or export statements in a file.
///
/// This rule reports an `export {}` that doesn't do anything in a file
/// already using ES modules.
///
/// ### Example
///
/// ### Bad
/// Examples of **incorrect** code for this rule:
/// ```ts
/// export const value = 'Hello, world!';
/// export {};
/// ```
///
/// ### Good
/// Examples of **correct** code for this rule:
/// ```ts
/// export const value = 'Hello, world!';
/// ```
///
NoUselessEmptyExport,
correctness,
fix
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ use oxc_span::Span;
use crate::{context::LintContext, rule::Rule, AstNode};

fn no_anonymous_default_export_diagnostic(span: Span, kind: ErrorNodeKind) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow anonymous functions and classes as the default export")
.with_help(format!("The {kind} should be named."))
OxcDiagnostic::warn(format!("This {kind} default export is missing a name"))
// TODO: suggest a name. https://github.com/sindresorhus/eslint-plugin-unicorn/blob/d3e4b805da31c6ed7275e2e2e770b6b0fbcf11c2/rules/no-anonymous-default-export.js#L41
.with_label(span)
}

Expand All @@ -24,7 +24,9 @@ declare_oxc_lint!(
/// Disallow anonymous functions and classes as the default export
///
/// ### Why is this bad?
/// Naming default exports improves codebase searchability by ensuring consistent identifier use for a module's default export, both where it's declared and where it's imported.
/// Naming default exports improves codebase searchability by ensuring
/// consistent identifier use for a module's default export, both where it's
/// declared and where it's imported.
///
/// ### Example
///
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ use oxc_span::{GetSpan, Span};
use crate::{context::LintContext, rule::Rule, AstNode};

fn no_await_expression_member_diagnostic(span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow member access from await expression")
.with_help("When accessing a member from an await expression, the await expression has to be parenthesized, which is not readable.")
OxcDiagnostic::warn("Do not access a member directly from an await expression.")
.with_help("Assign the result of the await expression to a variable, then access the member from that variable.")
.with_label(span)
}

Expand All @@ -17,12 +17,12 @@ pub struct NoAwaitExpressionMember;
declare_oxc_lint!(
/// ### What it does
///
/// This rule disallows member access from await expression
/// Disallows member access from `await` expressions.
///
/// ### Why is this bad?
///
/// When accessing a member from an await expression,
/// the await expression has to be parenthesized, which is not readable.
/// When accessing a member from an `await` expression,
/// the `await` expression has to be parenthesized, which is not readable.
///
/// ### Example
/// ```javascript
Expand All @@ -35,7 +35,8 @@ declare_oxc_lint!(
/// }
/// ```
NoAwaitExpressionMember,
style
style,
pending
);

impl Rule for NoAwaitExpressionMember {
Expand Down
19 changes: 15 additions & 4 deletions crates/oxc_linter/src/rules/unicorn/no_static_only_class.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ use oxc_span::Span;
use crate::{context::LintContext, rule::Rule, AstNode};

fn no_static_only_class_diagnostic(span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow classes that only have static members.")
.with_help("A class with only static members could just be an object instead.")
OxcDiagnostic::warn("Use an object instead of a class with only static members.")
.with_label(span)
}

Expand All @@ -23,7 +22,6 @@ declare_oxc_lint!(
///
/// A class with only static members could just be an object instead.
///
///
/// ### Example
///
/// Examples of **incorrect** code for this rule:
Expand All @@ -41,8 +39,21 @@ declare_oxc_lint!(
/// constructor() {}
/// }
/// ```
/// ```javascript
/// const X = {
/// foo: false,
/// bar() {}
/// };
/// ```
/// ```javascript
/// class X {
/// static #foo = false; // private field
/// static bar() {}
/// }
/// ```
NoStaticOnlyClass,
pedantic
pedantic,
pending
);

impl Rule for NoStaticOnlyClass {
Expand Down
24 changes: 8 additions & 16 deletions crates/oxc_linter/src/snapshots/jsx_no_undef.snap
Original file line number Diff line number Diff line change
@@ -1,58 +1,50 @@
---
source: crates/oxc_linter/src/tester.rs
---
⚠ eslint-plugin-react(jsx-no-undef): Disallow undeclared variables in JSX
⚠ eslint-plugin-react(jsx-no-undef): 'App' is not defined.
╭─[jsx_no_undef.tsx:1:26]
1 │ var React; React.render(<App />);
· ───
╰────
help: 'App' is not defined.

⚠ eslint-plugin-react(jsx-no-undef): Disallow undeclared variables in JSX
⚠ eslint-plugin-react(jsx-no-undef): 'Appp' is not defined.
╭─[jsx_no_undef.tsx:1:26]
1 │ var React; React.render(<Appp.Foo />);
· ────
╰────
help: 'Appp' is not defined.

⚠ eslint-plugin-react(jsx-no-undef): Disallow undeclared variables in JSX
⚠ eslint-plugin-react(jsx-no-undef): 'appp' is not defined.
╭─[jsx_no_undef.tsx:1:26]
1 │ var React; React.render(<appp.Foo />);
· ────
╰────
help: 'appp' is not defined.

⚠ eslint-plugin-react(jsx-no-undef): Disallow undeclared variables in JSX
⚠ eslint-plugin-react(jsx-no-undef): 'appp' is not defined.
╭─[jsx_no_undef.tsx:1:26]
1 │ var React; React.render(<appp.foo.Bar />);
· ────
╰────
help: 'appp' is not defined.

⚠ eslint-plugin-react(jsx-no-undef): Disallow undeclared variables in JSX
⚠ eslint-plugin-react(jsx-no-undef): 'Foo' is not defined.
╭─[jsx_no_undef.tsx:1:26]
1 │ var React; React.render(<Foo />);
· ───
╰────
help: 'Foo' is not defined.

⚠ eslint-plugin-react(jsx-no-undef): Disallow undeclared variables in JSX
⚠ eslint-plugin-react(jsx-no-undef): 'Unknown' is not defined.
╭─[jsx_no_undef.tsx:1:35]
1 │ var React; Unknown; React.render(<Unknown />)
· ───────
╰────
help: 'Unknown' is not defined.

⚠ eslint-plugin-react(jsx-no-undef): Disallow undeclared variables in JSX
⚠ eslint-plugin-react(jsx-no-undef): 'App' is not defined.
╭─[jsx_no_undef.tsx:1:49]
1 │ var React; { const App = null; }; React.render(<App />);
· ───
╰────
help: 'App' is not defined.

⚠ eslint-plugin-react(jsx-no-undef): Disallow undeclared variables in JSX
⚠ eslint-plugin-react(jsx-no-undef): 'App' is not defined.
╭─[jsx_no_undef.tsx:1:42]
1 │ var React; enum A { App }; React.render(<App />);
· ───
╰────
help: 'App' is not defined.
Loading