fix(eslint-plugin): [unbound-method] report on destructuring in function parameters #8952

auvred · 2024-04-19T07:01:46Z

PR Checklist

Addresses an existing open issue: fixes Bug: [unbound-method] Does not fail for destructured parameters #8947
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

Old approach: query for VariableDeclarator, AssignmentExpression and check if they have ObjectPattern

New approach: query for ObjectPattern, check if VariableDeclarator, AssignmentExpression, AssignmentPattern its parent

We're iterating over ObjectPattern's properties:

If ObjectPatterns's parent has init node, check if it's safe to destructure this init node:
- if it's unsafe, then report on this property and go to the next property
- if it's safe, then check if ObjectPatterns's parent is AssignmentPattern:
  - if false, go to the next property
  - if true, continue working on this property
If ObjectPattern has a type annotation, then check if it's safe to destructure this type
If there is no type annotation, check the inferred property type

…ion parameters

typescript-eslint · 2024-04-19T07:01:49Z

Thanks for the PR, @auvred!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

netlify · 2024-04-19T07:02:04Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`7dafb3a`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/66352c91184fe100088c2860
😎 Deploy Preview	https://deploy-preview-8952--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 92 (🔴 down 5 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

nx-cloud · 2024-04-19T07:03:37Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 7dafb3a. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 31 targets

Sent with 💌 from NxCloud.

kirkwaiblinger

I really like these changes! The algorithm change makes sense to me, the code is easy to follow, and the new code for type inspection is very thorough.

Added a few questions and some test cases to flesh out edge cases.

kirkwaiblinger · 2024-04-20T04:28:38Z

packages/eslint-plugin/src/rules/unbound-method.ts

+          node.parent.type === AST_NODE_TYPES.AssignmentExpression
+        ) {
+          initNode = node.parent.right;
+        }


should there be some kind of assertion here to ensure that every way that ObjectPattern could occur is handled (especially in light of the bug that this PR fixes)? Relates to #6225

Interestingly, the introduction of the type checking actually supports unaccounted-for parents pretty well.

Check this out 😆

// should be invalid, passes (didn't in previous implementation) function f({ evil: { nesting } } = { evil: { nesting: function () { } } }) { } // should also be invalid, fails (since it's _only_ looking at the type) function g({ evil: { nesting } }: any = { evil: { nesting: function () { } } }) { }

To be clear, I don't think you need to account for these crazy scenarios (or, even more heinous things like const [{ a }, { b }] = [{ a() {} }, { b: 'b' }]; in this PR. I am just pointing out that it's cool that you've incidentally added partial support for them!

There is one false positive regression introduced that I can think of, though....

// the possible assignments _are_ provably bound. // but the presence of the annotation causes `b` to flag as possibly unbound. const { a: { b } = { b: () => { } } }: { a: { b(): void } } = { a: undefined }

but I really don't think it's a blocker granted how absurd it is.

packages/eslint-plugin/src/rules/unbound-method.ts

auvred · 2024-04-23T16:23:16Z

packages/eslint-plugin/src/rules/unbound-method.ts

-          nativelyBoundMembers.has(getMemberFullName(node)) &&
-          isNotImported(objectSymbol, currentSourceFile)
+          notImported &&
+          nativelyBoundMembers.has(`${object.name}.${property.name}`)


const { log } = console;

In our current configuration, the console.log declarations are under @types/node. So if we remove this check, the rule will report on this case. Not sure what's the best solution there..

Are you saying, you'd like to get rid of the first half of this function and only have the part after the return statement, but that that would cause bugs to do so? I'm not 100% following.

kirkwaiblinger

A couple questions/commenting requests, but no significant concrete changes being requested at this time.

kirkwaiblinger · 2024-05-09T05:09:54Z

packages/eslint-plugin/src/rules/unbound-method.ts

