Rule Change: no-unused-vars should consider this is used

[fisker]

What rule do you want to change?

no-unused-vars

What change to do you want to make?

Generate fewer warnings

How do you think the change should be implemented?

Other

Example code

let promise;

function doSomeThingOnce() {
  promise ??= doSomeThing();
}

function doSomeThing() {}
doSomeThingOnce();
doSomeThingOnce();

What does the rule currently do for this code?

Error 1:5 'promise' is assigned a value but never used.  ([no-unused-vars](https://eslint.org/docs/rules/no-unused-vars))

playground

What will the rule do after it's changed?

Should not report this case.

Participation

• I am willing to submit a pull request to implement this change.

Additional comments

No response

[Rec0iL99]

Hi @fisker, can you please provide more context as to why this change is needed?

The variable promise is still unused here in the example and ESLint is right in reporting this right? Am I missing something here?

[aladdin-add]

technically it's a bug to me.

// same as
promise = promise ?? doSomeThing();
promise = promise ?? doSomeThing();

promise was assigned a value and its value was actually read and used.

[fisker]

@Rec0iL99 Let me change the question. How should I fix my case?

[Rec0iL99]

// same as
promise = promise ?? doSomeThing();
promise = promise ?? doSomeThing();

Ah, ok I get your point now. My apologies.

no-unused-vars rule is reported here:

let promise;

function doSomeThingOnce() {
  promise ??= doSomething();
}

function doSomeThing() {}
doSomeThingOnce();
doSomeThingOnce();

But not here where it is doing the same thing:

let promise;

function doSomeThingOnce() {
  promise = promise ?? doSomething();
}

function doSomeThing() {}
doSomeThingOnce();
doSomeThingOnce();

Looks like a bug.

[gweesin]

@Rec0iL99 It looks interesting. The promise variable is not actually used based on the result, but from the perspective of reading and writing the variable, it is indeed being used.

So, could I consider this to be a valid expression?

Now I intend to make these two codeblocks consistent, but I have encountered some difficulties. I have identified the problem in the isUnusedExpression method, but I am currently unsure how to handle this scenario and make it compatible with all shorthand assignment statements.

/**
 * Checks whether a given node is unused expression or not.
 * @param {ASTNode} node The node itself
 * @returns {boolean} The node is an unused expression.
 * @private
 */
function isUnusedExpression(node) {
    const parent = node.parent;

    if (parent.type === "ExpressionStatement") {
        // TODO we need to update this method or take some action here to resolve this question
        return true;
    }

    if (parent.type === "SequenceExpression") {
        const isLastExpression = parent.expressions[parent.expressions.length - 1] === node;

        if (!isLastExpression) {
            return true;
        }
        return isUnusedExpression(parent);
    }

    return false;
}

[gweesin]

😕 I apologize for my impatience. We should first discuss which segment's result should be considered normal before considering how to modify it.

[aladdin-add]

IMHO, it should be seen as used for promise = promise ?? doSomething() and promise ??= doSomething(), so no linting errors should be reported. marking as accepted.

[mdjermanovic]

I think promise should be considered used if there's a promise ??=, promise ||=, or promise &&= reference.

var promise1, promise2;

promise1 ?? x; // this is considered using `promise1`
promise2 ??= x; // so this should be considered using `promise2`

The fix would check the assignment operator here:

eslint/lib/rules/no-unused-vars.js

Lines 466 to 470 in 9718a97

	(
	parent.type === "AssignmentExpression" &&
	parent.left === id &&
	isUnusedExpression(parent)
	) ||

@gweesin would you like to work on this?

[gweesin]

@mdjermanovic Yes, I'm glad to have the opportunity to try and resolve this issue.

While attempting to fix this issue, I noticed a conflict with existing test cases. I am a bit confused and would appreciate some clarification on what behavior would be considered reasonable in this case.

This is the brief example mentioned above.

var promise1, promise2;

promise1 ?? x; // this is considered using `promise1`
promise2 ??= x; // so this should be considered using `promise2`

if the cases are considered used, then why should the following scenario be considered as not being used?

var promise1, promise2;

promise1 + 1; // this is considered using `promise1`
promise2 += 1; // however, the current test cases expects `promise2` is never used 

in my opinion, the +=, |= and ??= are all AssignmentExpressions, right?

If we expect promise2 ??= x; to be considered used, should promise2 += x; to be considered used?

I apologize for the lengthy description, but I believe it is necessary to ensure the specific expected behavior. Only then can I determine whether I need to accommodate just the current issue or make adjustments to the previous behavior and corresponding test cases as well.

I am eagerly looking forward to your response.

[Skalman]

If we expect promise2 ??= x; to be considered used, should promise2 += x; to be considered used?

No. In promise2 ??= x, x is conditionally evaluated, but in promise2 += x, x is always evaluated, making promise2 unnecessary.

// These are equivalent
promise2 ??= x;

if (promise2 == null) { // <-- it's used here
  promise2 = x;
}

Such an if statement is hidden in ??=, ||= and &&=, but not in any of the other assignment operators.

[gweesin]

Thank you for the explanation. I think I understand why we need to do it this way. I will consider how to adjust the code accordingly.