feat: migrate a new rule 'use-read-only-spread' (#472)

This commit adds a new rule 'use-read-only-spread' originally developed and used here: https://github.com/adeira/universe/blob/91ba39d4f59ac0376121b67e443f474a6ade4feb/src/eslint-plugin-adeira/src/rules/flow-use-readonly-spread.js It prevents users from accidentally creating an object which is no longer read-only because of Flow spread operator.
gajus · Apr 6, 2021 · 998eb5a · 998eb5a
1 parent 6d5362b
commit 998eb5a
Show file tree

Hide file tree

Showing 7 changed files with 263 additions and 0 deletions.
diff --git a/.README/README.md b/.README/README.md
@@ -194,4 +194,5 @@ When `true`, only checks files with a [`@flow` annotation](http://flowtype.org/d
 {"gitdown": "include", "file": "./rules/type-import-style.md"}
 {"gitdown": "include", "file": "./rules/union-intersection-spacing.md"}
 {"gitdown": "include", "file": "./rules/use-flow-type.md"}
+{"gitdown": "include", "file": "./rules/use-read-only-spread.md"}
 {"gitdown": "include", "file": "./rules/valid-syntax.md"}
diff --git a/.README/rules/use-read-only-spread.md b/.README/rules/use-read-only-spread.md
@@ -0,0 +1,39 @@
+### `use-read-only-spread`
+
+Warns against accidentally creating an object which is no longer read-only because of how spread operator works in Flow. Imagine the following code:
+
+```flow js
+type INode = {|
+  +type: string,
+|};
+
+type Identifier = {|
+  ...INode,
+  +name: string,
+|};
+```
+
+You might expect the identifier name to be read-only, however, that's not true ([flow.org/try](https://flow.org/try/#0C4TwDgpgBAkgcgewCbQLxQN4B8BQUoDUokAXFAM7ABOAlgHYDmANDlgL4DcOOx0MKdYDQBmNCFSjpseKADp58ZBBb4CdAIYBbCGUq1GLdlxwBjBHUpQAHmX4RBIsRKlQN2sgHIPTKL08eoTm4rWV5JKA8AZQALBABXABskVwRgKAAjaAB3WmB1dISIAEIPLhC3NAiY+KSUtMyoHJo8guLSnCA)):
+
+```flow js
+const x: Identifier = { name: '', type: '' };
+
+x.type = 'Should not be writable!'; // No Flow error
+x.name = 'Should not be writable!'; // No Flow error
+```
+
+This rule suggests to use `$ReadOnly<…>` to prevent accidental loss of readonly-ness:
+
+```flow js
+type Identifier = $ReadOnly<{|
+  ...INode,
+  +name: string,
+|}>;
+
+const x: Identifier = { name: '', type: '' };
+
+x.type = 'Should not be writable!'; // $FlowExpectedError[cannot-write]
+x.name = 'Should not be writable!'; // $FlowExpectedError[cannot-write]
+```
+
+<!-- assertions useReadOnlySpread -->
diff --git a/README.md b/README.md
@@ -56,6 +56,7 @@
         * [`type-import-style`](#eslint-plugin-flowtype-rules-type-import-style)
         * [`union-intersection-spacing`](#eslint-plugin-flowtype-rules-union-intersection-spacing)
         * [`use-flow-type`](#eslint-plugin-flowtype-rules-use-flow-type)
+        * [`use-read-only-spread`](#eslint-plugin-flowtype-rules-use-read-only-spread)
         * [`valid-syntax`](#eslint-plugin-flowtype-rules-valid-syntax)
 
 
@@ -6430,6 +6431,105 @@ import type A from "a"; type X<B = A<string>> = { b: B }; let x: X; console.log(
 
 
 
+<a name="eslint-plugin-flowtype-rules-use-read-only-spread"></a>
+### <code>use-read-only-spread</code>
+
+Warns against accidentally creating an object which is no longer read-only because of how spread operator works in Flow. Imagine the following code:
+
+```flow js
+type INode = {|
+  +type: string,
+|};
+
+type Identifier = {|
+  ...INode,
+  +name: string,
+|};
+```
+
+You might expect the identifier name to be read-only, however, that's not true ([flow.org/try](https://flow.org/try/#0C4TwDgpgBAkgcgewCbQLxQN4B8BQUoDUokAXFAM7ABOAlgHYDmANDlgL4DcOOx0MKdYDQBmNCFSjpseKADp58ZBBb4CdAIYBbCGUq1GLdlxwBjBHUpQAHmX4RBIsRKlQN2sgHIPTKL08eoTm4rWV5JKA8AZQALBABXABskVwRgKAAjaAB3WmB1dISIAEIPLhC3NAiY+KSUtMyoHJo8guLSnCA)):
+
+```flow js
+const x: Identifier = { name: '', type: '' };
+
+x.type = 'Should not be writable!'; // No Flow error
+x.name = 'Should not be writable!'; // No Flow error
+```
+
+This rule suggests to use `$ReadOnly<…>` to prevent accidental loss of readonly-ness:
+
+```flow js
+type Identifier = $ReadOnly<{|
+  ...INode,
+  +name: string,
+|}>;
+
+const x: Identifier = { name: '', type: '' };
+
+x.type = 'Should not be writable!'; // $FlowExpectedError[cannot-write]
+x.name = 'Should not be writable!'; // $FlowExpectedError[cannot-write]
+```
+
+The following patterns are considered problems:
+
+```js
+type INode = {||};
+type Identifier = {|
+  ...INode,
+  +aaa: string,
+|};
+// Message: Flow type with spread property and all readonly properties should be wrapped in '$ReadOnly<…>' to prevent accidental loss of readonly-ness.
+
+type INode = {||};
+type Identifier = {|
+  ...INode,
+  +aaa: string,
+  +bbb: string,
+|};
+// Message: Flow type with spread property and all readonly properties should be wrapped in '$ReadOnly<…>' to prevent accidental loss of readonly-ness.
+```
+
+The following patterns are not considered problems:
+
+```js
+type INode = {||};
+type Identifier = {|
+  ...INode,
+  name: string,
+|};
+
+type INode = {||};
+type Identifier = {|
+  ...INode,
+  name: string, // writable on purpose
+  +surname: string,
+|};
+
+type Identifier = {|
+  +name: string,
+|};
+
+type INode = {||};
+type Identifier = $ReadOnly<{|
+  ...INode,
+  +name: string,
+|}>;
+
+type INode = {||};
+type Identifier = $ReadOnly<{|
+  ...INode,
+  name: string, // writable on purpose
+|}>;
+
+type INode = {||};
+type Identifier = $ReadOnly<{|
+  ...INode,
+  -name: string,
+|}>;
+```
+
+
+
 <a name="eslint-plugin-flowtype-rules-valid-syntax"></a>
 ### <code>valid-syntax</code>
 

diff --git a/src/index.js b/src/index.js
@@ -39,6 +39,7 @@ import typeIdMatch from './rules/typeIdMatch';
 import typeImportStyle from './rules/typeImportStyle';
 import unionIntersectionSpacing from './rules/unionIntersectionSpacing';
 import useFlowType from './rules/useFlowType';
+import useReadOnlySpread from './rules/useReadOnlySpread';
 import validSyntax from './rules/validSyntax';
 import spreadExactType from './rules/spreadExactType';
 import arrowParens from './rules/arrowParens';
@@ -85,6 +86,7 @@ const rules = {
   'type-import-style': typeImportStyle,
   'union-intersection-spacing': unionIntersectionSpacing,
   'use-flow-type': useFlowType,
+  'use-read-only-spread': useReadOnlySpread,
   'valid-syntax': validSyntax,
 };
 

diff --git a/src/rules/useReadOnlySpread.js b/src/rules/useReadOnlySpread.js
@@ -0,0 +1,44 @@
+const meta = {
+  messages: {
+    readonlySpread:
+      'Flow type with spread property and all readonly properties should be ' +
+    'wrapped in \'$ReadOnly<…>\' to prevent accidental loss of readonly-ness.',
+  },
+};
+
+const create = (context) => {
+  return {
+    TypeAlias (node) {
+      if (node.right.type === 'GenericTypeAnnotation' && node.right.id.name === '$ReadOnly') {
+        // it's already $ReadOnly<…>, nothing to do
+      } else if (node.right.type === 'ObjectTypeAnnotation') {
+        // let's iterate all props and if everything is readonly then throw
+        let shouldThrow = false;
+        let hasSpread = false;
+        for (const property of node.right.properties) {
+          if (property.type === 'ObjectTypeProperty') {
+            if (property.variance && property.variance.kind === 'plus') {
+              shouldThrow = true;
+            } else {
+              shouldThrow = false;
+              break;
+            }
+          } else if (property.type === 'ObjectTypeSpreadProperty') {
+            hasSpread = true;
+          }
+        }
+        if (hasSpread === true && shouldThrow === true) {
+          context.report({
+            messageId: 'readonlySpread',
+            node: node.right,
+          });
+        }
+      }
+    },
+  };
+};
+
+export default {
+  create,
+  meta,
+};
diff --git a/tests/rules/assertions/useReadOnlySpread.js b/tests/rules/assertions/useReadOnlySpread.js
@@ -0,0 +1,76 @@
+export default {
+  invalid: [
+    {
+      code: `type INode = {||};
+type Identifier = {|
+  ...INode,
+  +aaa: string,
+|};`,
+      errors: [{
+        message: 'Flow type with spread property and all readonly properties should be wrapped in \'$ReadOnly<…>\' to prevent accidental loss of readonly-ness.',
+      }],
+    },
+    {
+      code: `type INode = {||};
+type Identifier = {|
+  ...INode,
+  +aaa: string,
+  +bbb: string,
+|};`,
+      errors: [{
+        message: 'Flow type with spread property and all readonly properties should be wrapped in \'$ReadOnly<…>\' to prevent accidental loss of readonly-ness.',
+      }],
+    },
+  ],
+
+  valid: [
+    // Object with spread operator:
+    {
+      code: `type INode = {||};
+type Identifier = {|
+  ...INode,
+  name: string,
+|};`,
+    },
+    {
+      code: `type INode = {||};
+type Identifier = {|
+  ...INode,
+  name: string, // writable on purpose
+  +surname: string,
+|};`,
+    },
+
+    // Object without spread operator:
+    {
+      code: `type Identifier = {|
+  +name: string,
+|};`,
+    },
+
+    // Read-only object with spread:
+    {
+      code: `type INode = {||};
+type Identifier = $ReadOnly<{|
+  ...INode,
+  +name: string,
+|}>;`,
+    },
+    {
+      code: `type INode = {||};
+type Identifier = $ReadOnly<{|
+  ...INode,
+  name: string, // writable on purpose
+|}>;`,
+    },
+
+    // Write-only object with spread:
+    {
+      code: `type INode = {||};
+type Identifier = $ReadOnly<{|
+  ...INode,
+  -name: string,
+|}>;`,
+    },
+  ],
+};
diff --git a/tests/rules/index.js b/tests/rules/index.js
@@ -51,6 +51,7 @@ const reportingRules = [
   'type-import-style',
   'union-intersection-spacing',
   'use-flow-type',
+  'use-read-only-spread',
   'valid-syntax',
 ];