Update: add enforceForJSX option to no-unused-expressions rule

[duncanbeevers]

React's createElement call is side-effect free, as are most JSX pragmas.
An unused JSX element indicates a logic error in the same way any unused, side-effect free expression is.
This extension the no-unused-expression rule flags unused JSX elements unless the (new) allowJsx configuration option is set

Prerequisites checklist

• I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

• Documentation update
• Bug fix (template)
• New rule (template)
• Changes an existing rule (template)
• Add autofixing to a rule
• Add a CLI option
• Add something to the core
• Other, please explain:

What rule do you want to change?
no-unused-expression

Does this change cause the rule to produce more or fewer warnings?
More warnings; Unused JSX expressions are newly flagged

How will the change be implemented? (New option, new default behavior, etc.)?
New default behavior; new option to opt-out

No new default behavior; new option to opt-in

Please provide some example code that this change will affect:

// Flagged (with opt-in)
<div />

// Unflagged
const partial = <div />

What does the rule currently do for this code?
JSX expressions are currently ignored by no-unused-expression

What will the rule do after it's changed?
Unused JSX expressions will be flagged

What changes did you make? (Give an overview)

Added recognition for JSXElement in the no-unused-expression rule Checker, following the code patterns already in-place in the rule.
Added tests demonstrating flagged and unflagged code, and how the allowJsx option interacts with such code.

Is there anything you'd like reviewers to focus on?

This originally came out of this eslint-plugin-react PR.
The scope of this PR is somewhat smaller and had a couple of key differences.

• It includes an escape hatch (allowJsx option) since some JSX pragmas may not be side-effect-free
• It does not recognize literal React.createElement() calls; the original PR, focused on React, flagged these unused expressions

[ljharb]

allowJsx that defaults to false seems like a potential breaking change; it should probably be ignoreJSX that defaults to false?

Otherwise, this LGTM!

[duncanbeevers]

I've updated this PR, replacing the acceptJsx option with an ignoreJSX option.

The naming seems a little weird; I thought disallowJSX might be closer to what's intended, but ignoreJSX matches the option used by no-unused-parens, so it's not without precedent.

[mdjermanovic]

This used to be reported by default until ESLint v7.5.0.

In 6ea3178 we changed the logic of this rule, from reporting all expressions except those for which we know that they have side effects, to reporting only known expressions for which we are sure that they don't have side effects.

The goal was to avoid possible false positives with unknown nodes like experimental syntax, but it's possible that we forgot about JSX.

That said, it does seem better to have an option for JSX rather than reverting the old behavior, since we can't know whether it is React or something else.

[duncanbeevers]

I changed the no-unused-expression option name
ignoreJSX→disallowJSX

The naming reflects the opt-in nature of the API;
"Disallow unused JSX expressions? Yes!"

[btmills]

@ljharb in the eslint-plugin-react PR, you wrote "A jsx element can’t be side-effecting." I know that's true for React, and I haven't encountered any non-React uses where it's false, but is it valid to assume it's always true? Since JSX is sugar for a function call, it'd be up to the semantics of whatever JSX pragma someone's using.

I ask because I think it makes a difference to end user developers. If ESLint can't assume JSX is side-effect free in all cases, checking for unused JSX would require developers to first be aware of this option and then enable it.

eslint-plugin-react can assume that JSX has no side effects. If you were to add the proposed rule to eslint-plugin-react, what are the chances it'd make it into react/recommended? That seems like it would help the largest number of developers.

I realize a whole new rule is a lot more implementation than an option on the built-in rule. I'm open to adding this option, but I think doing it this way would help fewer developers avoid the exact footguns that inspired the original rule proposal.

[ljharb]

I agree it could be theoretically side-effecting, but jsx isn’t a standard - it’s react-specific syntax that a few other ecosystems have co-opted, and as you say, all co-opted similarly.

Adding a new rule to a config is a breaking change, and i don't plan for one any time soon, but surely it would go there on the next semver-major, should such a rule exist.

However, this seems like a reasonable option here, since it wouldn’t be enabled by default, and any ecosystem that chose to make jsx have side effects would likely find “tell users to disable this rule’s option” to be the least of their problems.

[duncanbeevers]

any ecosystem that chose to make jsx have side effects would likely find “tell users to disable this rule’s option” to be the least of their problems.

😂

[mdjermanovic]

TSC Summary: no-unused-expressions currently doesn't report JSX expressions. This might be an unintentional regression introduced in v7.5.0, when we switched from maintaining a list of nodes that shouldn't be reported to maintaining a list of nodes that should be reported. On the other hand, the actual behavior (not reporting JSX) is in line with our policy of not assuming any semantics for JSX, per which we can't know whether or not JSX is side-effect free.

This PR aims to add an option for JSX expressions to the no-unused-expressions rule. Default value retains the current v7.18.0 behavior (not reporting JSX). When set, it restores pre-v7.5.0 behavior (reporting JSX).

TSC Question: shall we accept this PR?

[btmills]

We're on board to add this as an opt-in option, so I've labeled it as accepted!

I feel kind of guilty bikeshedding the option name, but it took me a minute to resolve the double negative, and I'm wondering if it's just me. Once we've settled on the name, can you document it in docs/rules/no-unused-expressions.md and indicate that it's disabled by default?

[btmills]

I see how the name disallowJSX follows from the implementation, but I'm wondering if the double negative might confuse users. For consistency with the other options, I could go for your original option name allowJSX but defaulting to true so it's not a breaking change. I also think checkJSX defaulting to false would be clear. Anyone have any preferences?

[ljharb]

It’s always super weird when absence and false aren’t the same - iow, i think a boolean option should never default to true.

[btmills]

That’s a great point. I also wasn’t thrilled about another “allow” option having the opposite default, so checkJSX seems like the better trade off now.

[mdjermanovic]

Maybe enforceForJSX?

[duncanbeevers]

• Updated option name to enforceForJSX
• Added valid tests cases for enforceForJSX option-enabled element and fragment usages
• Added simple documentation for the option to docs/rules/no-unused-expressions.md

I considered adding more explanation to the documentation, but I didn't want to lead readers into the weeds.
Hopefully the simple explanation suffices.