ESLint Plugin: Add rule valid-sprintf (#13756)

* Blocks: Embed: Avoid unneccessary sprintf

* ESLint Plugin: Add rule valid-sprintf

* i18n: Disable valid-sprintf for intentional error test case

packages/block-library/src/embed/settings.js

@@ -11,7 +11,7 @@ import classnames from 'classnames/dedupe';
 /**
  * WordPress dependencies
  */
-import { __, sprintf } from '@wordpress/i18n';
+import { __ } from '@wordpress/i18n';
 import { compose } from '@wordpress/compose';
 import { RichText } from '@wordpress/editor';
 import { withSelect, withDispatch } from '@wordpress/data';
@@ -38,8 +38,7 @@ const embedAttributes = {
 };
 
 export function getEmbedBlockSettings( { title, description, icon, category = 'embed', transforms, keywords = [], supports = {}, responsive = true } ) {
-	// translators: %s: Name of service (e.g. VideoPress, YouTube)
-	const blockDescription = description || sprintf( __( 'Add a block that displays content pulled from other sites, like Twitter, Instagram or YouTube.' ), title );
+	const blockDescription = description || __( 'Add a block that displays content pulled from other sites, like Twitter, Instagram or YouTube.' );
 	const edit = getEmbedEditComponent( title, icon, responsive );
 	return {
 		title,

packages/eslint-plugin/CHANGELOG.md

@@ -8,6 +8,7 @@
 
 - New Rule: [`@wordpress/no-unused-vars-before-return`](https://github.com/WordPress/gutenberg/blob/master/packages/eslint-plugin/docs/rules/no-unused-vars-before-return.md)
 - New Rule: [`@wordpress/dependency-group`](https://github.com/WordPress/gutenberg/blob/master/packages/eslint-plugin/docs/rules/dependency-group.md)
+- New Rule: [`@wordpress/valid-sprintf`](https://github.com/WordPress/gutenberg/blob/master/packages/eslint-plugin/docs/rules/valid-sprintf.md)
 
 ## 1.0.0 (2018-12-12)
 

packages/eslint-plugin/README.md

@@ -51,6 +51,7 @@ Rule|Description
 ---|---
 [no-unused-vars-before-return](/packages/eslint-plugin/docs/rules/no-unused-vars-before-return.md)|Disallow assigning variable values if unused before a return
 [dependency-group](/packages/eslint-plugin/docs/rules/dependency-group.md)|Enforce dependencies docblocks formatting
+[valid-sprintf](/packages/eslint-plugin/docs/rules/valid-sprintf.md)|Disallow assigning variable values if unused before a return
 
 ### Legacy
 

packages/eslint-plugin/configs/custom.js

@@ -5,6 +5,7 @@ module.exports = {
 	rules: {
 		'@wordpress/dependency-group': 'error',
 		'@wordpress/no-unused-vars-before-return': 'error',
+		'@wordpress/valid-sprintf': 'error',
 		'no-restricted-syntax': [
 			'error',
 			{

packages/eslint-plugin/docs/rules/valid-sprintf.md

@@ -0,0 +1,28 @@
+# Enforce valid sprintf usage (valid-sprintf)
+
+[`sprintf`](https://github.com/WordPress/gutenberg/blob/master/packages/i18n/README.md#api) must be called with a valid format string with at least one placeholder, and with a valid set of placeholder substitute values.
+
+## Rule details
+
+Examples of **incorrect** code for this rule:
+
+```js
+sprintf();
+sprintf( '%s' );
+sprintf( 1, 'substitute' );
+sprintf( [], 'substitute' );
+sprintf( '%%', 'substitute' );
+sprintf( __( '%%' ), 'substitute' );
+sprintf( _n( '%s', '' ), 'substitute' );
+sprintf( _n( '%s', '%s %s' ), 'substitute' );
+```
+
+Examples of **correct** code for this rule:
+
+```js
+sprintf( '%s', 'substitute' );
+sprintf( __( '%s' ), 'substitute' );
+sprintf( _x( '%s' ), 'substitute' );
+sprintf( _n( '%s', '%s' ), 'substitute' );
+sprintf( _nx( '%s', '%s' ), 'substitute' );
+```

packages/eslint-plugin/rules/__tests__/valid-sprintf.js

@@ -0,0 +1,75 @@
+/**
+ * External dependencies
+ */
+import { RuleTester } from 'eslint';
+
+/**
+ * Internal dependencies
+ */
+import rule from '../valid-sprintf';
+
+const ruleTester = new RuleTester( {
+	parserOptions: {
+		ecmaVersion: 6,
+	},
+} );
+
+ruleTester.run( 'valid-sprintf', rule, {
+	valid: [
+		{
+			code: `sprintf( '%s', 'substitute' )`,
+		},
+		{
+			code: `sprintf( __( '%s' ), 'substitute' )`,
+		},
+		{
+			code: `sprintf( _x( '%s' ), 'substitute' )`,
+		},
+		{
+			code: `sprintf( _n( '%s', '%s' ), 'substitute' )`,
+		},
+		{
+			code: `sprintf( _nx( '%s', '%s' ), 'substitute' )`,
+		},
+		{
+			code: `var getValue = () => ''; sprintf( getValue(), 'substitute' )`,
+		},
+		{
+			code: `var value = ''; sprintf( value, 'substitute' )`,
+		},
+	],
+	invalid: [
+		{
+			code: `sprintf()`,
+			errors: [ { message: 'sprintf must be called with a format string' } ],
+		},
+		{
+			code: `sprintf( '%s' )`,
+			errors: [ { message: 'sprintf must be called with placeholder value argument(s)' } ],
+		},
+		{
+			code: `sprintf( 1, 'substitute' )`,
+			errors: [ { message: 'sprintf must be called with a valid format string' } ],
+		},
+		{
+			code: `sprintf( [], 'substitute' )`,
+			errors: [ { message: 'sprintf must be called with a valid format string' } ],
+		},
+		{
+			code: `sprintf( '%%', 'substitute' )`,
+			errors: [ { message: 'sprintf format string must contain at least one placeholder' } ],
+		},
+		{
+			code: `sprintf( __( '%%' ), 'substitute' )`,
+			errors: [ { message: 'sprintf format string must contain at least one placeholder' } ],
+		},
+		{
+			code: `sprintf( _n( '%s', '' ), 'substitute' )`,
+			errors: [ { message: 'sprintf format string options must have the same number of placeholders' } ],
+		},
+		{
+			code: `sprintf( _n( '%s', '%s %s' ), 'substitute' )`,
+			errors: [ { message: 'sprintf format string options must have the same number of placeholders' } ],
+		},
+	],
+} );

packages/eslint-plugin/rules/valid-sprintf.js

@@ -0,0 +1,150 @@
+/**
+ * Regular expression matching the presence of a printf format string
+ * placeholder. This naive pattern which does not validate the format.
+ *
+ * @type {RegExp}
+ */
+const REGEXP_PLACEHOLDER = /%[^%]/g;
+
+/**
+ * Given a function name and array of argument Node values, returns all
+ * possible string results from the corresponding translate function, or
+ * undefined if the function is not a translate function.
+ *
+ * @param {string}        functionName Function name.
+ * @param {espree.Node[]} args         Espree argument Node objects.
+ *
+ * @return {?Array<string>} All possible translate function string results.
+ */
+function getTranslateStrings( functionName, args ) {
+	switch ( functionName ) {
+		case '__':
+		case '_x':
+			args = args.slice( 0, 1 );
+			break;
+
+		case '_n':
+		case '_nx':
+			args = args.slice( 0, 2 );
+			break;
+
+		default:
+			return;
+	}
+
+	return args
+		.filter( ( arg ) => arg.type === 'Literal' )
+		.map( ( arg ) => arg.value );
+}
+
+module.exports = {
+	meta: {
+		type: 'problem',
+		schema: [],
+	},
+	create( context ) {
+		return {
+			CallExpression( node ) {
+				const { callee, arguments: args } = node;
+				if ( callee.name !== 'sprintf' ) {
+					return;
+				}
+
+				if ( ! args.length ) {
+					context.report(
+						node,
+						'sprintf must be called with a format string'
+					);
+					return;
+				}
+
+				if ( args.length < 2 ) {
+					context.report(
+						node,
+						'sprintf must be called with placeholder value argument(s)'
+					);
+					return;
+				}
+
+				let candidates;
+				switch ( args[ 0 ].type ) {
+					case 'Literal':
+						candidates = [ args[ 0 ].value ].filter( ( arg ) => {
+							// Since a Literal may be a number, verify the
+							// value is a string.
+							return typeof arg === 'string';
+						} );
+						break;
+
+					case 'CallExpression':
+						// All possible options (arguments) from a translate
+						// function must be valid.
+						candidates = getTranslateStrings(
+							args[ 0 ].callee.name,
+							args[ 0 ].arguments
+						);
+
+						// An unknown function call may produce a valid string
+						// value. Ideally its result is verified, but this is
+						// not straight-forward to implement. Thus, bail.
+						if ( candidates === undefined ) {
+							return;
+						}
+
+						break;
+
+					case 'Identifier':
+						// Identifiers may refer to a valid string variable.
+						// Ideally its reference value is verified, but this is
+						// not straight-forward to implement. Thus, bail.
+						return;
+
+					default:
+						candidates = [];
+				}
+
+				if ( ! candidates.length ) {
+					context.report(
+						node,
+						'sprintf must be called with a valid format string'
+					);
+					return;
+				}
+
+				let numPlaceholders;
+				for ( let i = 0; i < candidates.length; i++ ) {
+					const match = candidates[ i ].match( REGEXP_PLACEHOLDER );
+
+					// Prioritize placeholder number consistency over matching
+					// placeholder, since it's a more common error to omit a
+					// placeholder from the singular form of pluralization.
+					if (
+						numPlaceholders !== undefined &&
+						( ! match || numPlaceholders !== match.length )
+					) {
+						context.report(
+							node,
+							'sprintf format string options must have the same number of placeholders'
+						);
+						return;
+					}
+
+					if ( ! match ) {
+						context.report(
+							node,
+							'sprintf format string must contain at least one placeholder'
+						);
+						return;
+					}
+
+					if ( numPlaceholders === undefined ) {
+						// Track the number of placeholders discovered in the
+						// string to verify that all other candidate options
+						// have the same number.
+						numPlaceholders = match.length;
+					}
+				}
+			},
+		};
+	},
+};

packages/i18n/src/test/index.js

@@ -68,6 +68,8 @@ describe( 'i18n', () => {
 
 	describe( 'sprintf()', () => {
 		it( 'absorbs errors', () => {
+			// Disable reason: Failing case is the purpose of the test.
+			// eslint-disable-next-line @wordpress/valid-sprintf
 			const result = sprintf( 'Hello %(placeholder-not-provided)s' );
 
 			expect( console ).toHaveErrored();