Skip to content

Commit 82889ae

Browse files
DonIsaacDonIsaac
committed
fix(linter/no-extraneous-class): improve docs, reporting and code refactor (#10797)
## What This PR Does Deviates from `@typescript-eslint/no-extraneous-class` by allowing empty, decorated classes by default. This provides a better experience for NestJS user using Oxlint without a config file. In NestJS, [`Module`s are often empty but use decorators to declaratively state their contents](https://docs.nestjs.com/modules#feature-modules). Other similar frameworks (TSeD, maybe Angular) will also benefit. ### Other Changes - Fix span on empty class diagnostics to not cover decorators. - Provide help messages on all diagnostics - Add missing links and rich text stuff to docs
1 parent b215b6c commit 82889ae

File tree

2 files changed

+120
-138
lines changed

2 files changed

+120
-138
lines changed

‎crates/oxc_linter/src/rules/typescript/no_extraneous_class.rs‎

Lines changed: 83 additions & 107 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,12 @@ use oxc_ast::{
55
use oxc_diagnostics::OxcDiagnostic;
66
use oxc_macros::declare_oxc_lint;
77
use oxc_span::Span;
8+
use schemars::JsonSchema;
89

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

11-
#[derive(Debug, Default, Clone)]
12+
#[derive(Debug, Clone, Default, JsonSchema)]
13+
#[serde(renameAll = "camelCase", default)]
1214
pub struct NoExtraneousClass {
1315
allow_constructor_only: bool,
1416
allow_empty: bool,
@@ -19,26 +21,33 @@ pub struct NoExtraneousClass {
1921
declare_oxc_lint!(
2022
/// ### What it does
2123
///
22-
/// This rule reports when a class has no non-static members,
23-
/// such as for a class used exclusively as a static namespace.
24-
/// This rule also reports classes that have only a constructor and no fields.
25-
/// Those classes can generally be replaced with a standalone function.
24+
/// This rule reports when a class has no non-static members, such as for a
25+
/// class used exclusively as a static namespace. This rule also reports
26+
/// classes that have only a constructor and no fields. Those classes can
27+
/// generally be replaced with a standalone function.
2628
///
2729
/// ### Why is this bad?
2830
///
29-
/// Users who come from a OOP paradigm may wrap their utility functions in an extra class,
30-
/// instead of putting them at the top level of an ECMAScript module.
31-
/// Doing so is generally unnecessary in JavaScript and TypeScript projects.
31+
/// Users who come from a OOP paradigm may wrap their utility functions in
32+
/// an extra class, instead of putting them at the top level of an
33+
/// ECMAScript module. Doing so is generally unnecessary in JavaScript and
34+
/// TypeScript projects.
3235
///
33-
/// Wrapper classes add extra cognitive complexity to code without adding any structural improvements
36+
/// * Wrapper classes add extra cognitive complexity to code without adding
37+
/// any structural improvements
38+
/// * Whatever would be put on them, such as utility functions, are already
39+
/// organized by virtue of being in a module.
40+
/// * As an alternative, you can `import * as ...` the module to get all of them
41+
/// in a single object.
42+
/// * IDEs can't provide as good suggestions for static class or namespace
43+
/// imported properties when you start typing property names
44+
/// * It's more difficult to statically analyze code for unused variables,
45+
/// etc. when they're all on the class (see: [Finding dead code (and dead
46+
/// types) in TypeScript](https://effectivetypescript.com/2020/10/20/tsprune/)).
3447
///
35-
/// Whatever would be put on them, such as utility functions, are already organized by virtue of being in a module.
36-
///
37-
/// As an alternative, you can import * as ... the module to get all of them in a single object.
38-
/// IDEs can't provide as good suggestions for static class or namespace imported properties when you start typing property names
39-
///
40-
/// It's more difficult to statically analyze code for unused variables, etc.
41-
/// when they're all on the class (see: Finding dead code (and dead types) in TypeScript).
48+
/// This rule also reports classes that have only a constructor and no
49+
/// fields. Those classes can generally be replaced with a standalone
50+
/// function.
4251
///
4352
/// ### Example
4453
/// ```ts
@@ -63,16 +72,27 @@ declare_oxc_lint!(
6372
suspicious
6473
);
6574

66-
fn empty_no_extraneous_class_diagnostic(span: Span) -> OxcDiagnostic {
67-
OxcDiagnostic::warn("Unexpected empty class.").with_label(span)
75+
fn empty_class_diagnostic(span: Span, has_decorators: bool) -> OxcDiagnostic {
76+
let diagnostic = OxcDiagnostic::warn("Unexpected empty class.").with_label(span);
77+
if has_decorators {
78+
diagnostic.with_help(
79+
r#"Set "allowWithDecorator": true in your config to allow empty decorated classes"#,
80+
)
81+
} else {
82+
diagnostic
83+
}
6884
}
6985

7086
fn only_static_no_extraneous_class_diagnostic(span: Span) -> OxcDiagnostic {
71-
OxcDiagnostic::warn("Unexpected class with only static properties.").with_label(span)
87+
OxcDiagnostic::warn("Unexpected class with only static properties.")
88+
.with_label(span)
89+
.with_help("Try using standalone functions instead of static methods")
7290
}
7391

7492
fn only_constructor_no_extraneous_class_diagnostic(span: Span) -> OxcDiagnostic {
75-
OxcDiagnostic::warn("Unexpected class with only a constructor.").with_label(span)
93+
OxcDiagnostic::warn("Unexpected class with only a constructor.")
94+
.with_label(span)
95+
.with_help("Try replacing this class with a standalone function or deleting it entirely")
7696
}
7797

7898
impl Rule for NoExtraneousClass {
@@ -115,7 +135,19 @@ impl Rule for NoExtraneousClass {
115135
match body.as_slice() {
116136
[] => {
117137
if !self.allow_empty {
118-
ctx.diagnostic(empty_no_extraneous_class_diagnostic(class.span));
138+
let mut span = class.span;
139+
#[expect(clippy::checked_conversions, clippy::cast_possible_truncation)]
140+
if let Some(decorator) = class.decorators.last() {
141+
span = Span::new(decorator.span.end, span.end);
142+
// NOTE: there will always be a 'c' because of 'class' keyword.
143+
let start = ctx.source_range(span).find('c').unwrap();
144+
// SAFETY: source files are guaranteed to be less than
145+
// 2^32 characters, so conversion will never fail. Using
146+
// unchecked assert here removes a useless bounds check.
147+
unsafe { std::hint::assert_unchecked(start <= u32::MAX as usize) };
148+
span = span.shrink_left(start as u32);
149+
}
150+
ctx.diagnostic(empty_class_diagnostic(span, !class.decorators.is_empty()));
119151
}
120152
}
121153
[ClassElement::MethodDefinition(constructor)] if constructor.kind.is_constructor() => {
@@ -138,6 +170,7 @@ impl Rule for NoExtraneousClass {
138170
#[test]
139171
fn test() {
140172
use crate::tester::Tester;
173+
use serde_json::json;
141174

142175
let pass = vec![
143176
(
@@ -146,7 +179,7 @@ fn test() {
146179
public prop = 1;
147180
constructor() {}
148181
}
149-
",
182+
",
150183
None,
151184
),
152185
(
@@ -158,26 +191,12 @@ fn test() {
158191
}
159192
constructor() {}
160193
}
161-
",
162-
None,
163-
),
164-
(
165-
"
166-
class Foo {
167-
constructor(public bar: string) {}
168-
}
169-
",
194+
",
170195
None,
171196
),
172-
("class Foo {}", Some(serde_json::json!([{ "allowEmpty": true }]))),
173-
(
174-
"
175-
class Foo {
176-
constructor() {}
177-
}
178-
",
179-
Some(serde_json::json!([{ "allowConstructorOnly": true }])),
180-
),
197+
("class Foo { constructor(public bar: string) {} }", None),
198+
("class Foo {}", Some(json!([{ "allowEmpty": true }]))),
199+
("class Foo { constructor() {} }", Some(json!([{ "allowConstructorOnly": true }]))),
181200
(
182201
"
183202
export class Bar {
@@ -187,7 +206,7 @@ fn test() {
187206
}
188207
}
189208
",
190-
Some(serde_json::json!([{ "allowStaticOnly": true }])),
209+
Some(json!([{ "allowStaticOnly": true }])),
191210
),
192211
(
193212
"
@@ -196,16 +215,10 @@ fn test() {
196215
return 'I am foo!';
197216
}
198217
}
199-
",
218+
",
200219
None,
201220
),
202-
(
203-
"
204-
@FooDecorator
205-
class Foo {}
206-
",
207-
Some(serde_json::json!([{ "allowWithDecorator": true }])),
208-
),
221+
("@FooDecorator class Foo {} ", Some(json!([{ "allowWithDecorator": true }]))),
209222
(
210223
"
211224
@FooDecorator
@@ -216,25 +229,11 @@ fn test() {
216229
});
217230
}
218231
}
219-
",
220-
Some(serde_json::json!([{ "allowWithDecorator": true }])),
221-
),
222-
(
223-
"
224-
abstract class Foo {
225-
abstract property: string;
226-
}
227-
",
228-
None,
229-
),
230-
(
231-
"
232-
abstract class Foo {
233-
abstract method(): string;
234-
}
235-
",
236-
None,
232+
",
233+
Some(json!([{ "allowWithDecorator": true }])),
237234
),
235+
("abstract class Foo { abstract property: string; }", None),
236+
("abstract class Foo { abstract method(): string; }", None),
238237
];
239238

240239
let fail = vec![
@@ -258,14 +257,7 @@ fn test() {
258257
",
259258
None,
260259
),
261-
(
262-
"
263-
class Foo {
264-
constructor() {}
265-
}
266-
",
267-
None,
268-
),
260+
("class Foo { constructor() {} }", None),
269261
(
270262
"
271263
export class AClass {
@@ -280,20 +272,23 @@ fn test() {
280272
",
281273
None,
282274
),
275+
("export default class { static hello() {} }", None),
283276
(
284277
"
285-
export default class {
286-
static hello() {}
287-
}
288-
",
289-
None,
278+
@FooDecorator
279+
class Foo {}
280+
",
281+
Some(json!([{ "allowWithDecorator": false }])),
290282
),
291283
(
292284
"
293-
@FooDecorator
285+
@FooDecorator({
286+
wowThisDecoratorIsQuiteLarge: true,
287+
itShouldNotBeIncludedIn: 'the diagnostic span',
288+
})
294289
class Foo {}
295-
",
296-
Some(serde_json::json!([{ "allowWithDecorator": false }])),
290+
",
291+
Some(json!([{ "allowWithDecorator": false }])),
297292
),
298293
(
299294
"
@@ -305,31 +300,12 @@ fn test() {
305300
});
306301
}
307302
}
308-
",
309-
Some(serde_json::json!([{ "allowWithDecorator": false }])),
310-
),
311-
(
312-
"
313-
abstract class Foo {}
314-
",
315-
None,
316-
),
317-
(
318-
"
319-
abstract class Foo {
320-
static property: string;
321-
}
322-
",
323-
None,
324-
),
325-
(
326-
"
327-
abstract class Foo {
328-
constructor() {}
329-
}
330-
",
331-
None,
303+
",
304+
Some(json!([{ "allowWithDecorator": false }])),
332305
),
306+
("abstract class Foo {}", None),
307+
("abstract class Foo { static property: string; }", None),
308+
("abstract class Foo { constructor() {} }", None),
333309
];
334310

335311
Tester::new(NoExtraneousClass::NAME, NoExtraneousClass::PLUGIN, pass, fail).test_and_snapshot();

0 commit comments

Comments
 (0)