From 9d8cee62b74dbd5f8152650e4c8b918e5178767f Mon Sep 17 00:00:00 2001 From: Nevo Golan <20494189+Nevoss@users.noreply.github.com> Date: Fri, 25 Apr 2025 01:00:12 +0300 Subject: [PATCH 1/5] feat: add `require-visibility` rule --- package-lock.json | 4 +- src/index.ts | 2 + .../__tests__/require-visibility.test.ts | 115 +++++++++++++++++ src/rules/require-visibility.ts | 117 ++++++++++++++++++ 4 files changed, 236 insertions(+), 2 deletions(-) create mode 100644 src/rules/__tests__/require-visibility.test.ts create mode 100644 src/rules/require-visibility.ts diff --git a/package-lock.json b/package-lock.json index 4822918..635f347 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "eslint-plugin-php", - "version": "0.0.0", + "version": "0.0.2", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "eslint-plugin-php", - "version": "0.0.0", + "version": "0.0.2", "license": "MIT", "dependencies": { "@eslint/core": "^0.13.0", diff --git a/src/index.ts b/src/index.ts index 027e048..ac570d9 100644 --- a/src/index.ts +++ b/src/index.ts @@ -3,6 +3,7 @@ import { PHPLanguage } from './language/php-language'; import { eqeqeq } from './rules/eqeqeq'; import { noArrayKeyword } from './rules/no-array-keyword'; +import { requireVisibility } from './rules/require-visibility'; const plugin = { meta: { @@ -15,6 +16,7 @@ const plugin = { rules: { eqeqeq, 'no-array-keyword': noArrayKeyword, + 'require-visibility': requireVisibility, }, } satisfies ESLint.Plugin; diff --git a/src/rules/__tests__/require-visibility.test.ts b/src/rules/__tests__/require-visibility.test.ts new file mode 100644 index 0000000..c1c9d0c --- /dev/null +++ b/src/rules/__tests__/require-visibility.test.ts @@ -0,0 +1,115 @@ +import { RuleTester, type Rule } from 'eslint'; +import php from '../../index'; +import { requireVisibility } from '../require-visibility'; + +const ruleTester = new RuleTester({ + plugins: { + php, + }, + language: 'php/php', +}); + +// TODO: Fix the types. +ruleTester.run( + 'require-visibility', + requireVisibility as unknown as Rule.RuleModule, + { + valid: [ + '({ + meta: { + type: 'layout', + fixable: 'code', + hasSuggestions: true, + docs: { + description: 'Require visibility to be declared on methods', + }, + messages: { + requireVisibilityForMethod: + 'Visibility must be declared on method "{{name}}"', + requireVisibilityForClassConstant: + 'Visibility must be declared on class constants "{{name}}"', + requireVisibilityForProperty: + 'Visibility must be declared on properties "{{name}}"', + addVisibility: 'Add "{{visibility}}" visibility', + }, + schema: [], + }, + + create(context) { + return { + 'method[visibility=""]'(_node) { + const node = _node as Method; + + context.report({ + node, + messageId: 'requireVisibilityForMethod', + data: { + name: + typeof node.name === 'string' + ? node.name + : node.name.name, + }, + suggest: visibilityOptions.map((visibility) => ({ + messageId: 'addVisibility', + data: { visibility }, + fix(fixer) { + const nodeText = context.sourceCode.getText(node); + + return fixer.replaceText( + node, + nodeText.replace( + /^function /, + `${visibility} function `, + ), + ); + }, + })), + }); + }, + 'classconstant[visibility=""]'(_node) { + const node = _node as ClassConstant; + + context.report({ + node, + messageId: 'requireVisibilityForClassConstant', + data: { + name: node.constants.map(({ name }) => name).join(', '), + }, + suggest: visibilityOptions.map((visibility) => ({ + messageId: 'addVisibility', + data: { visibility }, + fix(fixer) { + const nodeText = context.sourceCode.getText(node); + + return fixer.replaceText( + node, + nodeText.replace( + /const /, + `${visibility} const `, + ), + ); + }, + })), + }); + }, + 'propertystatement[visibility=null]'(_node) { + const node = _node as PropertyStatement; + + context.report({ + node, + messageId: 'requireVisibilityForProperty', + data: { + name: node.properties + .map(({ name }) => name) + .join(', '), + }, + suggest: visibilityOptions.map((visibility) => ({ + messageId: 'addVisibility', + data: { visibility }, + fix(fixer) { + const nodeText = context.sourceCode.getText(node); + + return fixer.replaceText( + node, + nodeText.replace(/var /, `${visibility} `), + ); + }, + })), + }); + }, + }; + }, +}); From e1cf77b2e4a87830595cd1a38dae9b57421c266e Mon Sep 17 00:00:00 2001 From: Nevo Golan <20494189+Nevoss@users.noreply.github.com> Date: Fri, 25 Apr 2025 01:07:10 +0300 Subject: [PATCH 2/5] wip --- README.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 67c9933..4b2a072 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,8 @@ export default defineConfig([ ## Available Rules -| Rule ID | Description | Fixable? | -| ---------------------- | ------------------------------------- | -------- | -| `php/eqeqeq` | Require the use of `===` and `!==` | ❌ | -| `php/no-array-keyword` | Disallow the use of the array keyword | ✅ | +| Rule ID | Description | Fixable? | Suggestion? | +|--------------------------|------------------------------------------------------|----------|-------------| +| `php/eqeqeq` | Require the use of `===` and `!==` | ❌ | ❌ | +| `php/no-array-keyword` | Disallow the use of the array keyword | ✅ | ❌ | +| `php/require-visibility` | Require visibility for class methods and properties | ❌ | ✅ | From 0fd969d3d96b8dbe6196a5637d84302f4382c57d Mon Sep 17 00:00:00 2001 From: Nevo Golan <20494189+Nevoss@users.noreply.github.com> Date: Fri, 25 Apr 2025 01:07:56 +0300 Subject: [PATCH 3/5] wip --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 4b2a072..10cff59 100644 --- a/README.md +++ b/README.md @@ -42,6 +42,6 @@ export default defineConfig([ | Rule ID | Description | Fixable? | Suggestion? | |--------------------------|------------------------------------------------------|----------|-------------| -| `php/eqeqeq` | Require the use of `===` and `!==` | ❌ | ❌ | -| `php/no-array-keyword` | Disallow the use of the array keyword | ✅ | ❌ | -| `php/require-visibility` | Require visibility for class methods and properties | ❌ | ✅ | +| `php/eqeqeq` | Require the use of `===` and `!==` | ❌ | ❌ | +| `php/no-array-keyword` | Disallow the use of the array keyword | ✅ | ❌ | +| `php/require-visibility` | Require visibility for class methods and properties | ❌ | ✅ | From 71700d624842135c7c352e51c05c5c03c8cf4269 Mon Sep 17 00:00:00 2001 From: StyleShit <32631382+StyleShit@users.noreply.github.com> Date: Sun, 27 Apr 2025 14:35:46 +0300 Subject: [PATCH 4/5] wip --- @types/php-parser.d.ts | 16 +++ README.md | 14 ++- .../__tests__/require-visibility.test.ts | 119 ++++++++++++------ src/rules/require-visibility.ts | 87 ++++++++----- src/utils/extract-names.ts | 11 ++ tsconfig.json | 3 +- 6 files changed, 176 insertions(+), 74 deletions(-) create mode 100644 @types/php-parser.d.ts create mode 100644 src/utils/extract-names.ts diff --git a/@types/php-parser.d.ts b/@types/php-parser.d.ts new file mode 100644 index 0000000..e3fe8f9 --- /dev/null +++ b/@types/php-parser.d.ts @@ -0,0 +1,16 @@ +declare module 'php-parser' { + // There is a typing issue in `php-parser` that `name` is typed as `string`. + class Constant extends Node { + name: string | Identifier; + value: Node | string | number | boolean | null; + } + + class Property extends Statement { + name: string | Identifier; + value: Node | null; + readonly: boolean; + nullable: boolean; + type: Identifier | Identifier[] | null; + attrGroups: AttrGroup[]; + } +} diff --git a/README.md b/README.md index 10cff59..09f54f7 100644 --- a/README.md +++ b/README.md @@ -40,8 +40,12 @@ export default defineConfig([ ## Available Rules -| Rule ID | Description | Fixable? | Suggestion? | -|--------------------------|------------------------------------------------------|----------|-------------| -| `php/eqeqeq` | Require the use of `===` and `!==` | ❌ | ❌ | -| `php/no-array-keyword` | Disallow the use of the array keyword | ✅ | ❌ | -| `php/require-visibility` | Require visibility for class methods and properties | ❌ | ✅ | +🔧 - Automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/use/command-line-interface#--fix). + +💡 - Manually fixable by [editor suggestions](https://eslint.org/docs/latest/use/core-concepts#rule-suggestions). + +| Rule ID | Description | 🔧 | 💡 | +| ------------------------ | --------------------------------------------------- | --- | --- | +| `php/eqeqeq` | Require the use of `===` and `!==` | | | +| `php/no-array-keyword` | Disallow the use of the array keyword | 🔧 | | +| `php/require-visibility` | Require visibility for class methods and properties | | 💡 | diff --git a/src/rules/__tests__/require-visibility.test.ts b/src/rules/__tests__/require-visibility.test.ts index c1c9d0c..d7ba097 100644 --- a/src/rules/__tests__/require-visibility.test.ts +++ b/src/rules/__tests__/require-visibility.test.ts @@ -1,6 +1,6 @@ import { RuleTester, type Rule } from 'eslint'; import php from '../../index'; -import { requireVisibility } from '../require-visibility'; +import { requireVisibility, visibilityOptions } from '../require-visibility'; const ruleTester = new RuleTester({ plugins: { @@ -19,15 +19,20 @@ ruleTester.run( ' ({ + messageId: 'addVisibility', + data: { visibility }, + output: ` ({ + messageId: 'addVisibility', + data: { visibility }, + output: ` ({ + messageId: 'addVisibility', + data: { visibility }, + output: ` ({ + messageId: 'addVisibility', + data: { visibility }, + output: `({ meta: { @@ -16,16 +20,25 @@ export const requireVisibility = createRule({ fixable: 'code', hasSuggestions: true, docs: { - description: 'Require visibility to be declared on methods', + description: 'Require visibility for class methods and properties', }, messages: { requireVisibilityForMethod: - 'Visibility must be declared on method "{{name}}"', + "Visibility must be declared on method '{{name}}'.", + requireVisibilityForClassConstant: - 'Visibility must be declared on class constants "{{name}}"', + "Visibility must be declared on class constant '{{name}}'.", + + requireVisibilityForClassConstants: + "Visibility must be declared on class constants '{{name}}'.", + requireVisibilityForProperty: - 'Visibility must be declared on properties "{{name}}"', - addVisibility: 'Add "{{visibility}}" visibility', + "Visibility must be declared on property '{{name}}'.", + + requireVisibilityForProperties: + "Visibility must be declared on properties '{{name}}'.", + + addVisibility: "Add '{{visibility}}' visibility.", }, schema: [], }, @@ -52,61 +65,79 @@ export const requireVisibility = createRule({ return fixer.replaceText( node, - nodeText.replace( - /^function /, - `${visibility} function `, - ), + `${visibility} ${nodeText}`, ); }, })), }); }, + 'classconstant[visibility=""]'(_node) { const node = _node as ClassConstant; + const constKeywordLoc = context.sourceCode.findClosestKeyword( + node, + 'const', + ); + + if (!constKeywordLoc) { + return; + } + + const constantsNames = extractNames(node.constants); + context.report({ node, - messageId: 'requireVisibilityForClassConstant', + loc: { + start: constKeywordLoc.start, + end: context.sourceCode.getLoc(node).end, + }, + messageId: + constantsNames.length === 1 + ? 'requireVisibilityForClassConstant' + : 'requireVisibilityForClassConstants', data: { - name: node.constants.map(({ name }) => name).join(', '), + name: constantsNames.join(', '), }, suggest: visibilityOptions.map((visibility) => ({ messageId: 'addVisibility', data: { visibility }, fix(fixer) { - const nodeText = context.sourceCode.getText(node); - - return fixer.replaceText( - node, - nodeText.replace( - /const /, - `${visibility} const `, - ), + return fixer.insertTextBeforeRange( + [ + constKeywordLoc.start.offset, + constKeywordLoc.end.offset, + ], + `${visibility} `, ); }, })), }); }, - 'propertystatement[visibility=null]'(_node) { + + 'propertystatement[visibility=""]'(_node) { const node = _node as PropertyStatement; + const propertiesNames = extractNames(node.properties); + context.report({ node, - messageId: 'requireVisibilityForProperty', + messageId: + propertiesNames.length === 1 + ? 'requireVisibilityForProperty' + : 'requireVisibilityForProperties', data: { - name: node.properties - .map(({ name }) => name) - .join(', '), + name: propertiesNames.join(', '), }, suggest: visibilityOptions.map((visibility) => ({ messageId: 'addVisibility', data: { visibility }, - fix(fixer) { + fix: (fixer) => { const nodeText = context.sourceCode.getText(node); return fixer.replaceText( node, - nodeText.replace(/var /, `${visibility} `), + `${visibility} ${nodeText}`, ); }, })), diff --git a/src/utils/extract-names.ts b/src/utils/extract-names.ts new file mode 100644 index 0000000..184fa8a --- /dev/null +++ b/src/utils/extract-names.ts @@ -0,0 +1,11 @@ +import { Identifier } from 'php-parser'; + +type Nameable = { + name: string | Identifier; +}; + +export function extractNames(names: Nameable[]) { + return names.map(({ name }) => + typeof name === 'string' ? name : name.name, + ); +} diff --git a/tsconfig.json b/tsconfig.json index 8390ab5..a399416 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -14,6 +14,7 @@ "module": "preserve", "noEmit": true, - "lib": ["ESNext", "dom", "dom.iterable"] + "lib": ["ESNext", "dom", "dom.iterable"], + "typeRoots": ["./node_modules/@types", "./@types"] } } From e665c61323019130ebf8d5c854e75e91321d1779 Mon Sep 17 00:00:00 2001 From: StyleShit <32631382+StyleShit@users.noreply.github.com> Date: Sun, 27 Apr 2025 14:36:36 +0300 Subject: [PATCH 5/5] wip --- .changeset/two-adults-sleep.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/two-adults-sleep.md diff --git a/.changeset/two-adults-sleep.md b/.changeset/two-adults-sleep.md new file mode 100644 index 0000000..a56e7f5 --- /dev/null +++ b/.changeset/two-adults-sleep.md @@ -0,0 +1,5 @@ +--- +'eslint-plugin-php': minor +--- + +Add `require-visibility` rule