-
Notifications
You must be signed in to change notification settings - Fork 25
feat: discourage monkey patching #159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
saberzero1
wants to merge
7
commits into
obsidianmd:master
Choose a base branch
from
saberzero1:monkey-patching
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
2fb02b7
feat: discourage monkey patching
saberzero1 f9ccc0f
test: monkey patching cases
saberzero1 d24380b
docs: regenerated documentation
saberzero1 1d947fe
refactor: move monkey-patching package checks to banned dependencies
saberzero1 b78dbce
test: removed monkey-patching tests
saberzero1 2666ea4
docs: regenerated documentation
saberzero1 254991f
chore: addressed review feedback
saberzero1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # obsidianmd/no-monkey-patching | ||
|
|
||
| 📝 Discourage directly modifying prototypes. | ||
|
|
||
| 💼 This rule is enabled in the following configs: ✅ `recommended`, 🇬🇧 `recommendedWithLocalesEn`. | ||
|
|
||
| <!-- end auto-generated rule header --> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,175 @@ | ||
| import { TSESTree, ESLintUtils } from "@typescript-eslint/utils"; | ||
|
|
||
| const ruleCreator = ESLintUtils.RuleCreator( | ||
| (name) => | ||
| `https://github.com/obsidianmd/eslint-plugin/blob/master/docs/rules/${name}.md`, | ||
| ); | ||
|
|
||
| function isPrototypeAccess(node: TSESTree.Node): node is TSESTree.MemberExpression { | ||
| if (node.type !== TSESTree.AST_NODE_TYPES.MemberExpression) { | ||
| return false; | ||
| } | ||
| if ( | ||
| node.property.type === TSESTree.AST_NODE_TYPES.Identifier && | ||
| node.property.name === "prototype" | ||
| ) { | ||
| return true; | ||
| } | ||
| if ( | ||
| node.computed && | ||
| node.property.type === TSESTree.AST_NODE_TYPES.Literal && | ||
| node.property.value === "prototype" | ||
| ) { | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| function isPrototypeMemberAccess(node: TSESTree.MemberExpression): boolean { | ||
| return isPrototypeAccess(node.object); | ||
| } | ||
|
|
||
| function getMemberName(node: TSESTree.MemberExpression): string { | ||
| const parts: string[] = []; | ||
| let current: TSESTree.Expression = node; | ||
| while (current.type === TSESTree.AST_NODE_TYPES.MemberExpression) { | ||
| const prop = current.property; | ||
| if (prop.type === TSESTree.AST_NODE_TYPES.Identifier) { | ||
| parts.unshift(prop.name); | ||
| } else if (prop.type === TSESTree.AST_NODE_TYPES.Literal) { | ||
| parts.unshift(String(prop.value)); | ||
| } else { | ||
| parts.unshift("[computed]"); | ||
| } | ||
| current = current.object; | ||
| } | ||
| if (current.type === TSESTree.AST_NODE_TYPES.Identifier) { | ||
| parts.unshift(current.name); | ||
| } | ||
| return parts.join("."); | ||
| } | ||
|
|
||
| const PROTO_METHODS = { | ||
| defineProperty: "definePropertyOnPrototype", | ||
| defineProperties: "definePropertyOnPrototype", | ||
| assign: "assignToPrototype", | ||
| set: "assignToPrototype", | ||
| } as const; | ||
|
|
||
| function isGetPrototypeOfCall(node: TSESTree.Expression): boolean { | ||
| return ( | ||
| node.type === TSESTree.AST_NODE_TYPES.CallExpression && | ||
| node.callee.type === TSESTree.AST_NODE_TYPES.MemberExpression && | ||
| node.callee.object.type === TSESTree.AST_NODE_TYPES.Identifier && | ||
| (node.callee.object.name === "Object" || node.callee.object.name === "Reflect") && | ||
| node.callee.property.type === TSESTree.AST_NODE_TYPES.Identifier && | ||
| node.callee.property.name === "getPrototypeOf" | ||
| ); | ||
| } | ||
|
|
||
| export default ruleCreator({ | ||
| name: "no-monkey-patching", | ||
| meta: { | ||
| type: "problem" as const, | ||
| docs: { | ||
| description: | ||
| "Discourage directly modifying prototypes.", | ||
| }, | ||
| schema: [], | ||
| messages: { | ||
| directPrototypeAssignment: | ||
| "Do not assign to `{{name}}`. Directly modifying prototypes is unsafe and discouraged.", | ||
| definePropertyOnPrototype: | ||
| "Do not use `Object.defineProperty` on `{{name}}`. Directly modifying prototypes is unsafe and discouraged.", | ||
| assignToPrototype: | ||
| "Do not use `Object.assign` on `{{name}}`. Directly modifying prototypes is unsafe and discouraged.", | ||
| setPrototypeOf: | ||
| "Do not use `Object.setPrototypeOf` on a prototype. Directly modifying prototypes is unsafe and discouraged.", | ||
| deletePrototypeMember: | ||
| "Do not delete `{{name}}`. Directly modifying prototypes is unsafe and discouraged.", | ||
| getPrototypeOfAssignment: | ||
| "Do not assign to a member of `Object.getPrototypeOf(...)`. Directly modifying prototypes is unsafe and discouraged.", | ||
| }, | ||
| }, | ||
| defaultOptions: [], | ||
| create(context) { | ||
| return { | ||
| CallExpression(node: TSESTree.CallExpression) { | ||
| if ( | ||
| node.callee.type !== TSESTree.AST_NODE_TYPES.MemberExpression || | ||
| node.callee.object.type !== TSESTree.AST_NODE_TYPES.Identifier || | ||
| node.callee.property.type !== TSESTree.AST_NODE_TYPES.Identifier | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| const objectName = node.callee.object.name; | ||
| const methodName = node.callee.property.name; | ||
|
|
||
| if ( | ||
| (objectName === "Object" || objectName === "Reflect") && | ||
| methodName === "setPrototypeOf" && | ||
| node.arguments.length >= 1 && | ||
| isPrototypeAccess(node.arguments[0]) | ||
| ) { | ||
| context.report({ | ||
| node, | ||
| messageId: "setPrototypeOf", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if ( | ||
| (objectName === "Object" || objectName === "Reflect") && | ||
| methodName in PROTO_METHODS && | ||
| node.arguments.length >= 1 && | ||
| isPrototypeAccess(node.arguments[0]) | ||
| ) { | ||
| const target = node.arguments[0] as TSESTree.MemberExpression; | ||
| context.report({ | ||
| node, | ||
| messageId: PROTO_METHODS[methodName as keyof typeof PROTO_METHODS], | ||
| data: { name: getMemberName(target) }, | ||
| }); | ||
| return; | ||
| } | ||
| }, | ||
|
|
||
| AssignmentExpression(node: TSESTree.AssignmentExpression) { | ||
| if (node.left.type !== TSESTree.AST_NODE_TYPES.MemberExpression) { | ||
| return; | ||
| } | ||
|
|
||
| if (isGetPrototypeOfCall(node.left.object)) { | ||
| context.report({ | ||
| node, | ||
| messageId: "getPrototypeOfAssignment", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (isPrototypeMemberAccess(node.left) || isPrototypeAccess(node.left)) { | ||
| context.report({ | ||
| node, | ||
| messageId: "directPrototypeAssignment", | ||
| data: { name: getMemberName(node.left) }, | ||
| }); | ||
| } | ||
| }, | ||
|
|
||
| UnaryExpression(node: TSESTree.UnaryExpression) { | ||
| if ( | ||
| node.operator === "delete" && | ||
| node.argument.type === TSESTree.AST_NODE_TYPES.MemberExpression && | ||
| isPrototypeMemberAccess(node.argument) | ||
| ) { | ||
| context.report({ | ||
| node, | ||
| messageId: "deletePrototypeMember", | ||
| data: { name: getMemberName(node.argument) }, | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| import { RuleTester } from "@typescript-eslint/rule-tester"; | ||
| import noMonkeyPatchingRule from "../lib/rules/noMonkeyPatching.js"; | ||
|
|
||
| const ruleTester = new RuleTester(); | ||
|
|
||
| ruleTester.run("no-monkey-patching", noMonkeyPatchingRule, { | ||
| valid: [ | ||
| { | ||
| name: "normal import is allowed", | ||
| code: "import { Plugin } from 'obsidian';", | ||
| }, | ||
| { | ||
| name: "normal require is allowed", | ||
| code: "const obsidian = require('obsidian');", | ||
| }, | ||
| { | ||
| name: "assigning to own class prototype is allowed via normal property", | ||
| code: "const obj = {}; obj.foo = 'bar';", | ||
| }, | ||
| { | ||
| name: "Object.assign with plain objects is allowed", | ||
| code: "Object.assign(target, source);", | ||
| }, | ||
| { | ||
| name: "Object.defineProperty on plain object is allowed", | ||
| code: "Object.defineProperty(obj, 'key', { value: 42 });", | ||
| }, | ||
| { | ||
| name: "accessing prototype without assignment is allowed", | ||
| code: "const proto = Array.prototype;", | ||
| }, | ||
| { | ||
| name: "Object.getPrototypeOf without assignment is allowed", | ||
| code: "const x = Object.getPrototypeOf(obj);", | ||
| }, | ||
| { | ||
| name: "Object.create with prototype is allowed", | ||
| code: "const child = Object.create(Parent.prototype);", | ||
| }, | ||
| { | ||
| name: "Reflect.defineProperty on plain object is allowed", | ||
| code: "Reflect.defineProperty(obj, 'key', { value: 42 });", | ||
| }, | ||
| { | ||
| name: "Reflect.set on plain object is allowed", | ||
| code: "Reflect.set(target, 'key', value);", | ||
| }, | ||
| { | ||
| name: "delete on non-prototype member is allowed", | ||
| code: "delete obj.foo;", | ||
| }, | ||
| { | ||
| name: "Object.setPrototypeOf on non-prototype target is allowed", | ||
| code: "Object.setPrototypeOf(obj, proto);", | ||
| }, | ||
| ], | ||
| invalid: [ | ||
| { | ||
| name: "direct prototype method assignment is forbidden", | ||
| code: "Workspace.prototype.getActiveViewOfType = function() { return null; };", | ||
| errors: [{ messageId: "directPrototypeAssignment", data: { name: "Workspace.prototype.getActiveViewOfType" } }], | ||
| }, | ||
| { | ||
| name: "Array.prototype assignment is forbidden", | ||
| code: "Array.prototype.customMethod = function() {};", | ||
| errors: [{ messageId: "directPrototypeAssignment", data: { name: "Array.prototype.customMethod" } }], | ||
| }, | ||
| { | ||
| name: "Object.prototype assignment is forbidden", | ||
| code: "Object.prototype.foo = 'bar';", | ||
| errors: [{ messageId: "directPrototypeAssignment", data: { name: "Object.prototype.foo" } }], | ||
| }, | ||
| { | ||
| name: "assigning to .prototype itself is forbidden", | ||
| code: "MyClass.prototype = {};", | ||
| errors: [{ messageId: "directPrototypeAssignment", data: { name: "MyClass.prototype" } }], | ||
| }, | ||
| { | ||
| name: "Object.defineProperty on prototype is forbidden", | ||
| code: "Object.defineProperty(Workspace.prototype, 'method', { value: function() {} });", | ||
| errors: [{ messageId: "definePropertyOnPrototype", data: { name: "Workspace.prototype" } }], | ||
| }, | ||
| { | ||
| name: "Object.defineProperties on prototype is forbidden", | ||
| code: "Object.defineProperties(Array.prototype, { custom: { value: 1 } });", | ||
| errors: [{ messageId: "definePropertyOnPrototype", data: { name: "Array.prototype" } }], | ||
| }, | ||
| { | ||
| name: "Object.assign on prototype is forbidden", | ||
| code: "Object.assign(Element.prototype, { customMethod() {} });", | ||
| errors: [{ messageId: "assignToPrototype", data: { name: "Element.prototype" } }], | ||
| }, | ||
| { | ||
| name: "Reflect.defineProperty on prototype is forbidden", | ||
| code: "Reflect.defineProperty(Workspace.prototype, 'method', { value: function() {} });", | ||
| errors: [{ messageId: "definePropertyOnPrototype", data: { name: "Workspace.prototype" } }], | ||
| }, | ||
| { | ||
| name: "Reflect.set on prototype is forbidden", | ||
| code: "Reflect.set(Array.prototype, 'customMethod', function() {});", | ||
| errors: [{ messageId: "assignToPrototype", data: { name: "Array.prototype" } }], | ||
| }, | ||
| { | ||
| name: "computed prototype access via bracket notation is forbidden", | ||
| code: "Workspace['prototype'].getLeaf = function() {};", | ||
| errors: [{ messageId: "directPrototypeAssignment", data: { name: "Workspace.prototype.getLeaf" } }], | ||
| }, | ||
| { | ||
| name: "Object.defineProperty with computed prototype access is forbidden", | ||
| code: "Object.defineProperty(Workspace['prototype'], 'method', { value: function() {} });", | ||
| errors: [{ messageId: "definePropertyOnPrototype", data: { name: "Workspace.prototype" } }], | ||
| }, | ||
| { | ||
| name: "Object.getPrototypeOf assignment is forbidden", | ||
| code: "Object.getPrototypeOf(workspace).getLeaf = function() {};", | ||
| errors: [{ messageId: "getPrototypeOfAssignment" }], | ||
| }, | ||
| { | ||
| name: "Reflect.getPrototypeOf assignment is forbidden", | ||
| code: "Reflect.getPrototypeOf(workspace).getLeaf = function() {};", | ||
| errors: [{ messageId: "getPrototypeOfAssignment" }], | ||
| }, | ||
| { | ||
| name: "Object.setPrototypeOf on prototype is forbidden", | ||
| code: "Object.setPrototypeOf(MyClass.prototype, OtherClass.prototype);", | ||
| errors: [{ messageId: "setPrototypeOf" }], | ||
| }, | ||
| { | ||
| name: "Reflect.setPrototypeOf on prototype is forbidden", | ||
| code: "Reflect.setPrototypeOf(MyClass.prototype, OtherClass.prototype);", | ||
| errors: [{ messageId: "setPrototypeOf" }], | ||
| }, | ||
| { | ||
| name: "delete on prototype member is forbidden", | ||
| code: "delete Workspace.prototype.getLeaf;", | ||
| errors: [{ messageId: "deletePrototypeMember", data: { name: "Workspace.prototype.getLeaf" } }], | ||
| }, | ||
| ], | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These three expressions are nearly identical. Can we combine them? Claude has this suggestion:
They collapse cleanly into one:
then a single guard checking
node.callee.property.name in PROTO_METHODS. Cuts ~50 lines and removes the inconsistent arguments.length >= 2 vs >= 1 checks (those bounds are loose anyway and don't affect correctness).