From 47e18eb4ad7cf766254dd81f4f10c75e91e2da67 Mon Sep 17 00:00:00 2001 From: Cameron McHenry Date: Thu, 5 Sep 2024 01:01:24 -0400 Subject: [PATCH 1/7] Add tests --- .../rules/eslint/no_unused_vars/tests/oxc.rs | 25 ++++++++++++++++--- .../no_unused_vars@oxc-functions.snap | 20 +++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs index 1d3b476ee3361..e64a69595f40a 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs @@ -451,7 +451,7 @@ fn test_functions() { " function foo(a: number): number; function foo(a: number | string): number { - return Number(a) + return Number(a) } foo(); ", @@ -476,13 +476,32 @@ fn test_functions() { }); ", "const foo = () => function bar() { }\nfoo()", - "module.exports.foo = () => function bar() { }" + "module.exports.foo = () => function bar() { }", + // https://github.com/oxc-project/oxc/issues/5406 + " + export function log(message: string, ...interpolations: unknown[]): void; + export function log(message: string, ...interpolations: unknown[]): void { + console.log(message, interpolations); + } + ", + "declare function func(strings: any, ...values: any[]): object" ]; let fail = vec![ "function foo() {}", "function foo() { foo() }", "const foo = () => { function bar() { } }\nfoo()", + " + export function log(message: string, ...interpolations: unknown[]): void; + export function log(message: string, ...interpolations: unknown[]): void { + console.log(message); + } + ", + " + export function log(...messages: unknown[]): void { + return; + } + ", ]; let fix = vec![ @@ -915,7 +934,7 @@ fn test_type_references() { type PermissionValues = { [K in keyof T]: T[K] extends object ? PermissionValues : T[K]; }[keyof T]; - + export type ApiPermission = PermissionValues; export const API_PERMISSIONS = {} as const; diff --git a/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-functions.snap b/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-functions.snap index 6903fab79de39..31c927622e235 100644 --- a/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-functions.snap +++ b/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-functions.snap @@ -25,3 +25,23 @@ source: crates/oxc_linter/src/tester.rs 2 │ foo() ╰──── help: Consider removing this declaration. + + ⚠ eslint(no-unused-vars): Variable 'interpolations' is declared but never used. + ╭─[no_unused_vars.tsx:3:46] + 2 │ export function log(message: string, ...interpolations: unknown[]): void; + 3 │ export function log(message: string, ...interpolations: unknown[]): void { + · ──────────────┬───────────── + · ╰── 'interpolations' is declared here + 4 │ console.log(message); + ╰──── + help: Consider removing this declaration. + + ⚠ eslint(no-unused-vars): Variable 'messages' is declared but never used. + ╭─[no_unused_vars.tsx:2:29] + 1 │ + 2 │ export function log(...messages: unknown[]): void { + · ───────────┬────────── + · ╰── 'messages' is declared here + 3 │ return; + ╰──── + help: Consider removing this declaration. From 8e7eee93df2e3afab9a2153e24b9de0b75c3322d Mon Sep 17 00:00:00 2001 From: Cameron McHenry Date: Thu, 5 Sep 2024 01:04:26 -0400 Subject: [PATCH 2/7] Allow unused binding rest elements to be ignored when used as part of overload --- .../rules/eslint/no_unused_vars/allowed.rs | 36 +++++++++++++++++++ .../src/rules/eslint/no_unused_vars/mod.rs | 6 ++++ 2 files changed, 42 insertions(+) diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs index 6330ac5bbfaa6..8c7316b03a304 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs @@ -298,4 +298,40 @@ impl NoUnusedVars { _ => false, } } + + /// Returns `true` if this binding rest element should be allowed (i.e. not + /// reported). Currently, this handles the case where a rest element is part + /// of a TS function declaration. + pub(super) fn is_allowed_binding_rest_element<'a>( + &self, + semantic: &Semantic<'a>, + symbol: &Symbol<'_, 'a>, + ) -> bool { + // Handle binding rest element as part of a function parameter, for example: + // `function foo(...messages: string[]) {}` + let parent = symbol.iter_parents().find_map(|p| match p.kind() { + AstKind::FormalParameters(_) => Some(p.id()), + _ => None, + }); + match parent { + Some(params_id) => { + let mut parents_iter = + semantic.nodes().iter_parents(params_id).skip(1).map(AstNode::kind); + + // in function declarations, the parent immediately before the + // FormalParameters is a TSDeclareBlock + let Some(parent) = parents_iter.next() else { + return false; + }; + // allow unused rest parameters in function overloads + if matches!(parent, AstKind::Function(f) if f.r#type == FunctionType::TSDeclareFunction) + { + return true; + } + + false + } + None => false, + } + } } diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs index 13cc2f12525c2..4b4c679142c2d 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs @@ -298,6 +298,12 @@ impl NoUnusedVars { } ctx.diagnostic(diagnostic::param(symbol)); } + AstKind::BindingRestElement(_) => { + if self.is_allowed_binding_rest_element(ctx.semantic().as_ref(), symbol) { + return; + } + ctx.diagnostic(diagnostic::declared(symbol)); + } AstKind::Class(_) | AstKind::Function(_) => { if self.is_allowed_class_or_function(symbol) { return; From 8a514315e1966b283506294a08876a479245bc40 Mon Sep 17 00:00:00 2001 From: Cameron McHenry Date: Thu, 5 Sep 2024 01:16:19 -0400 Subject: [PATCH 3/7] Minimize diff --- .../oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs index e64a69595f40a..25cde5787e8d2 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs @@ -451,7 +451,7 @@ fn test_functions() { " function foo(a: number): number; function foo(a: number | string): number { - return Number(a) + return Number(a) } foo(); ", @@ -925,7 +925,7 @@ fn test_type_references() { " import type { mySchema } from './my-schema'; function test(arg: ReturnType) { - arg; + arg;g } test(''); ", From 63929b21bbe5cc5659cf13cbaf7cb76450f9df2a Mon Sep 17 00:00:00 2001 From: Cameron McHenry Date: Thu, 5 Sep 2024 01:17:19 -0400 Subject: [PATCH 4/7] remove letter --- .../oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs index 25cde5787e8d2..0ce4ecd56c7ef 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs @@ -925,7 +925,7 @@ fn test_type_references() { " import type { mySchema } from './my-schema'; function test(arg: ReturnType) { - arg;g + arg; } test(''); ", @@ -934,7 +934,7 @@ fn test_type_references() { type PermissionValues = { [K in keyof T]: T[K] extends object ? PermissionValues : T[K]; }[keyof T]; - + export type ApiPermission = PermissionValues; export const API_PERMISSIONS = {} as const; From dc4e0648d2df95069a4034016c6e3c2ded10fa19 Mon Sep 17 00:00:00 2001 From: Cameron McHenry Date: Thu, 5 Sep 2024 13:32:44 -0400 Subject: [PATCH 5/7] Check if a parent is a TS function declaration --- .../rules/eslint/no_unused_vars/allowed.rs | 35 +++++-------------- .../src/rules/eslint/no_unused_vars/mod.rs | 2 +- 2 files changed, 9 insertions(+), 28 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs index 8c7316b03a304..55fcdf28673ce 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs @@ -302,36 +302,17 @@ impl NoUnusedVars { /// Returns `true` if this binding rest element should be allowed (i.e. not /// reported). Currently, this handles the case where a rest element is part /// of a TS function declaration. - pub(super) fn is_allowed_binding_rest_element<'a>( - &self, - semantic: &Semantic<'a>, - symbol: &Symbol<'_, 'a>, - ) -> bool { - // Handle binding rest element as part of a function parameter, for example: - // `function foo(...messages: string[]) {}` - let parent = symbol.iter_parents().find_map(|p| match p.kind() { - AstKind::FormalParameters(_) => Some(p.id()), - _ => None, - }); - match parent { - Some(params_id) => { - let mut parents_iter = - semantic.nodes().iter_parents(params_id).skip(1).map(AstNode::kind); - - // in function declarations, the parent immediately before the - // FormalParameters is a TSDeclareBlock - let Some(parent) = parents_iter.next() else { - return false; - }; - // allow unused rest parameters in function overloads - if matches!(parent, AstKind::Function(f) if f.r#type == FunctionType::TSDeclareFunction) - { + pub(super) fn is_allowed_binding_rest_element<'a>(&self, symbol: &Symbol<'_, 'a>) -> bool { + for parent in symbol.iter_parents() { + // If this is a binding rest element that is part of a TS function parameter, + // for example: `function foo(...messages: string[]) {}`, then we will allow it. + if let AstKind::Function(f) = parent.kind() { + if f.is_typescript_syntax() { return true; } - - false } - None => false, } + + false } } diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs index 4b4c679142c2d..3ac48a43e8cde 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs @@ -299,7 +299,7 @@ impl NoUnusedVars { ctx.diagnostic(diagnostic::param(symbol)); } AstKind::BindingRestElement(_) => { - if self.is_allowed_binding_rest_element(ctx.semantic().as_ref(), symbol) { + if self.is_allowed_binding_rest_element(symbol) { return; } ctx.diagnostic(diagnostic::declared(symbol)); From 351d8348ce08863dd80e426a2e2f4ac7b721a1f4 Mon Sep 17 00:00:00 2001 From: Cameron McHenry Date: Thu, 5 Sep 2024 16:32:42 -0400 Subject: [PATCH 6/7] Return immediately when function --- crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs index 55fcdf28673ce..de92f0468cdd3 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs @@ -307,9 +307,7 @@ impl NoUnusedVars { // If this is a binding rest element that is part of a TS function parameter, // for example: `function foo(...messages: string[]) {}`, then we will allow it. if let AstKind::Function(f) = parent.kind() { - if f.is_typescript_syntax() { - return true; - } + return f.is_typescript_syntax(); } } From 0c9bfe5905c4bf2eaa4f31f71d1bac11c4bcc4a2 Mon Sep 17 00:00:00 2001 From: Cameron McHenry Date: Thu, 5 Sep 2024 16:41:23 -0400 Subject: [PATCH 7/7] Apply lint fixes --- crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs | 2 +- crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs index de92f0468cdd3..ef5c7e524fc15 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs @@ -302,7 +302,7 @@ impl NoUnusedVars { /// Returns `true` if this binding rest element should be allowed (i.e. not /// reported). Currently, this handles the case where a rest element is part /// of a TS function declaration. - pub(super) fn is_allowed_binding_rest_element<'a>(&self, symbol: &Symbol<'_, 'a>) -> bool { + pub(super) fn is_allowed_binding_rest_element(symbol: &Symbol) -> bool { for parent in symbol.iter_parents() { // If this is a binding rest element that is part of a TS function parameter, // for example: `function foo(...messages: string[]) {}`, then we will allow it. diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs index 3ac48a43e8cde..f9d31ae4b234c 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs @@ -299,7 +299,7 @@ impl NoUnusedVars { ctx.diagnostic(diagnostic::param(symbol)); } AstKind::BindingRestElement(_) => { - if self.is_allowed_binding_rest_element(symbol) { + if NoUnusedVars::is_allowed_binding_rest_element(symbol) { return; } ctx.diagnostic(diagnostic::declared(symbol));