fix(linter): Don't mark binding rest elements as unused in TS function overloads #5470

camchenry · 2024-09-05T05:08:53Z

Fixes linter: no-unused-vars false positive w/TS overload #5406

This implements a fix for the BindingRestElement symbol, which is currently unhandled and gets automatically marked as unused. If we happen to find that it is a child of declaration, then we will automatically allow the binding rest element.

The code for this was based on what we currently do in is_allowed_param_because_of_method:

oxc/crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs

Line 258 in 5187f38

    
           if matches!(parent, AstKind::Function(f) if f.r#type == FunctionType::TSDeclareFunction) {

I opted not to refactor this to re-use the same code though, as I think the duplication is still incidental and the implementations could diverge in the future.

… overload

graphite-app · 2024-09-05T05:08:58Z

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

codspeed-hq · 2024-09-05T05:25:26Z

CodSpeed Performance Report

Merging #5470 will not alter performance

_{Comparing camchenry:no-unused-vars-rest-elements-fix (621c4d4) with main (fce549e)}

Summary

✅ 29 untouched benchmarks

camc314

can't we just check if the funciton is TS syntax

oxc/crates/oxc_ast/src/ast_impl/js.rs

Lines 1069 to 1075 in f81e8a1

    
           pub fn is_typescript_syntax(&self) -> bool { 
        
               matches!( 
        
                   self.r#type, 
        
                   FunctionType::TSDeclareFunction | FunctionType::TSEmptyBodyFunctionExpression 
        
               ) || self.body.is_none() 
        
                   || self.declare 
        
           }

and skip checking it?

camchenry · 2024-09-05T17:34:25Z

@camc314 Thanks for the tip. Since we are looking at whether a given symbol is allowed, I changed it to check if the binding rest element is a child of a Function. If so, if is_typescript_syntax() is true, then we will allow it, otherwise it fails. I think this makes it a lot simpler, let me know if this makes sense.

crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs

camc314 · 2024-09-05T18:13:12Z

one last comment then this looks good to me.

thanks for contributing!

DonIsaac

See: Cam's comment thread

DonIsaac · 2024-09-05T19:22:49Z

@camchenry run just fix to auto-fix clippy/rustfmt/etc issues.

camc314 · 2024-09-05T19:41:17Z

interestingly enough @DonIsaac this isn't an error 🤔 (the arg argument)

fyi camchenry, lets not try to fix this (if we should) in this PR

changes resolved

camc314

Thank you!

## [0.9.3] - 2024-09-07 ### Features - be3a432 linter: Implement typescript/no-magic-numbers (#4745) (Alexander S.) - 09aa86d linter/eslint: Implement `sort-vars` rule (#5430) (Jelle van der Waa) - 2ec2f7d linter/eslint: Implement no-alert (#5535) (Edwin Lim) - a786acf linter/import: Add no-dynamic-require rule (#5389) (Jelle van der Waa) - 4473779 linter/node: Implement no-exports-assign (#5370) (dalaoshu) - b846432 linter/oxc: Add fixer for `erasing-op` (#5377) (camc314) - aff2c71 linter/react: Implement `self-closing-comp` (#5415) (Jelle van der Waa) ### Bug Fixes - 0df1d9d ast, codegen, linter: Panics in fixers. (#5431) (rzvxa) - cdd1a91 linter: Typescript/no-magic-numbers: remove double minus for reporting negative bigint numbers (#5565) (Alexander S.) - ff88c1f linter: Don't mark binding rest elements as unused in TS function overloads (#5470) (Cam McHenry) - 088733b linter: Handle loops in `getter-return` rule (#5517) (Cam McHenry) - 82c0a16 linter: `tree_shaking/no_side_effects_in_initialization` handle JSX correctly (#5450) (overlookmotel) - 6285a02 linter: `eslint/radix` rule correctly check for unbound symbols (#5446) (overlookmotel) - c8ab353 linter/tree-shaking: Align JSXMemberExpression's report (#5548) (mysteryven) - 5187f38 linter/tree-shaking: Detect the correct export symbol resolution (#5467) (mysteryven) ### Performance - 8170954 linter/react: Add should_run conditions for react rules (#5402) (Jelle van der Waa) ### Documentation - a540215 linter: Update docs `Examples` for linter rules (#5513) (dalaoshu) - 7414190 linter: Update docs `Example` for linter rules (#5479) (heygsc) ### Refactor - 0ac420d linter: Use meaningful names for diagnostic parameters (#5564) (Don Isaac) - 81a394d linter: Deduplicate code in `oxc/no-async-await` (#5549) (DonIsaac) - 979c16c linter: Reduce nested if statements in eslint/no_this_before_super (#5485) (IWANABETHATGUY) - 1d3e973 linter: Simplify `eslint/radix` rule (#5445) (overlookmotel) - fdb8857 linter: Use "parsed pattern" in `no_div_regex` rule. (#5417) (rzvxa) - 2ccbd93 linter: `react/jsx_no_undef` rule `get_member_ident` do not return Option (#5411) (overlookmotel) ### Styling - 2a43fa4 linter: Introduce the writing style from PR #5491 and reduce the if nesting (#5512) (dalaoshu)- d8b29e7 Add trailing line breaks to JSON files (#5544) (overlookmotel)- 694f032 Add trailing line breaks to `package.json` files (#5542) (overlookmotel) ### Testing - 340b535 linter/no-unused-vars: Arrow functions in tagged templates (#5510) (Don Isaac) - af69393 linter/no-useless-spread: Ensure spreads on identifiers pass (#5561) (DonIsaac)- dc92489 Add trailing line breaks to conformance fixtures (#5541) (overlookmotel) Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>

camchenry added 2 commits September 5, 2024 01:01

Add tests

47e18eb

Allow unused binding rest elements to be ignored when used as part of…

8e7eee9

… overload

github-actions bot added the A-linter Area - Linter label Sep 5, 2024

Minimize diff

8a51431

Boshen requested review from DonIsaac and camc314 September 5, 2024 05:16

remove letter

63929b2

camc314 reviewed Sep 5, 2024

View reviewed changes

DonIsaac self-assigned this Sep 5, 2024

DonIsaac added the C-bug Category - Bug label Sep 5, 2024

Check if a parent is a TS function declaration

dc4e064

camchenry requested a review from camc314 September 5, 2024 17:34

camc314 reviewed Sep 5, 2024

View reviewed changes

crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs Outdated Show resolved Hide resolved

DonIsaac previously requested changes Sep 5, 2024

View reviewed changes

camchenry added 2 commits September 5, 2024 16:32

Return immediately when function

351d834

Apply lint fixes

0c9bfe5

camchenry requested review from camc314 and DonIsaac September 5, 2024 20:47

Merge branch 'main' into no-unused-vars-rest-elements-fix

621c4d4

camc314 approved these changes Sep 6, 2024

View reviewed changes

camc314 merged commit ff88c1f into oxc-project:main Sep 6, 2024
23 checks passed

oxc-bot mentioned this pull request Sep 7, 2024

release: oxlint v0.9.3 #5569

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(linter): Don't mark binding rest elements as unused in TS function overloads #5470

fix(linter): Don't mark binding rest elements as unused in TS function overloads #5470

camchenry commented Sep 5, 2024

graphite-app bot commented Sep 5, 2024

codspeed-hq bot commented Sep 5, 2024 •

edited

Loading

camc314 left a comment •

edited

Loading

camchenry commented Sep 5, 2024

camc314 commented Sep 5, 2024

DonIsaac left a comment

DonIsaac commented Sep 5, 2024

camc314 commented Sep 5, 2024 •

edited

Loading

camc314 left a comment

	pub fn is_typescript_syntax(&self) -> bool {
	matches!(
	self.r#type,
	FunctionType::TSDeclareFunction \| FunctionType::TSEmptyBodyFunctionExpression
	) \|\| self.body.is_none()
	\|\| self.declare
	}

fix(linter): Don't mark binding rest elements as unused in TS function overloads #5470

fix(linter): Don't mark binding rest elements as unused in TS function overloads #5470

Conversation

camchenry commented Sep 5, 2024

graphite-app bot commented Sep 5, 2024

Your org has enabled the Graphite merge queue for merging into main

codspeed-hq bot commented Sep 5, 2024 • edited Loading

CodSpeed Performance Report

Merging #5470 will not alter performance

Summary

camc314 left a comment • edited Loading

Choose a reason for hiding this comment

camchenry commented Sep 5, 2024

camc314 commented Sep 5, 2024

DonIsaac left a comment

Choose a reason for hiding this comment

DonIsaac commented Sep 5, 2024

camc314 commented Sep 5, 2024 • edited Loading

camc314 left a comment

Choose a reason for hiding this comment

codspeed-hq bot commented Sep 5, 2024 •

edited

Loading

camc314 left a comment •

edited

Loading

camc314 commented Sep 5, 2024 •

edited

Loading