From 854bc95b57bf990376bac34063c66e65fb185ee6 Mon Sep 17 00:00:00 2001 From: makotot Date: Thu, 17 Aug 2023 02:11:18 +0900 Subject: [PATCH 1/3] feat(linter): implement no-unsafe-declaration-merging --- crates/oxc_linter/src/rules.rs | 2 + .../no_unsafe_declaration_merging.rs | 178 ++++++++++++++++++ .../no_unsafe_declaration_merging.snap | 29 +++ 3 files changed, 209 insertions(+) create mode 100644 crates/oxc_linter/src/rules/typescript/no_unsafe_declaration_merging.rs create mode 100644 crates/oxc_linter/src/snapshots/no_unsafe_declaration_merging.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 8eb344df0dc83..d1f8473bc6482 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -86,6 +86,7 @@ mod typescript { pub mod no_non_null_asserted_optional_chain; pub mod no_this_alias; pub mod no_unnecessary_type_constraint; + pub mod no_unsafe_declaration_merging; pub mod no_var_requires; pub mod prefer_as_const; } @@ -170,6 +171,7 @@ oxc_macros::declare_all_lint_rules! { typescript::no_extra_non_null_assertion, typescript::no_non_null_asserted_optional_chain, typescript::no_unnecessary_type_constraint, + typescript::no_unsafe_declaration_merging, typescript::no_misused_new, typescript::no_this_alias, typescript::no_namespace, diff --git a/crates/oxc_linter/src/rules/typescript/no_unsafe_declaration_merging.rs b/crates/oxc_linter/src/rules/typescript/no_unsafe_declaration_merging.rs new file mode 100644 index 0000000000000..ddfc98a9a24da --- /dev/null +++ b/crates/oxc_linter/src/rules/typescript/no_unsafe_declaration_merging.rs @@ -0,0 +1,178 @@ +use oxc_ast::AstKind; +use oxc_diagnostics::{ + miette::{self, Diagnostic}, + thiserror::Error, +}; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + +use crate::{context::LintContext, rule::Rule, AstNode}; + +#[derive(Debug, Error, Diagnostic)] +#[error("typescript-eslint(no-unsafe-declaration-merging): Unsafe declaration merging between classes and interfaces.")] +#[diagnostic(severity(warning))] +struct NoUnsafeDeclarationMergingDiagnostic(#[label] pub Span); + +#[derive(Debug, Default, Clone)] +pub struct NoUnsafeDeclarationMerging; + +declare_oxc_lint!( + /// ### What it does + /// + /// Disallow unsafe declaration merging. + /// + /// ### Why is this bad? + /// + /// Declaration merging between classes and interfaces is unsafe. + /// The TypeScript compiler doesn't check whether properties are initialized, which can cause lead to TypeScript not detecting code that will cause runtime errors. + /// + /// ### Example + /// ```javascript + /// interface Foo {} + /// class Foo {} + /// ``` + NoUnsafeDeclarationMerging, + correctness +); + +impl Rule for NoUnsafeDeclarationMerging { + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let scope_symbols = ctx.semantic().scopes().get_bindings(node.scope_id()); + + match node.kind() { + AstKind::Class(decl) => { + if let Some(ident) = decl.id.as_ref() { + for scope_symbol in scope_symbols { + if let AstKind::TSInterfaceDeclaration(scope_decl) = ctx + .nodes() + .get_node(ctx.symbols().get_declaration(*scope_symbol.1)) + .kind() + { + if ident.name.as_str() == scope_decl.id.name.as_str() { + ctx.diagnostic(NoUnsafeDeclarationMergingDiagnostic(decl.span)); + } + } + } + } + } + AstKind::TSInterfaceDeclaration(decl) => { + for scope_symbol in scope_symbols { + if let AstKind::Class(scope_decl) = + ctx.nodes().get_node(ctx.symbols().get_declaration(*scope_symbol.1)).kind() + { + if let Some(scope_ident) = scope_decl.id.as_ref() { + if scope_ident.name.as_str() == decl.id.name.as_str() { + ctx.diagnostic(NoUnsafeDeclarationMergingDiagnostic(decl.span)); + } + } + } + } + } + _ => {} + } + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + ( + " + interface Foo {} + class Bar implements Foo {} + ", + None, + ), + ( + " + namespace Foo {} + namespace Foo {} + ", + None, + ), + ( + " + enum Foo {} + namespace Foo {} + ", + None, + ), + ( + " + namespace Fooo {} + function Foo() {} + ", + None, + ), + ( + " + const Foo = class {}; + ", + None, + ), + ( + " + interface Foo { + props: string; + } + + function bar() { + return class Foo {}; + } + ", + None, + ), + ( + " + interface Foo { + props: string; + } + + (function bar() { + class Foo {} + })(); + ", + None, + ), + ( + " + declare global { + interface Foo {} + } + + class Foo {} + ", + None, + ), + ]; + + let fail = vec![ + ( + " + interface Foo {} + class Foo {} + ", + None, + ), + ( + " + class Foo {} + interface Foo {} + ", + None, + ), + ( + " + declare global { + interface Foo {} + class Foo {} + } + ", + None, + ), + ]; + + Tester::new(NoUnsafeDeclarationMerging::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/no_unsafe_declaration_merging.snap b/crates/oxc_linter/src/snapshots/no_unsafe_declaration_merging.snap new file mode 100644 index 0000000000000..3136fde24e454 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_unsafe_declaration_merging.snap @@ -0,0 +1,29 @@ +--- +source: crates/oxc_linter/src/tester.rs +expression: no_unsafe_declaration_merging +--- + ⚠ typescript-eslint(no-unsafe-declaration-merging): Unsafe declaration merging between classes and interfaces. + ╭─[no_unsafe_declaration_merging.tsx:2:1] + 2 │ interface Foo {} + 3 │ class Foo {} + · ──────────── + 4 │ + ╰──── + + ⚠ typescript-eslint(no-unsafe-declaration-merging): Unsafe declaration merging between classes and interfaces. + ╭─[no_unsafe_declaration_merging.tsx:2:1] + 2 │ class Foo {} + 3 │ interface Foo {} + · ──────────────── + 4 │ + ╰──── + + ⚠ typescript-eslint(no-unsafe-declaration-merging): Unsafe declaration merging between classes and interfaces. + ╭─[no_unsafe_declaration_merging.tsx:3:1] + 3 │ interface Foo {} + 4 │ class Foo {} + · ──────────── + 5 │ } + ╰──── + + From 593d3cced9fb9e174de92e3670359579622e8f81 Mon Sep 17 00:00:00 2001 From: makotot Date: Tue, 22 Aug 2023 01:19:43 +0900 Subject: [PATCH 2/3] refactor: clean up --- .../no_unsafe_declaration_merging.rs | 42 ++++++++++++------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/crates/oxc_linter/src/rules/typescript/no_unsafe_declaration_merging.rs b/crates/oxc_linter/src/rules/typescript/no_unsafe_declaration_merging.rs index ddfc98a9a24da..e55c0aa1aa8e8 100644 --- a/crates/oxc_linter/src/rules/typescript/no_unsafe_declaration_merging.rs +++ b/crates/oxc_linter/src/rules/typescript/no_unsafe_declaration_merging.rs @@ -1,9 +1,10 @@ -use oxc_ast::AstKind; +use oxc_ast::{ast::BindingIdentifier, AstKind}; use oxc_diagnostics::{ miette::{self, Diagnostic}, thiserror::Error, }; use oxc_macros::declare_oxc_lint; +use oxc_semantic::SymbolId; use oxc_span::Span; use crate::{context::LintContext, rule::Rule, AstNode}; @@ -37,33 +38,29 @@ declare_oxc_lint!( impl Rule for NoUnsafeDeclarationMerging { fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + if !ctx.source_type().is_typescript() { + return; + } + let scope_symbols = ctx.semantic().scopes().get_bindings(node.scope_id()); match node.kind() { AstKind::Class(decl) => { if let Some(ident) = decl.id.as_ref() { for scope_symbol in scope_symbols { - if let AstKind::TSInterfaceDeclaration(scope_decl) = ctx - .nodes() - .get_node(ctx.symbols().get_declaration(*scope_symbol.1)) - .kind() + if let AstKind::TSInterfaceDeclaration(scope_interface) = + get_symbol_kind(*scope_symbol.1, ctx) { - if ident.name.as_str() == scope_decl.id.name.as_str() { - ctx.diagnostic(NoUnsafeDeclarationMergingDiagnostic(decl.span)); - } + check_and_diagnostic(ident, &scope_interface.id, decl.span, ctx); } } } } AstKind::TSInterfaceDeclaration(decl) => { for scope_symbol in scope_symbols { - if let AstKind::Class(scope_decl) = - ctx.nodes().get_node(ctx.symbols().get_declaration(*scope_symbol.1)).kind() - { - if let Some(scope_ident) = scope_decl.id.as_ref() { - if scope_ident.name.as_str() == decl.id.name.as_str() { - ctx.diagnostic(NoUnsafeDeclarationMergingDiagnostic(decl.span)); - } + if let AstKind::Class(scope_class) = get_symbol_kind(*scope_symbol.1, ctx) { + if let Some(scope_class_ident) = scope_class.id.as_ref() { + check_and_diagnostic(&decl.id, scope_class_ident, decl.span, ctx); } } } @@ -73,6 +70,21 @@ impl Rule for NoUnsafeDeclarationMerging { } } +fn check_and_diagnostic( + ident: &BindingIdentifier, + scope_ident: &BindingIdentifier, + span: Span, + ctx: &LintContext<'_>, +) { + if scope_ident.name.as_str() == ident.name.as_str() { + ctx.diagnostic(NoUnsafeDeclarationMergingDiagnostic(span)); + } +} + +fn get_symbol_kind<'a>(symbol_id: SymbolId, ctx: &LintContext<'a>) -> AstKind<'a> { + return ctx.nodes().get_node(ctx.symbols().get_declaration(symbol_id)).kind(); +} + #[test] fn test() { use crate::tester::Tester; From 4adf76fbb5576eb9c88568428059da77f407825f Mon Sep 17 00:00:00 2001 From: makotot Date: Tue, 22 Aug 2023 23:00:22 +0900 Subject: [PATCH 3/3] feat: improve diagnostic ux & performance --- .../no_unsafe_declaration_merging.rs | 21 ++++++++----------- .../no_unsafe_declaration_merging.snap | 21 +++++++++++++------ 2 files changed, 24 insertions(+), 18 deletions(-) diff --git a/crates/oxc_linter/src/rules/typescript/no_unsafe_declaration_merging.rs b/crates/oxc_linter/src/rules/typescript/no_unsafe_declaration_merging.rs index e55c0aa1aa8e8..610a2451b8f4d 100644 --- a/crates/oxc_linter/src/rules/typescript/no_unsafe_declaration_merging.rs +++ b/crates/oxc_linter/src/rules/typescript/no_unsafe_declaration_merging.rs @@ -11,8 +11,8 @@ use crate::{context::LintContext, rule::Rule, AstNode}; #[derive(Debug, Error, Diagnostic)] #[error("typescript-eslint(no-unsafe-declaration-merging): Unsafe declaration merging between classes and interfaces.")] -#[diagnostic(severity(warning))] -struct NoUnsafeDeclarationMergingDiagnostic(#[label] pub Span); +#[diagnostic(severity(warning), help("The TypeScript compiler doesn't check whether properties are initialized, which can cause lead to TypeScript not detecting code that will cause runtime errors."))] +struct NoUnsafeDeclarationMergingDiagnostic(#[label] Span, #[label] Span); #[derive(Debug, Default, Clone)] pub struct NoUnsafeDeclarationMerging; @@ -42,25 +42,23 @@ impl Rule for NoUnsafeDeclarationMerging { return; } - let scope_symbols = ctx.semantic().scopes().get_bindings(node.scope_id()); - match node.kind() { AstKind::Class(decl) => { if let Some(ident) = decl.id.as_ref() { - for scope_symbol in scope_symbols { + for (_, symbol_id) in ctx.semantic().scopes().get_bindings(node.scope_id()) { if let AstKind::TSInterfaceDeclaration(scope_interface) = - get_symbol_kind(*scope_symbol.1, ctx) + get_symbol_kind(*symbol_id, ctx) { - check_and_diagnostic(ident, &scope_interface.id, decl.span, ctx); + check_and_diagnostic(ident, &scope_interface.id, ctx); } } } } AstKind::TSInterfaceDeclaration(decl) => { - for scope_symbol in scope_symbols { - if let AstKind::Class(scope_class) = get_symbol_kind(*scope_symbol.1, ctx) { + for (_, symbol_id) in ctx.semantic().scopes().get_bindings(node.scope_id()) { + if let AstKind::Class(scope_class) = get_symbol_kind(*symbol_id, ctx) { if let Some(scope_class_ident) = scope_class.id.as_ref() { - check_and_diagnostic(&decl.id, scope_class_ident, decl.span, ctx); + check_and_diagnostic(&decl.id, scope_class_ident, ctx); } } } @@ -73,11 +71,10 @@ impl Rule for NoUnsafeDeclarationMerging { fn check_and_diagnostic( ident: &BindingIdentifier, scope_ident: &BindingIdentifier, - span: Span, ctx: &LintContext<'_>, ) { if scope_ident.name.as_str() == ident.name.as_str() { - ctx.diagnostic(NoUnsafeDeclarationMergingDiagnostic(span)); + ctx.diagnostic(NoUnsafeDeclarationMergingDiagnostic(ident.span, scope_ident.span)); } } diff --git a/crates/oxc_linter/src/snapshots/no_unsafe_declaration_merging.snap b/crates/oxc_linter/src/snapshots/no_unsafe_declaration_merging.snap index 3136fde24e454..1c3144193f5c6 100644 --- a/crates/oxc_linter/src/snapshots/no_unsafe_declaration_merging.snap +++ b/crates/oxc_linter/src/snapshots/no_unsafe_declaration_merging.snap @@ -3,27 +3,36 @@ source: crates/oxc_linter/src/tester.rs expression: no_unsafe_declaration_merging --- ⚠ typescript-eslint(no-unsafe-declaration-merging): Unsafe declaration merging between classes and interfaces. - ╭─[no_unsafe_declaration_merging.tsx:2:1] + ╭─[no_unsafe_declaration_merging.tsx:1:1] + 1 │ 2 │ interface Foo {} + · ─── 3 │ class Foo {} - · ──────────── + · ─── 4 │ ╰──── + help: The TypeScript compiler doesn't check whether properties are initialized, which can cause lead to TypeScript not detecting code that will cause runtime errors. ⚠ typescript-eslint(no-unsafe-declaration-merging): Unsafe declaration merging between classes and interfaces. - ╭─[no_unsafe_declaration_merging.tsx:2:1] + ╭─[no_unsafe_declaration_merging.tsx:1:1] + 1 │ 2 │ class Foo {} + · ─── 3 │ interface Foo {} - · ──────────────── + · ─── 4 │ ╰──── + help: The TypeScript compiler doesn't check whether properties are initialized, which can cause lead to TypeScript not detecting code that will cause runtime errors. ⚠ typescript-eslint(no-unsafe-declaration-merging): Unsafe declaration merging between classes and interfaces. - ╭─[no_unsafe_declaration_merging.tsx:3:1] + ╭─[no_unsafe_declaration_merging.tsx:2:1] + 2 │ declare global { 3 │ interface Foo {} + · ─── 4 │ class Foo {} - · ──────────── + · ─── 5 │ } ╰──── + help: The TypeScript compiler doesn't check whether properties are initialized, which can cause lead to TypeScript not detecting code that will cause runtime errors.