Skip to content
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

New: no-restricted-properties rule (fixes #3218) #7017

Merged
merged 7 commits into from Sep 2, 2016
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions conf/eslint.json
Expand Up @@ -88,6 +88,7 @@
"no-restricted-globals": "off",
"no-restricted-imports": "off",
"no-restricted-modules": "off",
"no-restricted-properties": "off",
"no-restricted-syntax": "off",
"no-return-assign": "off",
"no-script-url": "off",
Expand Down
73 changes: 73 additions & 0 deletions docs/rules/no-restricted-properties.md
@@ -0,0 +1,73 @@
# Disallow certain object properties (no-restricted-properties)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Lowercase "d" to start the heading (new style)


Certain properties on objects may be disallowed in a codebase. This is useful for deprecating an API or restricting usage of a module's methods. This rule looks for accessing a given property key on a given object name, either when reading the property's value or invoking it as a function. You may specify an optional message to indicate an alternative API or a reason for the restriction.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand on this? This section is supposed to be about the concept that the rule is based on rather than about the rule itself. Explain why you'd want to restrict properties and maybe give an example ("window.location"? Something else?).

The parts about the rule should go after "Rule Details heading.


## Rule Details

This rule is aimed at disallowing certain object properties from your code.

### Options

This rule takes a list of objects, where the object name and property names are specified:

```json
{
"rules": {
"no-restricted-properties": [2, [{
"object": "disallowedObjectName",
"property": "disallowedPropertyName"
}]]
}
}
```

Multiple object/property values can be disallowed, and you can specify an optional message:

```json
{
"rules": {
"no-restricted-properties": [2, [{
"object": "disallowedObjectName",
"property": "disallowedPropertyName"
}, {
"object": "disallowedObjectName",
"property": "anotherDisallowedPropertyName",
"message": "Please use allowedObjectName.allowedPropertyName."
}]]
}
}
```

The following patterns are considered problems:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our format is now

Examples of **incorrect** code for this rule:


```js
/* eslint no-restricted-properties: [2, {
"object": "disallowedObjectName",
"property": "disallowedPropertyName"
}] */

var example = disallowedObjectName.disallowedPropertyName; /*error Disallowed object property: disallowedObjectName.disallowedPropertyName.*/

disallowedObjectName.disallowedPropertyName(); /*error Disallowed object property: disallowedObjectName.disallowedPropertyName.*/
```

The following patterns are not considered problems:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Examples of **correct** code for this rule:


```js
/* eslint no-restricted-properties: [2, {
"object": "disallowedObjectName",
"property": "disallowedPropertyName"
}] */

var example = disallowedObjectName.somePropertyName;

allowedObjectName.disallowedPropertyName();
```

## When Not To Use It

If you don't have any object/property combinations to restrict, you should not use this rule.

## Related Rules

* [no-restricted-syntax](no-restricted-syntax.md)
83 changes: 83 additions & 0 deletions lib/rules/no-restricted-properties.js
@@ -0,0 +1,83 @@
/**
* @fileoverview Rule to disallow certain object properties
* @author Will Klein & Eli White
*/

"use strict";

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

module.exports = {
meta: {
docs: {
description: "disallow certain properties on certain objects",
category: "Node.js and CommonJS",
recommended: false
},

schema: {
type: "array",
items: {
type: "object",
properties: {
object: {
type: "string"
},
property: {
type: "string"
},
message: {
type: "string"
}
},
additionalProperties: false,
required: [
"object",
"property"
]
},
uniqueItems: true
}
},

create(context) {
const restrictedCalls = context.options;

if (restrictedCalls.length === 0) {
return {};
}

const restrictedProperties = context.options.reduce(function(restrictions, option) {
const objectName = option.object;
const propertyName = option.property;

restrictions[objectName] = restrictions[objectName] || {};
restrictions[objectName][propertyName] = {
message: option.message
};

return restrictions;
}, {});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use Map objects instead of plain objects.

/*eslint no-restricted-properties: [error, {object: "obj", property: "foo"}]*/

// should not warn.
toString.toString
obj.toString
obj.valueOf
/*eslint no-restricted-properties: [error, {object: "obj", property: "__proto__"}]*/

// should not warn.
obj.toString
obj.valueOf

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, good catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been great. This is the first time I have used Map. This was a great use for it. 👍


return {
MemberExpression(node) {
const objectName = node.object && node.object.name;
const propertyName = node.property && node.property.name;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only going to catch foo.bar notation. Should we also catch foo['bar'] as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @mysticatea had recently added a "get simple property name" method
to ast-utils, might want to hunt that down.

On Sep 2, 2016 8:56 AM, "Ilya Volodin" notifications@github.com wrote:

In lib/rules/no-restricted-properties.js
#7017 (comment):

  •        if (!restrictions.has(objectName)) {
    
  •            restrictions.set(objectName, new Map());
    
  •        }
    
  •        restrictions.get(objectName).set(propertyName, {
    
  •            message: option.message
    
  •        });
    
  •        return restrictions;
    
  •    }, new Map());
    
  •    return {
    
  •        MemberExpression(node) {
    
  •            const objectName = node.object && node.object.name;
    
  •            const propertyName = node.property && node.property.name;
    

This is only going to catch foo.bar notation. Should we also catch
foo['bar'] as well?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/eslint/eslint/pull/7017/files/576e4cc27d1d2f01961be20a41fa94df19a75b19#r77349937,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARWekx4AnK_LRONX9DtdWUi0el5Plroks5qmCsngaJpZM4JwEv3
.

const matchedObject = restrictedProperties[objectName];
const matchedObjectProperty = matchedObject && matchedObject[propertyName];

if (matchedObjectProperty) {
const message = matchedObjectProperty.message ? " " + matchedObjectProperty.message : "";

context.report(node, "'{{objectName}}.{{propertyName}}' is restricted from being used.{{message}}", {
objectName,
propertyName,
message
});
}
}
};
}
};
98 changes: 98 additions & 0 deletions tests/lib/rules/no-restricted-properties.js
@@ -0,0 +1,98 @@
/**
* @fileoverview Tests for no-restricted-properties rule.
* @author Will Klein
* @copyright 2016 Will Klein. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the copyright

*/

"use strict";

//------------------------------------------------------------------------------
// Requirements
//------------------------------------------------------------------------------

const rule = require("../../../lib/rules/no-restricted-properties"),
RuleTester = require("../../../lib/testers/rule-tester");

//------------------------------------------------------------------------------
// Tests
//------------------------------------------------------------------------------

const ruleTester = new RuleTester();

ruleTester.run("no-restricted-properties", rule, {
valid: [
{
code: "someObject.someProperty",
options: [{
object: "someObject",
property: "disallowedProperty"
}]
}, {
code: "anotherObject.disallowedProperty",
options: [{
object: "someObject",
property: "disallowedProperty"
}]
}, {
code: "someObject.someProperty()",
options: [{
object: "someObject",
property: "disallowedProperty"
}]
}, {
code: "anotherObject.disallowedProperty()",
options: [{
object: "someObject",
property: "disallowedProperty"
}]
}, {
code: "anotherObject.disallowedProperty()",
options: [{
object: "someObject",
property: "disallowedProperty",
message: "Please use someObject.allowedProperty instead."
}]
}
],

invalid: [
{
code: "someObject.disallowedProperty",
options: [{
object: "someObject",
property: "disallowedProperty"
}],
errors: [{
message: "'someObject.disallowedProperty' is restricted from being used.",
type: "MemberExpression"
}]
}, {
code: "someObject.disallowedProperty",
options: [{
object: "someObject",
property: "disallowedProperty",
message: "Please use someObject.allowedProperty instead."
}],
errors: [{
message: "'someObject.disallowedProperty' is restricted from being used. Please use someObject.allowedProperty instead.",
type: "MemberExpression"
}]
}, {
code: "someObject.disallowedProperty; anotherObject.anotherDisallowedProperty()",
options: [{
object: "someObject",
property: "disallowedProperty"
}, {
object: "anotherObject",
property: "anotherDisallowedProperty"
}],
errors: [{
message: "'someObject.disallowedProperty' is restricted from being used.",
type: "MemberExpression"
}, {
message: "'anotherObject.anotherDisallowedProperty' is restricted from being used.",
type: "MemberExpression"
}]
}
]
});