eslint-plugin: Add rule `no-unused-vars-before-return` #12828

aduth · 2018-12-12T22:27:34Z

Related: #12477
~~Blocked by #12827~~ (since merged)

This pull request seeks to introduce a new ESLint rule @wordpress/no-unused-vars-before-return. This rule captures potential performance issues related to the assignment of a variable from the result of a function call expression, where the declared variable is not referenced prior to a possible return path of the function.

Practically speaking, this looks like:

function example( number ) {
	const foo = doSomeCostlyOperation();
	if ( number > 10 ) {
		// Why did we bother doing the costly operation in the assignment
		// above, if it's never to be used when this logic path is satisfied?
		return number + 1;
	}

	// ...instead, it should have been assigned here, at the last possible point
	// before it's referenced.

	return number + foo;
}

Refer also to modified code in #12827 as example problematic code.

Try yourself: https://astexplorer.net/#/gist/43c7ba92c2ea5bce1cfdd870c0ad6c4e/432215c7751e8dcd2d50463c23459abd661c7572

Testing instructions:

~~(This is blocked by #12827 - You will observe lint errors in the meantime)~~

Ensure there are no lint errors:

npm run lint-js

Pending tasks:

Consider unit tests for the rule itself
Add documentation for rule usage

aduth · 2018-12-12T22:31:56Z

ESLint version was bumped because it adds support for the npm organization scope for reference in rules.

ntwb · 2018-12-13T01:39:29Z

Should the ESLint version also be bumped in @wordPress/scripts?

gutenberg/packages/scripts/package.json

Line 41 in 6404578

"eslint": "^4.19.1",

aduth · 2018-12-13T13:25:13Z

@ntwb It's an included change:

gutenberg/packages/scripts/package.json

Line 41 in fddb736

"eslint": "^5.10.0",

gziolo · 2018-12-13T14:50:45Z

Consider unit tests for the rule itself

It certainly works as expected on Travis :)

✖ 65 problems (62 errors, 3 warnings)

gziolo

It works great. I'm not super familiar with Eslint API and stuff, but the implementation looks good as far as I can tell.

We should add documentation which explains how this new rule work and changelog entry. How do you plan to tackle all errors? Should we open another PR and merge it sooner?

gziolo · 2018-12-13T14:55:50Z

packages/eslint-plugin/rules/index.js

@@ -0,0 +1 @@
+module.exports = require( 'requireindex' )( __dirname );


is it equivalent to re-exporting all files in the folder? just being curious :)

It’s equivalent to something like:

module.exports = { foo: require( './foo' ), bar: require( './bar' ), baz: require( './baz' ), };

gziolo · 2018-12-13T15:37:24Z

Consider unit tests for the rule itself

Ah right. That would be nice to have an infrastructure in place for rule's verification in general. I assume we will have more rules in the future. This might be similar to what we use for Babel preset: https://github.com/WordPress/gutenberg/blob/master/packages/babel-preset-default/test/index.js

aduth · 2019-01-18T22:50:38Z

I've rebased this to bring it up to date, and have added documentation / tests for the new rule which can hopefully serve as precedent for future rule additions.

packages/editor/src/components/block-switcher/index.js

packages/eslint-plugin/rules/__tests__/no-unused-vars-before-return.js

aduth · 2019-01-25T19:53:17Z

I separately addressed the style issues for BlockSwitcher in #13431 .

In the rebased branch, there are no instances of offending code in the codebase.

Other changes included in the rebase:

Bumped eslint to the latest latest (5.12.1, previously 5.10.0)
Marked the scripts change as a breaking change, since we're bumping a major version of a dependency

gziolo

LGTM, I really like this rule as it helps to keep functionality close to the place where it is actually used. Great work on this custom rule 💯

gziolo · 2019-01-28T12:11:31Z

packages/eslint-plugin/docs/rules/no-unused-vars-before-return.md

@@ -0,0 +1,31 @@
+# Avoid assigning variable values if unused before a return (no-unused-vars-before-return)
+
+To avoid wastefully computing the result of a function call when assigning a variable value, the variable should be declared as late as possible. In practice, if a function includes an early return path, any variable declared prior to the `return` must be referenced at least once. Otherwise, in the condition which satisfies that return path, the declared variable is effectively considered dead code. This can have a performance impact when the logic performed in assigning the value is non-trivial.


Reads nicely 👍

gziolo · 2019-01-28T12:13:03Z

packages/scripts/CHANGELOG.md

+
+### Breaking Changes
+
+- The bundled `eslint` dependency has been updated from requiring `^4.19.1` to requiring `^5.12.1`.


We should also bump Babel at the same time and apply some tweaks to the preset we ship in the same version bump. I will open another PR with changes proposed.

gziolo · 2019-01-28T12:18:59Z

packages/eslint-plugin/rules/no-unused-vars-before-return.js

+
+					context.report(
+						node,
+						`Declared variable \`${ variable.name }\` is unused before a return path`


I'm not quite sure that is going to be clear enough when printed on the console. Should it include more detailed info that explains that there is a return path where it is not used before it is even used? I doesn't read good but I hope it explain my concern. Anyway, I don't find it as blocker :)

I played with the message a bit more. One thing which confused me is its placement on the return statement vs. the assignment itself, which I've found can be adjusted. Though there's also some value in having it on the actual return which flags it as offending 🤷‍♂️

Any thoughts on (attached to the variable declaration):

Variables should not be assigned until just prior its first reference. An early return statement may leave this variable unused.

This is much better. Let's proceed with it 👍

gziolo · 2019-01-30T11:19:33Z

I pushed the fix which should fix the failing test: 31a46bf. Unit tests needed to be updated to reflect the change in the rule error message.

… note

…rnmobile/372-use-RichText-on-Title-block * 'master' of https://github.com/WordPress/gutenberg: Try alternate list item jump fix. (#12941) Mobile bottom sheet component (#13612) Remove unintentional right-margin on last odd-item. (#12199) Introduce left and right float alignment options to latest posts block (#8814) Fix Google Docs table paste (#13543) Increase bottom padding on gallery image caption (#13623) Fix the editor save keyboard shortcut not working in code editor view (#13159) Plugin: Deprecate gutenberg_add_admin_body_class (#13572) Rnmobile/upload media failed state (#13615) Make clickOnMoreMenuItem not dependent on aria labels (#13166) Add: className prop support to server side render. (#13568) Fix: Categories Block: hierarchical Dropdown (#13567) Docs: Add clarification about git workflow (#13534) Plugin: Remove `user_can_richedit` filtering (#13608) eslint-plugin: Add rule `no-unused-vars-before-return` (#12828) Image settings button (#13597) Fixed wording for the color picker saturation (#13479) # Conflicts: # packages/block-library/src/image/edit.native.js

* scripts: Bump ESLint dependency to 5.10.x * eslint-plugin: Add rule `no-unused-vars-before-return` * eslint-plugin: Rephrase return-unused message for declarator * Update error message in the unit test to reflect code changes * Scripts: Include Migration Guide link for ESLint major bump CHANGELOG note

aduth added the [Package] ESLint plugin /packages/eslint-plugin label Dec 12, 2018

aduth requested review from gziolo, ntwb and jorgefilipecosta December 12, 2018 22:27

aduth mentioned this pull request Dec 12, 2018

Performance: Move unused variable initializations after early return #12827

Merged

gziolo reviewed Dec 13, 2018

View reviewed changes

aduth mentioned this pull request Jan 3, 2019

Always show icon in block toolbar #11600

Merged

aduth force-pushed the add/eslint-rule-no-unused-before-return branch from fddb736 to 6777085 Compare January 18, 2019 22:49

aduth commented Jan 18, 2019

View reviewed changes

packages/editor/src/components/block-switcher/index.js Outdated Show resolved Hide resolved

aduth commented Jan 18, 2019

View reviewed changes

packages/eslint-plugin/rules/__tests__/no-unused-vars-before-return.js Show resolved Hide resolved

aduth added 2 commits January 25, 2019 14:50

scripts: Bump ESLint dependency to 5.10.x

cb3da2a

eslint-plugin: Add rule no-unused-vars-before-return

cc4642f

aduth force-pushed the add/eslint-rule-no-unused-before-return branch from 6777085 to cc4642f Compare January 25, 2019 19:51

gziolo approved these changes Jan 28, 2019

View reviewed changes

gziolo added this to the 5.0 (Gutenberg) milestone Jan 28, 2019

eslint-plugin: Rephrase return-unused message for declarator

bdab122

aduth mentioned this pull request Jan 29, 2019

Improve default Babel preset to include JSX pragma #13540

Merged

Update error message in the unit test to reflect code changes

31a46bf

gziolo added the [Type] Code Quality Issues or PRs that relate to code quality label Jan 30, 2019

Scripts: Include Migration Guide link for ESLint major bump CHANGELOG…

034c075

… note

aduth merged commit b9e9aed into master Jan 30, 2019

aduth deleted the add/eslint-rule-no-unused-before-return branch January 30, 2019 20:38

This was referenced Feb 4, 2019

Bump plugin version to 5.0.0-rc.1 #13652

Merged

Bump plugin version to 5.0.0 #13689

Closed

Bump plugin version to 5.0.0 #13692

Merged

gziolo mentioned this pull request Feb 11, 2019

Make Babel import JSX pragma plugin aware of wp.element.createElement #13809

Merged

2 tasks

aduth mentioned this pull request Sep 29, 2020

Refactor form step validation as required field 18F/identity-idp#4243

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eslint-plugin: Add rule `no-unused-vars-before-return` #12828

eslint-plugin: Add rule `no-unused-vars-before-return` #12828

aduth commented Dec 12, 2018 •

edited

Loading

aduth commented Dec 12, 2018

ntwb commented Dec 13, 2018

aduth commented Dec 13, 2018

gziolo commented Dec 13, 2018

gziolo left a comment

gziolo Dec 13, 2018

aduth Dec 18, 2018

gziolo commented Dec 13, 2018

aduth commented Jan 18, 2019

aduth commented Jan 25, 2019

gziolo left a comment

gziolo Jan 28, 2019

gziolo Jan 28, 2019

gziolo Jan 28, 2019

aduth Jan 28, 2019

gziolo Jan 29, 2019

gziolo commented Jan 30, 2019

		@@ -0,0 +1 @@
		module.exports = require( 'requireindex' )( __dirname );

		@@ -0,0 +1,31 @@
		# Avoid assigning variable values if unused before a return (no-unused-vars-before-return)

		To avoid wastefully computing the result of a function call when assigning a variable value, the variable should be declared as late as possible. In practice, if a function includes an early return path, any variable declared prior to the `return` must be referenced at least once. Otherwise, in the condition which satisfies that return path, the declared variable is effectively considered dead code. This can have a performance impact when the logic performed in assigning the value is non-trivial.


		### Breaking Changes

		- The bundled `eslint` dependency has been updated from requiring `^4.19.1` to requiring `^5.12.1`.

eslint-plugin: Add rule no-unused-vars-before-return #12828

eslint-plugin: Add rule no-unused-vars-before-return #12828

Conversation

aduth commented Dec 12, 2018 • edited Loading

aduth commented Dec 12, 2018

ntwb commented Dec 13, 2018

aduth commented Dec 13, 2018

gziolo commented Dec 13, 2018

gziolo left a comment

Choose a reason for hiding this comment

gziolo Dec 13, 2018

Choose a reason for hiding this comment

aduth Dec 18, 2018

Choose a reason for hiding this comment

gziolo commented Dec 13, 2018

aduth commented Jan 18, 2019

aduth commented Jan 25, 2019

gziolo left a comment

Choose a reason for hiding this comment

gziolo Jan 28, 2019

Choose a reason for hiding this comment

gziolo Jan 28, 2019

Choose a reason for hiding this comment

gziolo Jan 28, 2019

Choose a reason for hiding this comment

aduth Jan 28, 2019

Choose a reason for hiding this comment

gziolo Jan 29, 2019

Choose a reason for hiding this comment

gziolo commented Jan 30, 2019

eslint-plugin: Add rule `no-unused-vars-before-return` #12828

eslint-plugin: Add rule `no-unused-vars-before-return` #12828

aduth commented Dec 12, 2018 •

edited

Loading