From 30d53de3563cdc161b96528f8ddae32df7c67c9c Mon Sep 17 00:00:00 2001
From: dakkar <dakkar@thenautilus.net>
Date: Wed, 16 Oct 2024 14:58:59 +0100
Subject: [PATCH] fix argument/parameter checking

---
 eslint/locale.js      | 73 ++++++++++++++++++++++++++++++++++++++-----
 eslint/locale.test.js |  7 +++--
 2 files changed, 69 insertions(+), 11 deletions(-)

diff --git a/eslint/locale.js b/eslint/locale.js
index 746bff88a5..60ae21e6e5 100644
--- a/eslint/locale.js
+++ b/eslint/locale.js
@@ -50,6 +50,36 @@ function findCallExpression(node) {
 	return null;
 }
 
+function areArgumentsOneObject(node) {
+	return node.arguments.length === 1 &&
+		node.arguments[0].type === 'ObjectExpression';
+}
+
+// only call if `areArgumentsOneObject(node)` is true
+function getArgumentObjectProperties(node) {
+	return new Set(node.arguments[0].properties.map(
+		p => {
+			if (p.key && p.key.type === 'Identifier') return p.key.name;
+			return null;
+		},
+	));
+}
+
+function getTranslationParameters(translation) {
+	return new Set(Array.from(translation.matchAll(/\{(\w+)\}/g)).map( m => m[1] ));
+}
+
+function setDifference(a,b) {
+	const result = [];
+	for (const element of a.values()) {
+		if (!b.has(element)) {
+			result.push(element);
+		}
+	}
+
+	return result;
+}
+
 /* the actual rule body
  */
 function theRule(context) {
@@ -73,8 +103,8 @@ function theRule(context) {
 			const pathStr = `i18n.${method}.${path.join('.')}`;
 
 			// does that path point to a real translation?
-			const matchingNode = walkDown(locale, path);
-			if (!matchingNode) {
+			const translation = walkDown(locale, path);
+			if (!translation) {
 				context.report({
 					node,
 					message: `translation missing for ${pathStr}`,
@@ -84,7 +114,7 @@ function theRule(context) {
 
 			// some more checks on how the translation is called
 			if (method == 'ts') {
-				if (matchingNode.match(/\{/)) {
+				if (translation.match(/\{/)) {
 					context.report({
 						node,
 						message: `translation for ${pathStr} is parametric, but called via 'ts'`,
@@ -101,7 +131,7 @@ function theRule(context) {
 			}
 
 			if (method == 'tsx') {
-				if (!matchingNode.match(/\{/)) {
+				if (!translation.match(/\{/)) {
 					context.report({
 						node,
 						message: `translation for ${pathStr} is not parametric, but called via 'tsx'`,
@@ -110,7 +140,6 @@ function theRule(context) {
 				}
 
 				const callExpression = findCallExpression(node);
-
 				if (!callExpression) {
 					context.report({
 						node,
@@ -119,14 +148,42 @@ function theRule(context) {
 					return;
 				}
 
-				const parameterCount = [...matchingNode.matchAll(/\{/g)].length ?? 0;
-				const argumentCount = callExpression.arguments.length;
+				if (!areArgumentsOneObject(callExpression)) {
+					context.report({
+						node,
+						message: `translation for ${pathStr} should be called with a single object as argument`,
+					});
+					return;
+				}
+
+				const translationParameters = getTranslationParameters(translation);
+				const parameterCount = translationParameters.size;
+				const callArguments = getArgumentObjectProperties(callExpression);
+				const argumentCount = callArguments.size;
+
 				if (parameterCount !== argumentCount) {
 					context.report({
 						node,
 						message: `translation for ${pathStr} has ${parameterCount} parameters, but is called with ${argumentCount} arguments`,
 					});
-					return;
+				}
+
+				// node 20 doesn't have `Set.difference`...
+				const extraArguments = setDifference(callArguments, translationParameters);
+				const missingArguments = setDifference(translationParameters, callArguments);
+
+				if (extraArguments.length > 0) {
+					context.report({
+						node,
+						message: `translation for ${pathStr} passes unused arguments ${extraArguments.join(' ')}`,
+					});
+				}
+
+				if (missingArguments.length > 0) {
+					context.report({
+						node,
+						message: `translation for ${pathStr} does not pass arguments ${missingArguments.join(' ')}`,
+					});
 				}
 			}
 		},
diff --git a/eslint/locale.test.js b/eslint/locale.test.js
index 2ba1fb3d24..f08950b8c7 100644
--- a/eslint/locale.test.js
+++ b/eslint/locale.test.js
@@ -12,7 +12,7 @@ ruleTester.run(
     valid: [
       {code: 'i18n.ts.foo.bar', options: [locale] },
       {code: 'i18n.ts.top', options: [locale] },
-      {code: 'i18n.tsx.foo.baz(1)', options: [locale] },
+      {code: 'i18n.tsx.foo.baz({x:1})', options: [locale] },
       {code: 'whatever.i18n.ts.blah.blah', options: [locale] },
       {code: 'whatever.i18n.tsx.does.not.matter', options: [locale] },
       {code: 'whatever(i18n.ts.foo.bar)', options: [locale] },
@@ -20,10 +20,11 @@ ruleTester.run(
     invalid: [
       {code: 'i18n.ts.not', options: [locale], errors: 1 },
       {code: 'i18n.tsx.deep.not', options: [locale], errors: 1 },
-      {code: 'i18n.tsx.deep.not(12)', options: [locale], errors: 1 },
-      {code: 'i18n.tsx.top(1)', options: [locale], errors: 1 },
+      {code: 'i18n.tsx.deep.not({x:12})', options: [locale], errors: 1 },
+      {code: 'i18n.tsx.top({x:1})', options: [locale], errors: 1 },
       {code: 'i18n.ts.foo.baz', options: [locale], errors: 1 },
       {code: 'i18n.tsx.foo.baz', options: [locale], errors: 1 },
+      {code: 'i18n.tsx.foo.baz({y:2})', options: [locale], errors: 2 },
     ],
   },
 );