diff --git a/crates/oxc_ast/src/ast_impl/js.rs b/crates/oxc_ast/src/ast_impl/js.rs index 4fdab1343fc05..20e1243c58a6e 100644 --- a/crates/oxc_ast/src/ast_impl/js.rs +++ b/crates/oxc_ast/src/ast_impl/js.rs @@ -1045,6 +1045,11 @@ impl<'a> FormalParameter<'a> { pub fn is_public(&self) -> bool { matches!(self.accessibility, Some(TSAccessibility::Public)) } + + #[inline] + pub fn has_modifier(&self) -> bool { + self.accessibility.is_some() || self.readonly || self.r#override + } } impl FormalParameterKind { diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index c9d85f32933bc..5e38ded477a7a 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -141,6 +141,7 @@ mod typescript { pub mod no_empty_interface; pub mod no_explicit_any; pub mod no_extra_non_null_assertion; + pub mod no_extraneous_class; pub mod no_import_type_side_effects; pub mod no_misused_new; pub mod no_namespace; @@ -571,6 +572,7 @@ oxc_macros::declare_all_lint_rules! { typescript::no_non_null_asserted_nullish_coalescing, typescript::no_confusing_non_null_assertion, typescript::no_dynamic_delete, + typescript::no_extraneous_class, jest::consistent_test_it, jest::expect_expect, jest::max_expects, diff --git a/crates/oxc_linter/src/rules/typescript/no_extraneous_class.rs b/crates/oxc_linter/src/rules/typescript/no_extraneous_class.rs new file mode 100644 index 0000000000000..2d6c5a02ae338 --- /dev/null +++ b/crates/oxc_linter/src/rules/typescript/no_extraneous_class.rs @@ -0,0 +1,342 @@ +use oxc_ast::{ + ast::{ClassElement, FormalParameter}, + AstKind, +}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + +use crate::{context::LintContext, rule::Rule, AstNode}; + +#[derive(Debug, Default, Clone)] +pub struct NoExtraneousClass { + allow_constructor_only: bool, + allow_empty: bool, + allow_static_only: bool, + allow_with_decorator: bool, +} + +declare_oxc_lint!( + /// ### What it does + /// + /// This rule reports when a class has no non-static members, + /// such as for a class used exclusively as a static namespace. + /// This rule also reports classes that have only a constructor and no fields. + /// Those classes can generally be replaced with a standalone function. + /// + /// ### Why is this bad? + /// + /// Users who come from a OOP paradigm may wrap their utility functions in an extra class, + /// instead of putting them at the top level of an ECMAScript module. + /// Doing so is generally unnecessary in JavaScript and TypeScript projects. + /// + /// Wrapper classes add extra cognitive complexity to code without adding any structural improvements + /// + /// Whatever would be put on them, such as utility functions, are already organized by virtue of being in a module. + /// + /// As an alternative, you can import * as ... the module to get all of them in a single object. + /// IDEs can't provide as good suggestions for static class or namespace imported properties when you start typing property names + /// + /// It's more difficult to statically analyze code for unused variables, etc. + /// when they're all on the class (see: Finding dead code (and dead types) in TypeScript). + /// + /// ### Example + /// ```javascript + /// class StaticConstants { + /// static readonly version = 42; + /// + /// static isProduction() { + /// return process.env.NODE_ENV === 'production'; + /// } + /// } + /// + /// class HelloWorldLogger { + /// constructor() { + /// console.log('Hello, world!'); + /// } + /// } + /// + /// abstract class Foo {} + /// ``` + NoExtraneousClass, + suspicious +); + +fn empty_no_extraneous_class_diagnostic(span: Span) -> OxcDiagnostic { + OxcDiagnostic::warn("typescript-eslint(no-extraneous-class): Unexpected empty class.") + .with_label(span) +} + +fn only_static_no_extraneous_class_diagnostic(span: Span) -> OxcDiagnostic { + OxcDiagnostic::warn( + "typescript-eslint(no-extraneous-class): Unexpected class with only static properties.", + ) + .with_label(span) +} + +fn only_constructor_no_extraneous_class_diagnostic(span: Span) -> OxcDiagnostic { + OxcDiagnostic::warn( + "typescript-eslint(no-extraneous-class): Unexpected class with only a constructor.", + ) + .with_label(span) +} + +impl Rule for NoExtraneousClass { + fn from_configuration(value: serde_json::Value) -> Self { + use serde_json::Value; + let Some(config) = value.get(0).and_then(Value::as_object) else { + return Self::default(); + }; + Self { + allow_constructor_only: config + .get("allowConstructorOnly") + .and_then(Value::as_bool) + .unwrap_or(false), + allow_empty: config + .get("allowEmpty") // lb + .and_then(Value::as_bool) + .unwrap_or(false), + allow_static_only: config + .get("allowStaticOnly") + .and_then(Value::as_bool) + .unwrap_or(false), + allow_with_decorator: config + .get("allowWithDecorator") + .and_then(Value::as_bool) + .unwrap_or(false), + } + } + + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let AstKind::Class(class) = node.kind() else { + return; + }; + if class.super_class.is_some() + || (self.allow_with_decorator && !class.decorators.is_empty()) + { + return; + } + let span = class.id.as_ref().map_or(class.span, |id| id.span); + let body = &class.body.body; + match body.as_slice() { + [] => { + if !self.allow_empty { + ctx.diagnostic(empty_no_extraneous_class_diagnostic(class.span)); + } + } + [ClassElement::MethodDefinition(constructor)] if constructor.kind.is_constructor() => { + let only_constructor = + !constructor.value.params.items.iter().any(FormalParameter::has_modifier); + if only_constructor && !self.allow_constructor_only { + ctx.diagnostic(only_constructor_no_extraneous_class_diagnostic(span)); + } + } + _ => { + let only_static = body.iter().all(|prop| prop.r#static() && !prop.is_abstract()); + if only_static && !self.allow_static_only { + ctx.diagnostic(only_static_no_extraneous_class_diagnostic(span)); + } + } + }; + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + ( + " + class Foo { + public prop = 1; + constructor() {} + } + ", + None, + ), + ( + " + export class CClass extends BaseClass { + public static helper(): void {} + private static privateHelper(): boolean { + return true; + } + constructor() {} + } + ", + None, + ), + ( + " + class Foo { + constructor(public bar: string) {} + } + ", + None, + ), + ("class Foo {}", Some(serde_json::json!([{ "allowEmpty": true }]))), + ( + " + class Foo { + constructor() {} + } + ", + Some(serde_json::json!([{ "allowConstructorOnly": true }])), + ), + ( + " + export class Bar { + public static helper(): void {} + private static privateHelper(): boolean { + return true; + } + } + ", + Some(serde_json::json!([{ "allowStaticOnly": true }])), + ), + ( + " + export default class { + hello() { + return 'I am foo!'; + } + } + ", + None, + ), + ( + " + @FooDecorator + class Foo {} + ", + Some(serde_json::json!([{ "allowWithDecorator": true }])), + ), + ( + " + @FooDecorator + class Foo { + constructor(foo: Foo) { + foo.subscribe(a => { + console.log(a); + }); + } + } + ", + Some(serde_json::json!([{ "allowWithDecorator": true }])), + ), + ( + " + abstract class Foo { + abstract property: string; + } + ", + None, + ), + ( + " + abstract class Foo { + abstract method(): string; + } + ", + None, + ), + ]; + + let fail = vec![ + ("class Foo {}", None), + ( + " + class Foo { + public prop = 1; + constructor() { + class Bar { + static PROP = 2; + } + } + } + export class Bar { + public static helper(): void {} + private static privateHelper(): boolean { + return true; + } + } + ", + None, + ), + ( + " + class Foo { + constructor() {} + } + ", + None, + ), + ( + " + export class AClass { + public static helper(): void {} + private static privateHelper(): boolean { + return true; + } + constructor() { + class nestedClass {} + } + } + ", + None, + ), + ( + " + export default class { + static hello() {} + } + ", + None, + ), + ( + " + @FooDecorator + class Foo {} + ", + Some(serde_json::json!([{ "allowWithDecorator": false }])), + ), + ( + " + @FooDecorator + class Foo { + constructor(foo: Foo) { + foo.subscribe(a => { + console.log(a); + }); + } + } + ", + Some(serde_json::json!([{ "allowWithDecorator": false }])), + ), + ( + " + abstract class Foo {} + ", + None, + ), + ( + " + abstract class Foo { + static property: string; + } + ", + None, + ), + ( + " + abstract class Foo { + constructor() {} + } + ", + None, + ), + ]; + + Tester::new(NoExtraneousClass::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/no_extraneous_class.snap b/crates/oxc_linter/src/snapshots/no_extraneous_class.snap new file mode 100644 index 0000000000000..4a8793cb361f1 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_extraneous_class.snap @@ -0,0 +1,89 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ typescript-eslint(no-extraneous-class): Unexpected empty class. + ╭─[no_extraneous_class.tsx:1:1] + 1 │ class Foo {} + · ──────────── + ╰──── + + ⚠ typescript-eslint(no-extraneous-class): Unexpected class with only static properties. + ╭─[no_extraneous_class.tsx:5:14] + 4 │ constructor() { + 5 │ class Bar { + · ─── + 6 │ static PROP = 2; + ╰──── + + ⚠ typescript-eslint(no-extraneous-class): Unexpected class with only static properties. + ╭─[no_extraneous_class.tsx:10:17] + 9 │ } + 10 │ export class Bar { + · ─── + 11 │ public static helper(): void {} + ╰──── + + ⚠ typescript-eslint(no-extraneous-class): Unexpected class with only a constructor. + ╭─[no_extraneous_class.tsx:2:10] + 1 │ + 2 │ class Foo { + · ─── + 3 │ constructor() {} + ╰──── + + ⚠ typescript-eslint(no-extraneous-class): Unexpected empty class. + ╭─[no_extraneous_class.tsx:8:8] + 7 │ constructor() { + 8 │ class nestedClass {} + · ──────────────────── + 9 │ } + ╰──── + + ⚠ typescript-eslint(no-extraneous-class): Unexpected class with only static properties. + ╭─[no_extraneous_class.tsx:2:19] + 1 │ + 2 │ ╭─▶ export default class { + 3 │ │ static hello() {} + 4 │ ╰─▶ } + 5 │ + ╰──── + + ⚠ typescript-eslint(no-extraneous-class): Unexpected empty class. + ╭─[no_extraneous_class.tsx:2:4] + 1 │ + 2 │ ╭─▶ @FooDecorator + 3 │ ╰─▶ class Foo {} + 4 │ + ╰──── + + ⚠ typescript-eslint(no-extraneous-class): Unexpected class with only a constructor. + ╭─[no_extraneous_class.tsx:3:10] + 2 │ @FooDecorator + 3 │ class Foo { + · ─── + 4 │ constructor(foo: Foo) { + ╰──── + + ⚠ typescript-eslint(no-extraneous-class): Unexpected empty class. + ╭─[no_extraneous_class.tsx:2:4] + 1 │ + 2 │ abstract class Foo {} + · ───────────────────── + 3 │ + ╰──── + + ⚠ typescript-eslint(no-extraneous-class): Unexpected class with only static properties. + ╭─[no_extraneous_class.tsx:2:19] + 1 │ + 2 │ abstract class Foo { + · ─── + 3 │ static property: string; + ╰──── + + ⚠ typescript-eslint(no-extraneous-class): Unexpected class with only a constructor. + ╭─[no_extraneous_class.tsx:2:19] + 1 │ + 2 │ abstract class Foo { + · ─── + 3 │ constructor() {} + ╰────