Merge pull request #67 from solidjs-community/fix/as-expression

Treat type casts and assertions as transparent in reactivity analysis.

Author: joshwilsonvu

docs/reactivity.md

@@ -557,10 +557,16 @@ function createCustomStore() {
   );
 }
 
+const m = createMemo(() => 5) as Accessor<number>;
+
+const m = createMemo(() => 5)!;
+
+const m = createMemo(() => 5)! as Accessor<number>;
+
 ```
 <!-- AUTO-GENERATED-CONTENT:END -->
 
-### Implementation
+## Implementation
 
 We analyze in a single pass but take advantage of ESLint's ":exit" selector
 to take action both on the way down the tree and on the way back up. At any
@@ -586,3 +592,76 @@ Notes:
   match a tracked scope that expects a function.
 - This rule ignores classes. Solid is based on functions/closures only, and 
   it's uncommon to see classes with reactivity in Solid code.
+
+## Implementation v2 (in progress)
+
+`solid/reactivity` has been public for exactly one year (!) at the time of writing, and after lots
+of feedback, testing, and changes, I've noticed a few problems with its first implementation:
+
+- **Hard to change.** All of the data structure, analysis, and reporting code is colocated in a
+  single file with all the cases for detecting signals, props, and tracked scopes based on Solid
+  APIs. There's a few edge cases where detection code is mixed with analysis code. This makes it
+  hard for contributors to make PRs and hard for others to be able to maintain it.
+- **Limited to variables.** The analysis code relies heavily on ESLint's `ScopeManager` and scope
+  utilities, and therefore it can only deal with variable references, not implicitly reactive
+  expressions.
+- **Non-extensible.** Since detection code is hardcoded in the `reactivity.ts` file, it is plainly
+  not possible for users to mark certain APIs as reactive in some way.
+- **Susceptible to timing issues.** I'm very proud of the rule's stack-based algorithm for running
+  signal/props/tracked scope detection and reactivity analysis in a single pass. Its performance is
+  going to be extremely difficult to beat. But the single-pass approach puts complex requirements on
+  the detection phase—signals and props have to be marked before nested `function:exit`s, relying on
+  initialization before usage in source order. That's not a requirement I feel comfortable putting on
+  plugin authors, or really even myself in a few months.
+
+So, I've decided to partially rewrite the rule with a plugin architecture to alleviate these issues.
+Both the core detection code and any plugins to alter detection will use the same API.
+
+### Ease of change and extensibility: Plugins (Customizations)
+
+`solid/reactivity`, itself part of an ESLint plugin, will support plugins of its own.
+
+`eslint-plugin-solid` will expose a CLI command `eslint-plugin-solid` that searches
+`package.json` files in the current working directory and its `node_modules` for a
+`"solid/reactivity"` key at the top level (raw string search first for perf). This key will be expected to
+contain a relative path to a CommonJS or native ESM file, accessible from requiring a subpath of the
+module. For example:
+
+```ts
+const packageJson = { "solid/reactivity": "./reactivity-plugin.js" };
+require.resolve(`${packageJson.name}/${packageJson["solid/reactivity"]}`);
+// path to reactivity plugin
+```
+
+The command will not run any code in `node_modules`; it will just print out an example ESLint config for
+the `solid/reactivity` rule, configured to load all plugins found. For example:
+
+```json
+"solid/reactivity": [1, {
+  "plugins": ["./node_modules/some-module-with-plugin/solid-reactivity-plugin.cjs"]
+}]
+```
+
+This code can be inspected to ensure it matches expections, or edited to add additional paths to
+more plugins. You can manually configure a particular path as a plugin without running the CLI at
+all. At runtime, any plugins configured will be loaded and run alongside the base rules. Custom
+hooks (`use*`/`create*`) from imported from these packages will not be treated permissively, others
+will.
+
+> `eslint-plugin-solid` will **not** automatically load plugins. They must be preconfigured in an
+> ESLint config file.
+
+### Expression Support
+
+Having detection code being moved to plugins and an API boundary lets us put reference tracking into
+the analysis code, so analyzing arbitrary expressions for reactivity becomes easier. Instead of
+using `TSESLint.Scope.Reference`s only, a custom data structure can be built to handle any `Node` from
+the plugin API.
+
+### Timing
+
+This is a good opportunity to transition from a stack-based algorithm, where information is lost
+after the exit pass, to a tree-based algorithm that can capture all reactivity information in a data
+structure. By using the built tree to walk through the final analysis, and colocating references
+with their associated scopes, performance should stay good. The reactivity rule could then power 
+an editor plugin to show signals, tracked scopes, etc.

scripts/docs.ts

@@ -104,7 +104,7 @@ const buildOptions = (filename: string): string => {
   ].join("\n");
 };
 
-const pretty = (code: string) => prettier.format(code, { parser: "babel" }).trim();
+const pretty = (code: string) => prettier.format(code, { parser: "typescript" }).trim();
 const options = (options: Array<any>) =>
   options
     .map((o) =>

src/rules/reactivity.ts

@@ -14,6 +14,7 @@ import {
   isProgramOrFunctionNode,
   trackImports,
   isDOMElementName,
+  ignoreTransparentWrappers,
 } from "../utils";
 
 const { findVariable, getFunctionHeadLocation } = ASTUtils;
@@ -159,6 +160,7 @@ class ScopeStack extends Array<ScopeStackItem> {
         reference,
         declarationScope: variable.declarationScope,
       }));
+      // I don't think this is needed! Just a perf optimization
       variable.references = notInScope;
     }
   }
@@ -667,8 +669,10 @@ const rule: TSESLint.RuleModule<MessageIds, []> = {
     /** Checks VariableDeclarators, AssignmentExpressions, and CallExpressions for reactivity. */
     const checkForReactiveAssignment = (
       id: T.BindingName | T.AssignmentExpression["left"] | null,
-      init: T.Expression
+      init: T.Node
     ) => {
+      init = ignoreTransparentWrappers(init);
+
       // Mark return values of certain functions as reactive
       if (init.type === "CallExpression" && init.callee.type === "Identifier") {
         const { callee } = init;
@@ -871,8 +875,6 @@ const rule: TSESLint.RuleModule<MessageIds, []> = {
             // to updates to reactive variables; it's okay to poll the current
             // value. Consider them called-function tracked scopes for our purposes.
             pushTrackedScope(arg0, "called-function");
-          } else if (matchImport("createMutable", callee.name) && arg0) {
-            pushTrackedScope(arg0, "expression");
           } else if (matchImport("on", callee.name)) {
             // on accepts a signal or an array of signals as its first argument,
             // and a tracking function as its second
@@ -1051,10 +1053,8 @@ const rule: TSESLint.RuleModule<MessageIds, []> = {
         checkForSyncCallbacks(node);
 
         // ensure calls to reactive primitives use the results.
-        if (
-          node.parent?.type !== "AssignmentExpression" &&
-          node.parent?.type !== "VariableDeclarator"
-        ) {
+        const parent = node.parent && ignoreTransparentWrappers(node.parent, true);
+        if (parent?.type !== "AssignmentExpression" && parent?.type !== "VariableDeclarator") {
           checkForReactiveAssignment(null, node);
         }
       },

src/utils.ts

@@ -69,6 +69,17 @@ export function trace(node: T.Node, initialScope: TSESLint.Scope.Scope): T.Node
   return node;
 }
 
+/** Get the relevant node when wrapped by a node that doesn't change the behavior */
+export function ignoreTransparentWrappers(node: T.Node, up = false): T.Node {
+  if (node.type === "TSAsExpression" || node.type === "TSNonNullExpression") {
+    const next = up ? node.parent : node.expression;
+    if (next) {
+      return ignoreTransparentWrappers(next, up);
+    }
+  }
+  return node;
+}
+
 export type FunctionNode = T.FunctionExpression | T.ArrowFunctionExpression | T.FunctionDeclaration;
 const FUNCTION_TYPES = ["FunctionExpression", "ArrowFunctionExpression", "FunctionDeclaration"];
 export const isFunctionNode = (node: T.Node | null | undefined): node is FunctionNode =>

test/rules/reactivity.test.ts

@@ -1,5 +1,5 @@
 import { AST_NODE_TYPES as T } from "@typescript-eslint/utils";
-import { run } from "../ruleTester";
+import { run, tsOnlyTest } from "../ruleTester";
 import rule from "../../src/rules/reactivity";
 
 export const cases = run("reactivity", rule, {
@@ -251,6 +251,19 @@ export const cases = run("reactivity", rule, {
         (item) => ({})
       );
     }`,
+    // type casting
+    {
+      code: `const m = createMemo(() => 5) as Accessor<number>;`,
+      ...tsOnlyTest,
+    },
+    {
+      code: `const m = createMemo(() => 5)!;`,
+      ...tsOnlyTest,
+    },
+    {
+      code: `const m = createMemo(() => 5)! as Accessor<number>;`,
+      ...tsOnlyTest,
+    },
   ],
   invalid: [
     // Untracked signals