New rule: Disallow callbacks with errors and object/array deconstructors at the same time

[msiebuhr]

Purpose

Prevent invalid object/error de-constructing when used with error arguments.

function some_callback(err, {foo, bar}) {
    if (err) { console.error(err); }
    console.log(foo, bar);
 }

// Normal use
some_callback(undefined, {foo: 'some', bar: 'arg'});

// When failures occur, callbacks are usually called with just the error and no further arguments:
some_callback(new Error('Could not foo or bar'))
// TypeError: Cannot destructure property `foo` of 'undefined' or 'null'.

What category of rule is this? (place an "X" next to just one item)

[x] Warns about a potential error (problem)
[ ] Suggests an alternate way of doing something (suggestion)
[ ] Enforces code style (layout)
[ ] Other (please specify:)

Why should this rule be included in ESLint (instead of a plugin)?

I've seen quite a few people make this mistake and not realize that their error-handling flow is itself broken. We found at least two "vanishing errors" in our code-bases and quite a few potential ones by applying this rule.

Rough implementation

module.exports = {
    meta: {
        type: "problem",
        docs: {
            description: "Disabllow function argument deconstructuring with error arguments",
            category: "",
            recommended: true,
            url: ""
        }
    },
    create: function (context) {
        function TestFunctionArguments(node) {
            // Not relevant for functions with less than two arguments
            if (!node.params || node.params.length < 2) { return; }

            // Is the first argument named `err`
            const [firstParam, ...restParams] = node.params;
            if (firstParam.type !== 'Identifier' || firstParam.name !== 'err') { return; }

            for (const r of restParams) {
                if (r.type !== 'ArrayPattern' && r.type !== 'ObjectPattern') { continue; }

                context.report({
                    node: r,
                    message: 'Cannot use `err` argument with an {{ type }}',
                    data: {
                        type: r.type
                    }
                });
            }
        }

        return {
            FunctionDeclaration: TestFunctionArguments,
            FunctionExpression: TestFunctionArguments,
            ArrowFunctionExpression: TestFunctionArguments
        };
    }
};

(Moved from eslint/eslint#11539)

[mysticatea]

Hi. Thank you for the proposal!

Sounds good to me, but I have a question. If the destructuring binding has that default value, should the rule allow it? For example: function some_callback(err, {foo, bar} = {}) {...

[msiebuhr]

From some quick testing, it looks like it shouldn't be allowed:

> function some_callback(err, {foo=1, bar=2}) { console.log(err, foo, bar); }
> some_callback('123', {})
123 1 2
> some_callback('123')
TypeError: Cannot destructure property `foo` of 'undefined' or 'null'.
    at some_callback (repl:1:23)
>

[mysticatea]

My question is about the default value of whole the destructuring :)

function some_callback(err, {foo, bar} = {}) { console.log(err, foo, bar); }

[msiebuhr]

Hadn't thought of that - looks like a fix:

> function some_callback(err, {foo, bar} = {}) { console.log(err, foo, bar); }
> some_callback('123')
123 undefined undefined
>