ESLint Plugin: Fix no-unused-vars-before-return JSX identifier refere…

…nce (#21358) * ESLint Plugin: Fix no-unused-vars-before-return JSX identifier reference * Block Library: Social Link: Remove fixed ESLint disabling
WordPress · Apr 17, 2020 · c14ae55 · c14ae55
1 parent bed93c8
commit c14ae55
Show file tree

Hide file tree

Showing 4 changed files with 82 additions and 18 deletions.
diff --git a/packages/block-library/src/social-link/edit.js b/packages/block-library/src/social-link/edit.js
@@ -33,11 +33,6 @@ const SocialLinkEdit = ( { attributes, setAttributes, isSelected } ) => {
 		'wp-social-link__is-incomplete': ! url,
 	} );
 
-	// Disable reason: The rule is currently not considering use as JSX tagName.
-	//
-	// See: https://github.com/WordPress/gutenberg/issues/16418
-
-	// eslint-disable-next-line @wordpress/no-unused-vars-before-return
 	const IconComponent = getIconBySite( service );
 	const socialLinkName = getNameBySite( service );
 

diff --git a/packages/eslint-plugin/CHANGELOG.md b/packages/eslint-plugin/CHANGELOG.md
@@ -1,5 +1,9 @@
 ## Master
 
+### Bug Fixes
+
+- The `@wordpress/no-unused-vars-before-return` rule will now correctly identify valid usage of a variable as a JSX identifier.
+
 ## 5.0.1 (2020-04-15)
 
 ### Bug Fixes

diff --git a/packages/eslint-plugin/rules/__tests__/no-unused-vars-before-return.js b/packages/eslint-plugin/rules/__tests__/no-unused-vars-before-return.js
@@ -11,6 +11,9 @@ import rule from '../no-unused-vars-before-return';
 const ruleTester = new RuleTester( {
 	parserOptions: {
 		ecmaVersion: 6,
+		ecmaFeatures: {
+			jsx: true,
+		},
 	},
 } );
 
@@ -50,6 +53,13 @@ function example() {
 }`,
 			options: [ { excludePattern: '^do' } ],
 		},
+		{
+			code: `
+function MyComponent() {
+	const Foo = getSomeComponent();
+	return <Foo />;
+}`,
+		},
 	],
 	invalid: [
 		{

diff --git a/packages/eslint-plugin/rules/no-unused-vars-before-return.js b/packages/eslint-plugin/rules/no-unused-vars-before-return.js
@@ -1,4 +1,33 @@
-module.exports = {
+/** @typedef {import('eslint').Scope.Scope} ESLintScope */
+/** @typedef {import('eslint').Rule.RuleContext} ESLintRuleContext */
+/** @typedef {import('estree').Node} ESTreeNode */
+
+/**
+ * Mapping of function scope objects to a set of identified JSX identifiers
+ * within that scope.
+ *
+ * @type {WeakMap<ESLintScope,Set<ESTreeNode>>}
+ */
+const FUNCTION_SCOPE_JSX_IDENTIFIERS = new WeakMap();
+
+/**
+ * Returns the closest function scope for the current ESLint context object, or
+ * undefined if it cannot be determined.
+ *
+ * @param {ESLintRuleContext} context ESLint context object.
+ *
+ * @return {ESLintScope|undefined} Function scope, if known.
+ */
+function getClosestFunctionScope( context ) {
+	let functionScope = context.getScope();
+	while ( functionScope.type !== 'function' && functionScope.upper ) {
+		functionScope = functionScope.upper;
+	}
+
+	return functionScope;
+}
+
+module.exports = /** @type {import('eslint').Rule} */ ( {
 	meta: {
 		type: 'problem',
 		schema: [
@@ -13,6 +42,9 @@ module.exports = {
 			},
 		],
 	},
+	/**
+	 * @param {ESLintRuleContext} context Rule context.
+	 */
 	create( context ) {
 		const options = context.options[ 0 ] || {};
 		const { excludePattern } = options;
@@ -36,15 +68,27 @@ module.exports = {
 		}
 
 		return {
-			ReturnStatement( node ) {
-				let functionScope = context.getScope();
-				while (
-					functionScope.type !== 'function' &&
-					functionScope.upper
-				) {
-					functionScope = functionScope.upper;
+			JSXIdentifier( node ) {
+				// Currently, a scope's variable references does not include JSX
+				// identifiers. Account for this by visiting JSX identifiers
+				// first, and tracking them in a map per function scope, which
+				// is later merged with the known variable references.
+				const functionScope = getClosestFunctionScope( context );
+				if ( ! functionScope ) {
+					return;
+				}
+
+				if ( ! FUNCTION_SCOPE_JSX_IDENTIFIERS.has( functionScope ) ) {
+					FUNCTION_SCOPE_JSX_IDENTIFIERS.set(
+						functionScope,
+						new Set()
+					);
 				}
 
+				FUNCTION_SCOPE_JSX_IDENTIFIERS.get( functionScope ).add( node );
+			},
+			'ReturnStatement:exit'( node ) {
+				const functionScope = getClosestFunctionScope( context );
 				if ( ! functionScope ) {
 					return;
 				}
@@ -79,11 +123,22 @@ module.exports = {
 
 					// The first entry in `references` is the declaration
 					// itself, which can be ignored.
-					const isUsedBeforeReturn = variable.references
+					const identifiers = variable.references
 						.slice( 1 )
-						.some( ( reference ) => {
-							return reference.identifier.end < node.end;
-						} );
+						.map( ( reference ) => reference.identifier );
+
+					// Merge with any JSX identifiers in scope, if any.
+					if ( FUNCTION_SCOPE_JSX_IDENTIFIERS.has( functionScope ) ) {
+						const jsxIdentifiers = FUNCTION_SCOPE_JSX_IDENTIFIERS.get(
+							functionScope
+						);
+
+						identifiers.push( ...jsxIdentifiers );
+					}
+
+					const isUsedBeforeReturn = identifiers.some(
+						( identifier ) => identifier.end < node.end
+					);
 
 					if ( isUsedBeforeReturn ) {
 						continue;
@@ -98,4 +153,4 @@ module.exports = {
 			},
 		};
 	},
-};
+} );