From c6db7c4bfcdc6e41e57cf1cce0020137eb4c2646 Mon Sep 17 00:00:00 2001 From: Ulrich Stark Date: Sat, 22 Mar 2025 17:39:32 +0100 Subject: [PATCH 1/2] fix false positive --- ...necessary_parameter_property_assignment.rs | 27 ++++++++++++++----- ...cessary_parameter_property_assignment.snap | 9 ------- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/crates/oxc_linter/src/rules/typescript/no_unnecessary_parameter_property_assignment.rs b/crates/oxc_linter/src/rules/typescript/no_unnecessary_parameter_property_assignment.rs index 97ad70bb06995..1d4b34b9ffa94 100644 --- a/crates/oxc_linter/src/rules/typescript/no_unnecessary_parameter_property_assignment.rs +++ b/crates/oxc_linter/src/rules/typescript/no_unnecessary_parameter_property_assignment.rs @@ -2,12 +2,13 @@ use oxc_ast::{ AstKind, ast::{ AssignmentExpression, AssignmentOperator, AssignmentTarget, ClassElement, Expression, - FormalParameter, MethodDefinitionKind, Statement, + FormalParameter, Function, MethodDefinitionKind, Statement, }, }; use oxc_ast_visit::Visit; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; +use oxc_semantic::ScopeFlags; use oxc_span::{Atom, Span}; use rustc_hash::FxHashSet; @@ -105,13 +106,17 @@ impl Rule for NoUnnecessaryParameterPropertyAssignment { } } + let Some(function_body) = &method.value.body else { + return; + }; + let mut visitor = AssignmentVisitor { ctx, parameter_properties, assigned_before_unnecessary: FxHashSet::default(), assigned_before_constructor, }; - visitor.visit_method_definition(method); + visitor.visit_function_body(function_body); } } @@ -123,10 +128,11 @@ struct AssignmentVisitor<'a, 'b> { } impl<'a> Visit<'a> for AssignmentVisitor<'a, '_> { - fn visit_assignment_expression( - &mut self, - assignment_expr: &oxc_ast::ast::AssignmentExpression<'a>, - ) { + fn visit_function(&mut self, _it: &Function<'a>, _flags: ScopeFlags) { + // don't continue walking into functions as they have a different scoped "this" + } + + fn visit_assignment_expression(&mut self, assignment_expr: &AssignmentExpression<'a>) { let Some(this_property_name) = get_property_name(&assignment_expr.left) else { return; }; @@ -474,6 +480,15 @@ fn test() { })(); } ", + " + class Foo { + constructor(private foo: string) { + function bar() { + this.foo = foo; + } + } + } + ", ]; let fail = vec![ diff --git a/crates/oxc_linter/src/snapshots/typescript_no_unnecessary_parameter_property_assignment.snap b/crates/oxc_linter/src/snapshots/typescript_no_unnecessary_parameter_property_assignment.snap index 79343d258487c..e1df2415dd13c 100644 --- a/crates/oxc_linter/src/snapshots/typescript_no_unnecessary_parameter_property_assignment.snap +++ b/crates/oxc_linter/src/snapshots/typescript_no_unnecessary_parameter_property_assignment.snap @@ -136,15 +136,6 @@ source: crates/oxc_linter/src/tester.rs ╰──── help: Remove the unnecessary assignment - ⚠ typescript-eslint(no-unnecessary-parameter-property-assignment): Assignment of parameter property is unnecessary - ╭─[no_unnecessary_parameter_property_assignment.tsx:5:15] - 4 │ function bar() { - 5 │ this.foo = foo; - · ────────────── - 6 │ } - ╰──── - help: Remove the unnecessary assignment - ⚠ typescript-eslint(no-unnecessary-parameter-property-assignment): Assignment of parameter property is unnecessary ╭─[no_unnecessary_parameter_property_assignment.tsx:7:13] 6 │ } From 6cc85e3cf92e151a94435071ba6f24ece0132000 Mon Sep 17 00:00:00 2001 From: Ulrich Stark Date: Sat, 22 Mar 2025 17:41:12 +0100 Subject: [PATCH 2/2] suggest fix and add tests --- ...necessary_parameter_property_assignment.rs | 444 +++++++++++++++++- 1 file changed, 429 insertions(+), 15 deletions(-) diff --git a/crates/oxc_linter/src/rules/typescript/no_unnecessary_parameter_property_assignment.rs b/crates/oxc_linter/src/rules/typescript/no_unnecessary_parameter_property_assignment.rs index 1d4b34b9ffa94..84565818a963b 100644 --- a/crates/oxc_linter/src/rules/typescript/no_unnecessary_parameter_property_assignment.rs +++ b/crates/oxc_linter/src/rules/typescript/no_unnecessary_parameter_property_assignment.rs @@ -54,7 +54,7 @@ declare_oxc_lint!( NoUnnecessaryParameterPropertyAssignment, typescript, correctness, - pending, + suggestion, ); impl Rule for NoUnnecessaryParameterPropertyAssignment { @@ -182,9 +182,15 @@ impl<'a> Visit<'a> for AssignmentVisitor<'a, '_> { continue; // there already was an assignment outside the constructor } - self.ctx.diagnostic(no_unnecessary_parameter_property_assignment_diagnostic( - assignment_expr.span, - )); + self.ctx.diagnostic_with_suggestion( + no_unnecessary_parameter_property_assignment_diagnostic(assignment_expr.span), + |fixer| { + fixer.delete_range(Span::new( + assignment_expr.span.start, + assignment_expr.span.end + 1, + )) + }, + ); } } } @@ -260,47 +266,47 @@ fn test() { use crate::tester::Tester; let pass = vec![ - r" + " class Foo { constructor(public name: unknown) {} } ", - r" + " class Foo { constructor(public name: unknown, other: unknown) { this.other = other; } } ", - r" + " class Foo { constructor(public name: unknown) { this.other = name; } } ", - r" + " class Foo { constructor(name: unknown) { this.name = name; } } ", - r" + " class Foo { constructor(public name: unknown) { this.name = 'other'; } } ", - r" + " class Foo { constructor(public name: unknown) { this.name = name + 'edited'; } } ", - r" + " class Foo { constructor(public name: unknown) { if (maybeTrue) { @@ -492,14 +498,14 @@ fn test() { ]; let fail = vec![ - r" + " class Foo { constructor(public name: unknown) { this.name = name; } } ", - r" + " class Foo { constructor(other: unknown, public name: unknown) { this.other = other; @@ -507,7 +513,7 @@ fn test() { } } ", - r" + " class Foo { constructor(public name: unknown) { this.name = name; @@ -516,7 +522,7 @@ fn test() { } } ", - r" + " class Foo { constructor(public name: unknown) { if (maybeTrue) { @@ -675,11 +681,419 @@ fn test() { ", ]; + let fix = vec![ + ( + " + class Foo { + constructor(public name: unknown) { + this.name = name; + } + } + ", + " + class Foo { + constructor(public name: unknown) { + + } + } + ", + ), + ( + " + class Foo { + constructor(other: unknown, public name: unknown) { + this.other = other; + this.name = name; + } + } + ", + " + class Foo { + constructor(other: unknown, public name: unknown) { + this.other = other; + + } + } + ", + ), + ( + " + class Foo { + constructor(public name: unknown) { + this.name = name; + this.name = name; + this.name = name; + } + } + ", + " + class Foo { + constructor(public name: unknown) { + + + + } + } + ", + ), + ( + " + class Foo { + constructor(public name: unknown) { + if (maybeTrue) { + this.name = name; + } else { + this.name = name + 'edited'; + } + } + } + ", + " + class Foo { + constructor(public name: unknown) { + if (maybeTrue) { + + } else { + this.name = name + 'edited'; + } + } + } + ", + ), + ( + " + class Foo { + constructor(public foo: string) { + this.foo = foo; + } + } + ", + " + class Foo { + constructor(public foo: string) { + + } + } + ", + ), + ( + " + class Foo { + constructor(public foo?: string) { + this.foo = foo!; + } + } + ", + " + class Foo { + constructor(public foo?: string) { + + } + } + ", + ), + ( + " + class Foo { + constructor(public foo?: string) { + this.foo = foo as any; + } + } + ", + " + class Foo { + constructor(public foo?: string) { + + } + } + ", + ), + ( + " + class Foo { + constructor(public foo = '') { + this.foo = foo; + } + } + ", + " + class Foo { + constructor(public foo = '') { + + } + } + ", + ), + ( + " + class Foo { + constructor(public foo = '') { + this.foo = foo; + this.foo += 'foo'; + } + } + ", + " + class Foo { + constructor(public foo = '') { + + this.foo += 'foo'; + } + } + ", + ), + ( + " + class Foo { + constructor(public foo: string) { + this.foo ||= foo; + } + } + ", + " + class Foo { + constructor(public foo: string) { + + } + } + ", + ), + ( + " + class Foo { + constructor(public foo: string) { + this.foo ??= foo; + } + } + ", + " + class Foo { + constructor(public foo: string) { + + } + } + ", + ), + ( + " + class Foo { + constructor(public foo: string) { + this.foo &&= foo; + } + } + ", + " + class Foo { + constructor(public foo: string) { + + } + } + ", + ), + ( + " + class Foo { + constructor(private foo: string) { + this['foo'] = foo; + } + } + ", + " + class Foo { + constructor(private foo: string) { + + } + } + ", + ), + ( + " + class Foo { + constructor(private foo: string) { + function bar() { + this.foo = foo; + } + this.foo = foo; + } + } + ", + " + class Foo { + constructor(private foo: string) { + function bar() { + this.foo = foo; + } + + } + } + ", + ), + ( + " + class Foo { + constructor(private foo: string) { + this.bar = () => { + this.foo = foo; + }; + this.foo = foo; + } + } + ", + " + class Foo { + constructor(private foo: string) { + this.bar = () => { + this.foo = foo; + }; + + } + } + ", + ), + ( + " + class Foo { + constructor(private foo: string) { + class Bar { + constructor(private foo: string) { + this.foo = foo; + } + } + this.foo = foo; + } + } + ", + " + class Foo { + constructor(private foo: string) { + class Bar { + constructor(private foo: string) { + + } + } + + } + } + ", + ), + ( + " + class Foo { + constructor(private foo: string) { + this.foo = foo; + } + bar = () => { + this.foo = 'foo'; + }; + } + ", + " + class Foo { + constructor(private foo: string) { + + } + bar = () => { + this.foo = 'foo'; + }; + } + ", + ), + ( + " + class Foo { + constructor(private foo: string) { + this.foo = foo; + } + init = foo => { + this.foo = foo; + }; + } + ", + " + class Foo { + constructor(private foo: string) { + + } + init = foo => { + this.foo = foo; + }; + } + ", + ), + ( + " + class Foo { + constructor(private foo: string) { + this.foo = foo; + } + init = class Bar { + constructor(private foo: string) { + this.foo = foo; + } + }; + } + ", + " + class Foo { + constructor(private foo: string) { + + } + init = class Bar { + constructor(private foo: string) { + + } + }; + } + ", + ), + ( + " + class Foo { + constructor(private foo: string) { + { + this.foo = foo; + } + } + } + ", + " + class Foo { + constructor(private foo: string) { + { + + } + } + } + ", + ), + ( + " + class Foo { + constructor(private foo: string) { + (() => { + this.foo = foo; + })(); + } + } + ", + " + class Foo { + constructor(private foo: string) { + (() => { + + })(); + } + } + ", + ), + ]; + Tester::new( NoUnnecessaryParameterPropertyAssignment::NAME, NoUnnecessaryParameterPropertyAssignment::PLUGIN, pass, fail, ) + .expect_fix(fix) .test_and_snapshot(); }