+        isBuiltinSymbolLike(
+          services.program,
+          services.getTypeAtLocation(object),
+          [


What's the difference between this array literal and the SUPPORTED_GLOBALS array? Do we need both? Perhaps giving it a good name will make it clear?

Thoughts?

kirkwaiblinger · 2024-05-09T05:28:50Z

packages/eslint-plugin/src/rules/unbound-method.ts

-        }
-
-        const objectSymbol = services.getSymbolAtLocation(node.object);
+    function isNativelyBound(


It's not super obvious how this function works. I cloned it down to play around with the tests, and I'm still not 100% sure, but my rough understanding is that the first part checks by identity whether something is a natively bound member, and the second part checks via the type system, in order to catch a more general set of cases where the builtin namespace objects are potentially renamed.

Could you add some comments to help the reader 🙏 ?

kirkwaiblinger · 2024-05-09T06:33:15Z

packages/eslint-plugin/src/rules/unbound-method.ts

+          node.parent.type === AST_NODE_TYPES.AssignmentExpression
+        ) {
+          initNode = node.parent.right;
+        }


Interestingly, the introduction of the type checking actually supports unaccounted-for parents pretty well.

Check this out 😆

// should be invalid, passes (didn't in previous implementation) function f({ evil: { nesting } } = { evil: { nesting: function () { } } }) { } // should also be invalid, fails (since it's _only_ looking at the type) function g({ evil: { nesting } }: any = { evil: { nesting: function () { } } }) { }

To be clear, I don't think you need to account for these crazy scenarios (or, even more heinous things like const [{ a }, { b }] = [{ a() {} }, { b: 'b' }]; in this PR. I am just pointing out that it's cool that you've incidentally added partial support for them!

There is one false positive regression introduced that I can think of, though....

// the possible assignments _are_ provably bound. // but the presence of the annotation causes `b` to flag as possibly unbound. const { a: { b } = { b: () => { } } }: { a: { b(): void } } = { a: undefined }

but I really don't think it's a blocker granted how absurd it is.

kirkwaiblinger · 2024-05-09T06:49:41Z

packages/eslint-plugin/src/rules/unbound-method.ts

-          nativelyBoundMembers.has(getMemberFullName(node)) &&
-          isNotImported(objectSymbol, currentSourceFile)
+          notImported &&
+          nativelyBoundMembers.has(`${object.name}.${property.name}`)


Are you saying, you'd like to get rid of the first half of this function and only have the part after the return statement, but that that would cause bugs to do so? I'm not 100% following.

kirkwaiblinger · 2024-05-09T06:50:48Z

packages/eslint-plugin/src/rules/unbound-method.ts

+      ) {
+        const objectSymbol = services.getSymbolAtLocation(object);
+        const notImported =
+          objectSymbol && isNotImported(objectSymbol, currentSourceFile);


(nit, optional) feels like this should be an actual boolean

Suggested change

objectSymbol && isNotImported(objectSymbol, currentSourceFile);

objectSymbol != null && isNotImported(objectSymbol, currentSourceFile);

fix(eslint-plugin): [unbound-method] report on destructuring in funct…

0dcb9d7

…ion parameters

chore: lint --fix

0031ebf

kirkwaiblinger requested changes Apr 20, 2024

View reviewed changes

kirkwaiblinger added the awaiting response Issues waiting for a reply from the OP or another party label Apr 20, 2024

9f57b64

auvred commented Apr 23, 2024

View reviewed changes

auvred mentioned this pull request Apr 23, 2024

Repo: Make utility for static member access #8323

Open

auvred added 2 commits May 3, 2024 21:16

Merge branch 'main' into fix/8947

fe11a7b

refactor: improve iteration over union/intersection parts

7dafb3a

auvred requested a review from kirkwaiblinger May 3, 2024 18:43

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label May 3, 2024

kirkwaiblinger reviewed May 9, 2024

View reviewed changes

kirkwaiblinger added the awaiting response Issues waiting for a reply from the OP or another party label May 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [unbound-method] report on destructuring in function parameters #8952

fix(eslint-plugin): [unbound-method] report on destructuring in function parameters #8952

auvred commented Apr 19, 2024 •

edited

typescript-eslint bot commented Apr 19, 2024

netlify bot commented Apr 19, 2024 •

edited

nx-cloud bot commented Apr 19, 2024 •

edited

kirkwaiblinger left a comment

kirkwaiblinger Apr 20, 2024

kirkwaiblinger May 9, 2024

auvred Apr 23, 2024

kirkwaiblinger May 9, 2024

kirkwaiblinger left a comment

kirkwaiblinger May 9, 2024

kirkwaiblinger May 9, 2024

kirkwaiblinger May 9, 2024

kirkwaiblinger May 9, 2024

kirkwaiblinger May 9, 2024

	objectSymbol && isNotImported(objectSymbol, currentSourceFile);
	objectSymbol != null && isNotImported(objectSymbol, currentSourceFile);

fix(eslint-plugin): [unbound-method] report on destructuring in function parameters #8952

Are you sure you want to change the base?

fix(eslint-plugin): [unbound-method] report on destructuring in function parameters #8952

Conversation

auvred commented Apr 19, 2024 • edited

PR Checklist

Overview

typescript-eslint bot commented Apr 19, 2024

netlify bot commented Apr 19, 2024 • edited

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Apr 19, 2024 • edited

☁️ Nx Cloud Report

kirkwaiblinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirkwaiblinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auvred commented Apr 19, 2024 •

edited

netlify bot commented Apr 19, 2024 •

edited

nx-cloud bot commented Apr 19, 2024 •

edited