Lint rule to forbid access of cross-fork fields (#19679)
* Lint rule to forbid access of cross-fork fields We use a shared Fiber type for both reconciler forks (old and new). It is a superset of all the fields used by both forks. However, there are some fields that should only be used in the new fork, and others that should only be used in the old fork. Ideally we would enforce this with separate Flow types for each fork. The problem is that the Fiber type is accessed by some packages outside the reconciler (like React DOM), and get passed into the reconciler as arguments. So there's no way to fork the Fiber type without also forking the packages where they are used. FiberRoot has the same issue. Instead, I've added a lint rule that forbids cross-fork access of fork-specific fields. Fields that end in `_old` or `_new` are forbidden from being used inside the new or old fork respectively. Or you can specific custom fields using the ESLint plugin options. I used this plugin to find and remove references to the effect list in d2e914a. * Mark effect list fields as old And `subtreeTag` as new. I didn't mark `lastEffect` because that name is also used by the Hook type. Not super important; could rename to `lastEffect_old` but idk if it's worth the effort.
Andrew Clark committed
Aug 24, 2020 at 12:07 UTC
af219cc6e6c514099a667ffab4e2d80c5c0c1bcc
4 files changed
+228
.eslintrc.js
+12
@@ -99,6 +99,18 @@ module.exports = {
99
'react-internal/warning-args': ERROR,
100
'react-internal/no-production-logging': ERROR,
101
'react-internal/no-cross-fork-imports': ERROR,
102
+ 'react-internal/no-cross-fork-types': [
103
+ ERROR,
104
+ {
105
+ old: [
106
+ 'firstEffect',
107
+ 'nextEffect',
108
+ // Disabled because it's also used by the Hook type.
109
+ // 'lastEffect',
110
+ ],
111
+ new: ['subtreeTag'],
112
+ },
113
+ ],
114
},
115
116
overrides: [
scripts/eslint-rules/__tests__/no-cross-fork-types-test.internal.js
new
+89
@@ -0,0 +1,89 @@
1
+/**
2
+ * Copyright (c) Facebook, Inc. and its affiliates.
3
+ *
4
+ * This source code is licensed under the MIT license found in the
5
+ * LICENSE file in the root directory of this source tree.
6
+ *
7
+ * @emails react-core
8
+ */
9
+
10
+'use strict';
11
+
12
+const rule = require('../no-cross-fork-types');
13
+const RuleTester = require('eslint').RuleTester;
14
+const ruleTester = new RuleTester({
15
+ parserOptions: {
16
+ ecmaVersion: 8,
17
+ sourceType: 'module',
18
+ },
19
+});
20
+
21
+const newAccessWarning =
22
+ 'Field cannot be accessed inside the old reconciler fork, only the ' +
23
+ 'new fork.';
24
+
25
+const oldAccessWarning =
26
+ 'Field cannot be accessed inside the new reconciler fork, only the ' +
27
+ 'old fork.';
28
+
29
+ruleTester.run('eslint-rules/no-cross-fork-types', rule, {
30
+ valid: [
31
+ {
32
+ code: `
33
+const a = obj.key_old;
34
+const b = obj.key_new;
35
+const {key_old, key_new} = obj;
36
+`,
37
+ filename: 'ReactFiberWorkLoop.js',
38
+ },
39
+ {
40
+ code: `
41
+const a = obj.key_old;
42
+const {key_old} = obj;
43
+`,
44
+ filename: 'ReactFiberWorkLoop.old.js',
45
+ },
46
+ {
47
+ code: `
48
+const a = obj.key_new;
49
+const {key_new} = obj;
50
+`,
51
+ filename: 'ReactFiberWorkLoop.new.js',
52
+ },
53
+ ],
54
+ invalid: [
55
+ {
56
+ code: 'const a = obj.key_new;',
57
+ filename: 'ReactFiberWorkLoop.old.js',
58
+ errors: [{message: newAccessWarning}],
59
+ },
60
+ {
61
+ code: 'const a = obj.key_old;',
62
+ filename: 'ReactFiberWorkLoop.new.js',
63
+ errors: [{message: oldAccessWarning}],
64
+ },
65
+
66
+ {
67
+ code: 'const {key_new} = obj;',
68
+ filename: 'ReactFiberWorkLoop.old.js',
69
+ errors: [{message: newAccessWarning}],
70
+ },
71
+ {
72
+ code: 'const {key_old} = obj;',
73
+ filename: 'ReactFiberWorkLoop.new.js',
74
+ errors: [{message: oldAccessWarning}],
75
+ },
76
+ {
77
+ code: 'const subtreeTag = obj.subtreeTag;',
78
+ filename: 'ReactFiberWorkLoop.old.js',
79
+ options: [{new: ['subtreeTag']}],
80
+ errors: [{message: newAccessWarning}],
81
+ },
82
+ {
83
+ code: 'const firstEffect = obj.firstEffect;',
84
+ filename: 'ReactFiberWorkLoop.new.js',
85
+ options: [{old: ['firstEffect']}],
86
+ errors: [{message: oldAccessWarning}],
87
+ },
88
+ ],
89
+});
scripts/eslint-rules/index.js
+1
@@ -8,5 +8,6 @@ module.exports = {
8
'invariant-args': require('./invariant-args'),
9
'no-production-logging': require('./no-production-logging'),
10
'no-cross-fork-imports': require('./no-cross-fork-imports'),
11
+ 'no-cross-fork-types': require('./no-cross-fork-types'),
12
},
13
};
scripts/eslint-rules/no-cross-fork-types.js
new
+126
@@ -0,0 +1,126 @@
1
+/**
2
+ * Copyright (c) Facebook, Inc. and its affiliates.
3
+ *
4
+ * This source code is licensed under the MIT license found in the
5
+ * LICENSE file in the root directory of this source tree.
6
+ *
7
+ * @emails react-core
8
+ */
9
+
10
+/* eslint-disable no-for-of-loops/no-for-of-loops */
11
+
12
+'use strict';
13
+
14
+function isOldFork(filename) {
15
+ return filename.endsWith('.old.js') || filename.endsWith('.old');
16
+}
17
+
18
+function isNewFork(filename) {
19
+ return filename.endsWith('.new.js') || filename.endsWith('.new');
20
+}
21
+
22
+function warnIfNewField(context, newFields, identifier) {
23
+ const name = identifier.name;
24
+ if (name.endsWith('_new') || (newFields !== null && newFields.has(name))) {
25
+ context.report({
26
+ node: identifier,
27
+ message:
28
+ 'Field cannot be accessed inside the old reconciler fork, only the ' +
29
+ 'new fork.',
30
+ });
31
+ }
32
+}
33
+
34
+function warnIfOldField(context, oldFields, identifier) {
35
+ const name = identifier.name;
36
+ if (name.endsWith('_old') || (oldFields !== null && oldFields.has(name))) {
37
+ context.report({
38
+ node: identifier,
39
+ message:
40
+ 'Field cannot be accessed inside the new reconciler fork, only the ' +
41
+ 'old fork.',
42
+ });
43
+ }
44
+}
45
+
46
+module.exports = {
47
+ meta: {
48
+ type: 'problem',
49
+ fixable: 'code',
50
+ },
51
+ create(context) {
52
+ const sourceFilename = context.getFilename();
53
+
54
+ if (isOldFork(sourceFilename)) {
55
+ const options = context.options;
56
+ let newFields = null;
57
+ if (options !== null) {
58
+ for (const option of options) {
59
+ if (option.new !== undefined) {
60
+ if (newFields === null) {
61
+ newFields = new Set(option.new);
62
+ } else {
63
+ for (const field of option.new) {
64
+ newFields.add(field);
65
+ }
66
+ }
67
+ }
68
+ }
69
+ }
70
+ return {
71
+ MemberExpression(node) {
72
+ const property = node.property;
73
+ if (property.type === 'Identifier') {
74
+ warnIfNewField(context, newFields, property);
75
+ }
76
+ },
77
+
78
+ ObjectPattern(node) {
79
+ for (const property of node.properties) {
80
+ const key = property.key;
81
+ if (key.type === 'Identifier') {
82
+ warnIfNewField(context, newFields, key);
83
+ }
84
+ }
85
+ },
86
+ };
87
+ }
88
+
89
+ if (isNewFork(sourceFilename)) {
90
+ const options = context.options;
91
+ let oldFields = null;
92
+ if (options !== null) {
93
+ for (const option of options) {
94
+ if (option.old !== undefined) {
95
+ if (oldFields === null) {
96
+ oldFields = new Set(option.old);
97
+ } else {
98
+ for (const field of option.new) {
99
+ oldFields.add(field);
100
+ }
101
+ }
102
+ }
103
+ }
104
+ }
105
+ return {
106
+ MemberExpression(node) {
107
+ const property = node.property;
108
+ if (property.type === 'Identifier') {
109
+ warnIfOldField(context, oldFields, property);
110
+ }
111
+ },
112
+
113
+ ObjectPattern(node) {
114
+ for (const property of node.properties) {
115
+ const key = property.key;
116
+ if (key.type === 'Identifier') {
117
+ warnIfOldField(context, oldFields, key);
118
+ }
119
+ }
120
+ },
121
+ };
122
+ }
123
+
124
+ return {};
125
+ },
126
+};