Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from Feb 10, 2021

Conversation

duncanbeevers
Copy link
Contributor

@duncanbeevers duncanbeevers commented Jan 18, 2021

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

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

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jan 18, 2021
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
ljharb
ljharb approved these changes Jan 18, 2021
Copy link
Sponsor Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Otherwise, this LGTM!

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 when the (new) ignoreJSX configuration option is set
@mdjermanovic mdjermanovic added enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Jan 18, 2021
@duncanbeevers
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

I changed the no-unused-expression option name
ignoreJSXdisallowJSX

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

@btmills
Copy link
Member

btmills commented Jan 22, 2021

@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
Copy link
Sponsor Contributor

ljharb commented Jan 22, 2021

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
Copy link
Contributor Author

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
Copy link
Member

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?

@mdjermanovic mdjermanovic added the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Jan 28, 2021
@btmills btmills added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion tsc agenda This issue will be discussed by ESLint's TSC at the next meeting labels Jan 29, 2021
Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -50,6 +50,10 @@ module.exports = {
allowTaggedTemplates: {
type: "boolean",
default: false
},
disallowJSX: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe enforceForJSX?

tests/lib/rules/no-unused-expressions.js Show resolved Hide resolved
@duncanbeevers
Copy link
Contributor Author

duncanbeevers commented Jan 30, 2021

  • 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.

This was referenced Mar 17, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Aug 10, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Aug 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants