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

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

Merged
merged 5 commits into from Jan 30, 2019

Conversation

Projects
None yet
3 participants
@aduth
Copy link
Member

aduth commented Dec 12, 2018

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

This comment has been minimized.

Copy link
Member Author

aduth commented Dec 12, 2018

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

@ntwb

This comment has been minimized.

Copy link
Member

ntwb commented Dec 13, 2018

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

"eslint": "^4.19.1",

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Dec 13, 2018

@ntwb It's an included change:

"eslint": "^5.10.0",

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Dec 13, 2018

Consider unit tests for the rule itself

It certainly works as expected on Travis :)

✖ 65 problems (62 errors, 3 warnings)

@gziolo
Copy link
Member

gziolo left a comment

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?

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

This comment has been minimized.

@gziolo

gziolo Dec 13, 2018

Member

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

This comment has been minimized.

@aduth

aduth Dec 18, 2018

Author Member

It’s equivalent to something like:

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

This comment has been minimized.

Copy link
Member

gziolo commented Dec 13, 2018

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 aduth force-pushed the add/eslint-rule-no-unused-before-return branch from fddb736 to 6777085 Jan 18, 2019

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Jan 18, 2019

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.

@aduth aduth force-pushed the add/eslint-rule-no-unused-before-return branch from 6777085 to cc4642f Jan 25, 2019

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Jan 25, 2019

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

gziolo approved these changes Jan 28, 2019

Copy link
Member

gziolo left a comment

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 💯

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

This comment has been minimized.

@gziolo

gziolo Jan 28, 2019

Member

Reads nicely 👍


### Breaking Changes

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

This comment has been minimized.

@gziolo

gziolo Jan 28, 2019

Member

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.


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

This comment has been minimized.

@gziolo

gziolo Jan 28, 2019

Member

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 :)

This comment has been minimized.

@aduth

aduth Jan 28, 2019

Author Member

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 comment has been minimized.

@gziolo

gziolo Jan 29, 2019

Member

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

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

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 30, 2019

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.

@aduth aduth merged commit b9e9aed into master Jan 30, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the add/eslint-rule-no-unused-before-return branch Jan 30, 2019

daniloercoli added a commit that referenced this pull request Feb 1, 2019

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment