Skip to content

Commit

Permalink
fix(linter): improve diagnostic messages for various lint rules
Browse files Browse the repository at this point in the history
This is the first of a few PRs to remove "Disallow <x>" from all diagnostic
messages. More to come.
  • Loading branch information
DonIsaac committed Sep 16, 2024
1 parent 2c1cc99 commit ef2b1f6
Show file tree
Hide file tree
Showing 20 changed files with 231 additions and 218 deletions.
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]
1var 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]
1var 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]
1var 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]
1var 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]
1var 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]
1var 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]
1var 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]
1var React; enum A { App }; React.render(<App />);
· ───
╰────
help: 'App' is not defined.
Loading

0 comments on commit ef2b1f6

Please sign in to comment.