From 82674d871874a4e86bca1e39b081bbcf05127f61 Mon Sep 17 00:00:00 2001 From: dakkar Date: Wed, 16 Oct 2024 14:01:54 +0100 Subject: [PATCH 01/13] lint all uses of translations --- eslint/locale.js | 145 +++++++++++++++++++++++++++++ eslint/locale.test.js | 29 ++++++ packages/frontend/eslint.config.js | 5 + 3 files changed, 179 insertions(+) create mode 100644 eslint/locale.js create mode 100644 eslint/locale.test.js diff --git a/eslint/locale.js b/eslint/locale.js new file mode 100644 index 0000000000..b6f6f762be --- /dev/null +++ b/eslint/locale.js @@ -0,0 +1,145 @@ +/* This is a ESLint rule to report use of the `i18n.ts` and `i18n.tsx` + * objects that reference translation items that don't actually exist + * in the lexicon (the `locale/` files) + */ + +/* given a MemberExpression node, collects all the member names + * + * e.g. for a bit of code like `foo=one.two.three`, `collectMembers` + * called on the node for `three` would return `['one', 'two', + * 'three']` + */ +function collectMembers(node) { + if (!node) return []; + if (node.type !== 'MemberExpression') return []; + return [ node.property.name, ...collectMembers(node.parent) ]; +} + +/* given an object and an array of names, recursively descends the + * object via those names + * + * e.g. `walkDown({one:{two:{three:15}}},['one','two','three'])` would + * return 15 + */ +function walkDown(locale, path) { + if (!locale) return null; + if (!path || path.length === 0) return locale; + return walkDown(locale[path[0]], path.slice(1)); +} + +/* given a MemberExpression node, returns its attached CallExpression + * node if present + * + * e.g. for a bit of code like `foo=one.two.three()`, + * `findCallExpression` called on the node for `three` would return + * the node for function call (which is the parent of the `one` and + * `two` nodes, and holds the nodes for the argument list) + * + * if the code had been `foo=one.two.three`, `findCallExpression` + * would have returned null, because there's no function call attached + * to the MemberExpressions + */ +function findCallExpression(node) { + if (node.type === 'CallExpression') return node + if (node.parent?.type === 'CallExpression') return node.parent; + if (node.parent?.type === 'MemberExpression') return findCallExpression(node.parent); + return null; +} + +/* the actual rule body + */ +function theRule(context) { + // we get the locale/translations via the options; it's the data + // that goes into a specific language's JSON file, see + // `scripts/build-assets.mjs` + const locale = context.options[0]; + return { + // for all object member access that have an identifier 'i18n'... + 'MemberExpression:has(> Identifier[name=i18n])': (node) => { + // sometimes we get MemberExpression nodes that have a + // *descendent* with the right identifier: skip them, we'll get + // the right ones as well + if (node.object?.name != 'i18n') { + return; + } + + // `method` is going to be `'ts'` or `'tsx'`, `path` is going to + // be the various translation steps/names + const [ method, ...path ] = collectMembers(node); + const pathStr = `i18n.${method}.${path.join('.')}`; + + // does that path point to a real translation? + const matchingNode = walkDown(locale, path); + if (!matchingNode) { + context.report({ + node, + message: `translation missing for ${pathStr}`, + }); + return; + } + + // some more checks on how the translation is called + if (method == 'ts') { + if (matchingNode.match(/\{/)) { + context.report({ + node, + message: `translation for ${pathStr} is parametric, but called via 'ts'`, + }); + return; + } + + if (findCallExpression(node)) { + context.report({ + node, + message: `translation for ${pathStr} is not parametric, but is called as a function`, + }); + } + } + + if (method == 'tsx') { + if (!matchingNode.match(/\{/)) { + context.report({ + node, + message: `translation for ${pathStr} is not parametric, but called via 'tsx'`, + }); + return; + } + + const callExpression = findCallExpression(node); + + if (!callExpression) { + context.report({ + node, + message: `translation for ${pathStr} is parametric, but not called as a function`, + }); + return; + } + + const parameterCount = [...matchingNode.matchAll(/\{/g)].length ?? 0; + const argumentCount = callExpression.arguments.length; + if (parameterCount !== argumentCount) { + context.report({ + node, + message: `translation for ${pathStr} has ${parameterCount} parameters, but is called with ${argumentCount} arguments`, + }); + return; + } + } + }, + }; +} + +module.exports = { + meta: { + type: 'problem', + docs: { + description: 'assert that all translations used are present in the locale files', + }, + schema: [ + // here we declare that we need the locale/translation as a + // generic object + { type: 'object', additionalProperties: true }, + ], + }, + create: theRule, +}; diff --git a/eslint/locale.test.js b/eslint/locale.test.js new file mode 100644 index 0000000000..cf64961054 --- /dev/null +++ b/eslint/locale.test.js @@ -0,0 +1,29 @@ +const {RuleTester} = require("eslint"); +const localeRule = require("./locale"); + +const locale = { foo: { bar: 'ok', baz: 'good {x}' }, top: '123' }; + +const ruleTester = new RuleTester(); + +ruleTester.run( + 'sharkey-locale', + localeRule, + { + valid: [ + {code: 'i18n.ts.foo.bar', options: [locale] }, + {code: 'i18n.ts.top', options: [locale] }, + {code: 'i18n.tsx.foo.baz(1)', options: [locale] }, + {code: 'whatever.i18n.ts.blah.blah', options: [locale] }, + {code: 'whatever.i18n.tsx.does.not.matter', options: [locale] }, + ], + 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.ts.foo.baz', options: [locale], errors: 1 }, + {code: 'i18n.tsx.foo.baz', options: [locale], errors: 1 }, + ], + }, +); + diff --git a/packages/frontend/eslint.config.js b/packages/frontend/eslint.config.js index 28796e8d6b..2841a5592a 100644 --- a/packages/frontend/eslint.config.js +++ b/packages/frontend/eslint.config.js @@ -4,6 +4,8 @@ import parser from 'vue-eslint-parser'; import pluginVue from 'eslint-plugin-vue'; import pluginMisskey from '@misskey-dev/eslint-plugin'; import sharedConfig from '../shared/eslint.config.js'; +import localeRule from '../../eslint/locale.js'; +import { build as buildLocales } from '../../locales/index.js'; export default [ ...sharedConfig, @@ -14,6 +16,7 @@ export default [ ...pluginVue.configs['flat/recommended'], { files: ['{src,test,js,@types}/**/*.{ts,vue}'], + plugins: { sharkey: { rules: { locale: localeRule } } }, languageOptions: { globals: { ...Object.fromEntries(Object.entries(globals.node).map(([key]) => [key, 'off'])), @@ -44,6 +47,8 @@ export default [ }, }, rules: { + 'sharkey/locale': ['error', buildLocales()['ja-JP']], + '@typescript-eslint/no-empty-interface': ['error', { allowSingleExtends: true, }], From dba32772007a3d0f2caacdb3130a8519c1528b19 Mon Sep 17 00:00:00 2001 From: dakkar Date: Wed, 16 Oct 2024 14:13:05 +0100 Subject: [PATCH 02/13] fix CallExpression detection --- eslint/locale.js | 10 +++++++--- eslint/locale.test.js | 1 + 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/eslint/locale.js b/eslint/locale.js index b6f6f762be..746bff88a5 100644 --- a/eslint/locale.js +++ b/eslint/locale.js @@ -40,9 +40,13 @@ function walkDown(locale, path) { * to the MemberExpressions */ function findCallExpression(node) { - if (node.type === 'CallExpression') return node - if (node.parent?.type === 'CallExpression') return node.parent; - if (node.parent?.type === 'MemberExpression') return findCallExpression(node.parent); + if (!node.parent) return null; + + // the second half of this guard protects from cases like + // `foo(one.two.three)` where the CallExpression is parent of the + // MemberExpressions, but via `arguments`, not `callee` + if (node.parent.type === 'CallExpression' && node.parent.callee === node) return node.parent; + if (node.parent.type === 'MemberExpression') return findCallExpression(node.parent); return null; } diff --git a/eslint/locale.test.js b/eslint/locale.test.js index cf64961054..2ba1fb3d24 100644 --- a/eslint/locale.test.js +++ b/eslint/locale.test.js @@ -15,6 +15,7 @@ ruleTester.run( {code: 'i18n.tsx.foo.baz(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] }, ], invalid: [ {code: 'i18n.ts.not', options: [locale], errors: 1 }, From 30d53de3563cdc161b96528f8ddae32df7c67c9c Mon Sep 17 00:00:00 2001 From: dakkar Date: Wed, 16 Oct 2024 14:58:59 +0100 Subject: [PATCH 03/13] 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 }, ], }, ); From f11536c927e0ce4fdc18e3c179835cdde3942b5a Mon Sep 17 00:00:00 2001 From: dakkar Date: Wed, 16 Oct 2024 15:56:48 +0100 Subject: [PATCH 04/13] ignore weirder cases --- eslint/locale.js | 7 ++++++- eslint/locale.test.js | 2 ++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/eslint/locale.js b/eslint/locale.js index 60ae21e6e5..a686a4b616 100644 --- a/eslint/locale.js +++ b/eslint/locale.js @@ -23,7 +23,7 @@ function collectMembers(node) { */ function walkDown(locale, path) { if (!locale) return null; - if (!path || path.length === 0) return locale; + if (!path || path.length === 0 || !path[0]) return locale; return walkDown(locale[path[0]], path.slice(1)); } @@ -112,6 +112,11 @@ function theRule(context) { return; } + // we hit something weird, assume the programmers know what + // they're doing (this is usually some complicated slicing of + // the translation structure) + if (typeof(translation) !== 'string') return; + // some more checks on how the translation is called if (method == 'ts') { if (translation.match(/\{/)) { diff --git a/eslint/locale.test.js b/eslint/locale.test.js index f08950b8c7..626fe1587c 100644 --- a/eslint/locale.test.js +++ b/eslint/locale.test.js @@ -11,6 +11,8 @@ ruleTester.run( { valid: [ {code: 'i18n.ts.foo.bar', options: [locale] }, + // we don't detect the problem here, but should still accept it + {code: 'i18n.ts.foo["something"]', options: [locale] }, {code: 'i18n.ts.top', options: [locale] }, {code: 'i18n.tsx.foo.baz({x:1})', options: [locale] }, {code: 'whatever.i18n.ts.blah.blah', options: [locale] }, From b0bc24f01b16df61aa7b8b3781fb69f997286f64 Mon Sep 17 00:00:00 2001 From: dakkar Date: Wed, 16 Oct 2024 15:57:15 +0100 Subject: [PATCH 05/13] lint Vue templates as well the argument detection doesn't work inside templates when invoked via the `` component, because it's too complicated for me now --- eslint/locale.js | 241 ++++++++++++++++++++++++------------------ eslint/locale.test.js | 47 +++++--- 2 files changed, 168 insertions(+), 120 deletions(-) diff --git a/eslint/locale.js b/eslint/locale.js index a686a4b616..7e5f3419fc 100644 --- a/eslint/locale.js +++ b/eslint/locale.js @@ -50,6 +50,15 @@ function findCallExpression(node) { return null; } +// same, but for Vue expressions (``) +function findVueExpression(node) { + if (!node.parent) return null; + + if (node.parent.type.match(/^VExpr/) && node.parent.expression === node) return node.parent; + if (node.parent.type === 'MemberExpression') return findVueExpression(node.parent); + return null; +} + function areArgumentsOneObject(node) { return node.arguments.length === 1 && node.arguments[0].type === 'ObjectExpression'; @@ -82,117 +91,141 @@ function setDifference(a,b) { /* the actual rule body */ +function theRuleBody(context,node) { + // we get the locale/translations via the options; it's the data + // that goes into a specific language's JSON file, see + // `scripts/build-assets.mjs` + const locale = context.options[0]; + + // sometimes we get MemberExpression nodes that have a + // *descendent* with the right identifier: skip them, we'll get + // the right ones as well + if (node.object?.name != 'i18n') { + return; + } + + // `method` is going to be `'ts'` or `'tsx'`, `path` is going to + // be the various translation steps/names + const [ method, ...path ] = collectMembers(node); + const pathStr = `i18n.${method}.${path.join('.')}`; + + // does that path point to a real translation? + const translation = walkDown(locale, path); + if (!translation) { + context.report({ + node, + message: `translation missing for ${pathStr}`, + }); + return; + } + + // we hit something weird, assume the programmers know what + // they're doing (this is usually some complicated slicing of + // the translation structure) + if (typeof(translation) !== 'string') return; + + const callExpression = findCallExpression(node); + const vueExpression = findVueExpression(node); + + // some more checks on how the translation is called + if (method === 'ts') { + // the ` component gets parametric translations via + // `i18n.ts.*`, but we error out elsewhere + if (translation.match(/\{/) && !vueExpression) { + context.report({ + node, + message: `translation for ${pathStr} is parametric, but called via 'ts'`, + }); + return; + } + + if (callExpression) { + context.report({ + node, + message: `translation for ${pathStr} is not parametric, but is called as a function`, + }); + } + } + + if (method === 'tsx') { + if (!translation.match(/\{/)) { + context.report({ + node, + message: `translation for ${pathStr} is not parametric, but called via 'tsx'`, + }); + return; + } + + if (!callExpression && !vueExpression) { + context.report({ + node, + message: `translation for ${pathStr} is parametric, but not called as a function`, + }); + return; + } + + // we're not currently checking arguments when used via the + // `` component, because it's too complicated (also, it + // would have to be done inside the `if (method === 'ts')`) + if (!callExpression) return; + + 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`, + }); + } + + // 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(' ')}`, + }); + } + } +} + function theRule(context) { // we get the locale/translations via the options; it's the data // that goes into a specific language's JSON file, see // `scripts/build-assets.mjs` const locale = context.options[0]; - return { - // for all object member access that have an identifier 'i18n'... - 'MemberExpression:has(> Identifier[name=i18n])': (node) => { - // sometimes we get MemberExpression nodes that have a - // *descendent* with the right identifier: skip them, we'll get - // the right ones as well - if (node.object?.name != 'i18n') { - return; - } - // `method` is going to be `'ts'` or `'tsx'`, `path` is going to - // be the various translation steps/names - const [ method, ...path ] = collectMembers(node); - const pathStr = `i18n.${method}.${path.join('.')}`; - - // does that path point to a real translation? - const translation = walkDown(locale, path); - if (!translation) { - context.report({ - node, - message: `translation missing for ${pathStr}`, - }); - return; - } - - // we hit something weird, assume the programmers know what - // they're doing (this is usually some complicated slicing of - // the translation structure) - if (typeof(translation) !== 'string') return; - - // some more checks on how the translation is called - if (method == 'ts') { - if (translation.match(/\{/)) { - context.report({ - node, - message: `translation for ${pathStr} is parametric, but called via 'ts'`, - }); - return; - } - - if (findCallExpression(node)) { - context.report({ - node, - message: `translation for ${pathStr} is not parametric, but is called as a function`, - }); - } - } - - if (method == 'tsx') { - if (!translation.match(/\{/)) { - context.report({ - node, - message: `translation for ${pathStr} is not parametric, but called via 'tsx'`, - }); - return; - } - - const callExpression = findCallExpression(node); - if (!callExpression) { - context.report({ - node, - message: `translation for ${pathStr} is parametric, but not called as a function`, - }); - return; - } - - 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`, - }); - } - - // 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(' ')}`, - }); - } - } + // for all object member access that have an identifier 'i18n'... + return context.getSourceCode().parserServices.defineTemplateBodyVisitor( + { + // this is for