Bug: no-restricted-properties misses destructuring object returned from function call

[merrywhether]

Environment

Node version: v16.14.2
npm version: v8.5.0
Local ESLint version: v8.24.0 (Currently used)
Global ESLint version: Not found
Operating System: darwin 21.6.0

What parser are you using?

@typescript-eslint/parser

What did you do?

After targeting a property in no-restricted-properties, immediately destructuring an object returned from a function call and accessing the restricted property:

const { bad } = foo();

ESLint demo example

What did you expect to happen?

Accessing restricted property via immediately destructuring object returned from function call is marked as lint error.

What actually happened?

Accessing restricted property via immediately destructuring object returned from function call is not recognized as error.

Participation

• I am willing to submit a pull request for this issue.

Additional comments

AIUI the issue arises from checking the RHS type in an assignment. In the case where all instances of a property are restricted (no paired object argument) the RHS type check is unnecessary. Given the tight coupling with object, a "simple" solution might be treating this object as anonymous/unnamable and defaulting it to undefined (or empty string or some other sentinel that would always fail the ensuing object-name-related Map accesses):

function checkDestructuringAssignment(node) {
    if (node.left.type === "ObjectPattern") {
        const objectName = node.right.type === "Identifier" ? node.right.name : undefined;
            node.left.properties.forEach(property => {
                checkPropertyAccess(node.left, objectName, astUtils.getStaticPropertyName(property));
            });
        }
    } 
}

(I fully admit there might be reasons why this doesn't work)

It would also be possible to extend the API with the ability to constrain the functions whose returned objects' properties can't be accessed (similar to object), but this does increase complexity as you have to handle CallExpression, AsyncExpression, and probably lots more. So maybe that's better left to future need-based implementation?

(useful reference PR for me or whoever winds up tackling this)

[mdjermanovic]

Hi @merrywhether, thanks for the issue!

I think this is a bug. The following property accesses are basically the same, yet only the first is reported:

/* eslint no-restricted-properties: ["error", { property: "bad" }] */

bad = foo().bad; // error

({ bad } = foo()); // no error

I couldn't find any mention of this in #7151, or a test case that would confirm intent to allow access to restricted properties when the destructuring source is not given as an identifier, so this is most likely an oversight in the implementation.

[mdjermanovic]

The rule should probably check all object patterns. In each of the following examples, bad property of an object is accessed:

/* eslint no-restricted-properties: ["error", { property: "bad" }] */

const { bad } = foo(); // should be error?

const { bar: { bad } } = foo; // // should be error?

const { bar: { bad } = {} } = foo; // // should be error?

const [{ bad }] = foo; // // should be error?

({ bad } = foo()); // // should be error?

({ bar: { bad } } = foo); // // should be error?

({ bar: { bad } = {} } = foo); // // should be error?

[{ bad }] = foo; // // should be error?

({ bad }) => {}; // // should be error?

({ bad } = {}) => {}; // // should be error?

({ bad: bar }) => {}; // // should be error?

({ bar: { bad } = {} }) => {}; // // should be error?

Currently, neither of the above examples is reported. Since this would introduce many new cases in which the rule warns, I'd like more opinions from the team.

[snitin315]

@mdjermanovic I feel the same that it should check all object patterns. So all the above examples should be reported 👍🏻

[thewizarodofoz]

Hey good people, wanted to check whether anyone is working on this.
I have no experience in implementing rules, but I could give it a go if no one is available.

[Tanujkanti4441]

The rule should probably check all object patterns. In each of the following examples, bad property of an object is accessed:

/* eslint no-restricted-properties: ["error", { property: "bad" }] */

const { bad } = foo(); // should be error?

const { bar: { bad } } = foo; // // should be error?

const { bar: { bad } = {} } = foo; // // should be error?

const [{ bad }] = foo; // // should be error?

({ bad } = foo()); // // should be error?

({ bar: { bad } } = foo); // // should be error?

({ bar: { bad } = {} } = foo); // // should be error?

[{ bad }] = foo; // // should be error?

({ bad }) => {}; // // should be error?

({ bad } = {}) => {}; // // should be error?

({ bad: bar }) => {}; // // should be error?

({ bar: { bad } = {} }) => {}; // // should be error?

@mdjermanovic, in these cases rule should report just the bad property ? or also the object's name that property is inside , if object is specified in the rules option ?

if yes for the reporting objects as well then the for object pattern in function arguments, will it work like this ?

/* eslint no-restricted-properties: ["error", {object: "restrictedObject", property: "bad" }] */

const foo = ({ bad } = {}, ) => {}; // // should be error

foo(restrictedObject);  

and for the example below what would be the restricted object ?

/* eslint no-restricted-properties: ["error", {object: "restrictedObject", property: "bad" }] */

const { bar: { bad } } = foo; // // should be error?

const [{ bad }] = foo; // // should be error?

[arka1002]

I’m going to do this, give me two weeks.

[Tanujkanti4441]

I’m going to do this, give me two weeks.

Ok! assigning you.